etcd: STM transaction queue to effectively reduce retries for conflicting transactions

[bhandras]

rebased on #4411
now on master since #4411 is merged

This PR adds (and integrates) commitQueue which' purpose is to detect conflicts for concurrently applied transactions and effectively reduce retries, by applying queuing up conflicting transactions for sequential execution, while leaving all non-conflicting ones to run freely (potentially in parallel).

[Roasbeef]

The implementation is much simpler than I thought it would be! Just completed an initial pass, and nothing glaring jumped out. Will do another pass once I run it on an actual replicated db lnd instance. It would also be interesting to create a small patch that lets us run certain itests w/ and w/o this change so we can gauge the rough impact of the change on perf.

[Roasbeef]

Why don't we need to block if there's a pending transaction in the queue that reads this key, we want to write it, but don't also read the key ourselves?

[bhandras]

It's because if our read set contains the key then we already increased c.readerMap[key] above, so to make sure reader lock count is non zero we have to "uncount ourselves".

[halseth]

I think this would be much easier to reason about by just by doing two passes through the sets:

for key := range rset {
    blocked |= c.writerMap[key] > 0
}
for key := range wset {
    blocked |= c.writerMap[key] > 0 || c.readerMap[key] > 0
}
for key := range rset {
    c.readerMap[key] += 1
}
for key := range wset {
    c.writerMap[key] += 1
}

Performance wise I doubt we'll see any difference.

[bhandras]

Yeah, it's a bit hard to read... Unfortunately we can't use the the simplified version above because if the same transaction also reads the key (where no other readers are present) then will unnecessary block. This is the reason for the rsetContainsKey variable. Added a few comments to clarify. I'm open to any suggestions you may find that simplifies though.

[bhandras]

** and also simplified a bit to make it more? readable

[cfromknecht]

very cool how small the diff is!

[cfromknecht]

is it intentional that this shadows the err in the outer scope? o/w i don't see where that error is read?

[bhandras]

Yes, so it's a bit tricky to read this at first, but it's really simple actually.

1. What we do is we first run the apply closure to gather the read/write sets so we can add the tx to the contention queue.
2. The execute closure is executed there (either immediately or in the queue goroutine).

The err simply holds the error trough the above described execution graph.
3) we wait for the done signal and them clean the keys from the queue.

[cfromknecht]

thanks for the explanation, makes sense now! i also see that it's the return value at the end of the function, so that's where it is "read"

[halseth]

Since we return immediately if the error is non-nil here, wouldn't if err := apply(s); err != nil be equivalent?

I agree that the shadowing is tricky to read. Could make sense to add more errors with descriptive names (i.e. executeErr) to make it easier.

[bhandras]

Yeah, maybe it's simpler to read if we distinguish errors by scope. PTAL

[bhandras]

The implementation is much simpler than I thought it would be! Just completed an initial pass, and nothing glaring jumped out. Will do another pass once I run it on an actual replicated db lnd instance. It would also be interesting to create a small patch that lets us run certain itests w/ and w/o this change so we can gauge the rough impact of the change on perf.

Yes, originally the queue was optional but decided to make it non-optional as really it should be on all the time.
We can still do the comparison, as it just requires removing these commits from the itest PR (#4402)

[cfromknecht]

LGTM 🌮

[halseth]

Fun change :)

[halseth]

style suggestion: blocked ||= c.writerMap[key] > 0

[bhandras]

done

[halseth]

Since we return immediately if the error is non-nil here, wouldn't if err := apply(s); err != nil be equivalent?

I agree that the shadowing is tricky to read. Could make sense to add more errors with descriptive names (i.e. executeErr) to make it easier.

[halseth]

use the more common waitgroup pattern instead?

[bhandras]

done

[halseth]

I think this would be much easier to reason about by just by doing two passes through the sets:

for key := range rset {
    blocked |= c.writerMap[key] > 0
}
for key := range wset {
    blocked |= c.writerMap[key] > 0 || c.readerMap[key] > 0
}
for key := range rset {
    c.readerMap[key] += 1
}
for key := range wset {
    c.writerMap[key] += 1
}

Performance wise I doubt we'll see any difference.

[bhandras]

Thanks for the review @halseth! Main change is the (hopefully) more readable rset/wset scans. PTAL

[bhandras]

done

[bhandras]

Yeah, it's a bit hard to read... Unfortunately we can't use the the simplified version above because if the same transaction also reads the key (where no other readers are present) then will unnecessary block. This is the reason for the rsetContainsKey variable. Added a few comments to clarify. I'm open to any suggestions you may find that simplifies though.

[bhandras]

done

[bhandras]

Yeah, maybe it's simpler to read if we distinguish errors by scope. PTAL

[halseth]

still not sure if this is correct. Say this tx reads and writes this key, increases c.readerMap[key] to 2.

That will leave (c.readerMap[key] > 0 && !keyRead) == false while it should be blocked.

[bhandras]

That was a really nice catch!

[bhandras]

After some back and forth, decided to go with the simplified version above just with three loops.

[Roasbeef]

I found the latest iteration much easier to reason about this time around, kudos to the prior reviewers in this series!

LGTM 🚁

Should wait to merge this till we get 3/3 since the last iteration had a nice find.

[Roasbeef]

👍

[halseth]

LGTM now, great work! 😀

[halseth]

👍

[halseth]

nit: optimization here and below, can immediately break loop if already blocked.

[bhandras]

done

[bhandras]

Thanks everyone for the reviews!