Refactor write batching #143
Conversation
Avoid holding transaction-specific state in the handler, which must then be protected by multiple locks. This is error prone and is a potential performance bottleneck. Instead, extract batch writes to a separate struct for clarity, and maintain the batch write state in the stub, which is unique to a specific transaction so requires no locking. The stub (and associated batch write state) is also discarded after a transaction invocation so there is no risk of memory leaks due to redundant batch write state being left in the shared handler. Signed-off-by: Mark S. Lewis <[email protected]>
|
I'm not sure I trust the unit tests enough to be confident this refactor maintains exactly the previous behaviour. It needs a good eyeball and perhaps trying out in actual use. Presented for review at least to get feedback on the significantly simplified implementation approach. |
|
I've have pushed an additional commit to address the bugs you highlighted and the key issue that I identified. I think the fact that significant functional differences are not caught by the unit tests while refactoring the implementation highlights that the unit tests are insufficient. |
- Check whether batching is supported when starting a batch. - Better keying of batched writes. Signed-off-by: Mark S. Lewis <[email protected]>
bestbeforetoday
force-pushed
the
batch-refactor
branch
from
ab11b92 to
ce5a571
Compare
Signed-off-by: Mark S. Lewis <[email protected]>
bestbeforetoday
force-pushed
the
batch-refactor
branch
from
f8cfe3c to
126c4a8
Compare
When I wrote, I added some by analogy. Probably not enough. Can you prepare a list of those unit tests that you would like to see. And as soon as we finish with your pr, I will implement them. |
Every comment you have raised on this PR identifying a behavioural issue really should have been caught by unit tests. That would be a good list to work with. |
- Preallocate writes slice. Signed-off-by: Mark S. Lewis <[email protected]>
Avoid holding transaction-specific state in the handler, which must then be protected by multiple locks. This is error prone and is a potential performance bottleneck. Instead, extract batch writes to a separate struct for clarity, and maintain the batch write state in the stub, which is unique to a specific transaction so requires no locking.
The stub (and associated batch write state) is also discarded after a transaction invocation so there is no risk of memory leaks due to redundant batch write state being left in the shared handler.