Reset shared_future in FutureBucket when it's ready #2303
Conversation
| @@ -299,7 +315,7 @@ FutureBucket::startMerge(Application& app, uint32_t maxProtocolVersion, | |||
|
|
|||
| assert(curr); | |||
| assert(snap); | |||
| assert(!mOutputBucket.valid()); | |||
| assert(!mOutputBucketFuture.valid()); | |||
There was a problem hiding this comment.
do we also need assert(!mOutputBucket) ?
There was a problem hiding this comment.
Good idea, fixed (plus one other small simplification). If the change looks ok, I can squash the commits.
| @@ -59,7 +59,8 @@ class FutureBucket | |||
| std::shared_ptr<Bucket> mInputCurrBucket; | |||
| std::shared_ptr<Bucket> mInputSnapBucket; | |||
| std::vector<std::shared_ptr<Bucket>> mInputShadowBuckets; | |||
| std::shared_future<std::shared_ptr<Bucket>> mOutputBucket; | |||
| std::shared_ptr<Bucket> mOutputBucket; | |||
| std::shared_future<std::shared_ptr<Bucket>> mOutputBucketFuture; | |||
There was a problem hiding this comment.
We do not test the internals of FutureBucket, only its public interface, which functionally did not change (also, i initially had a test that verified that use_count remains the same when FutureBucket is resolved, but turns out that use_count returns an approximate value in the presence of multiple threads. Happy to add it back though if you prefer)
src/bucket/FutureBucket.cpp
Outdated
| } | ||
|
|
||
| if (mOutputBucketHash.empty()) | ||
| { | ||
| mOutputBucketHash = binToHex(bucket->getHash()); | ||
| mOutputBucketHash = binToHex(mOutputBucket->getHash()); |
There was a problem hiding this comment.
mOutputBucket is only set above, right?
So why not setting mOutputBucketHash in the same place?
Actually maybe resolve should just start with if (LIVE_OUTPUT) { return mOutputBucket; } right now the code is written in an almost idempotent way, which doesn't make much sense
marta-lokhova
force-pushed
the
abandon_shared_future
branch
from
727cb98
to
d50d4a6
Compare
|
r+ d50d4a6 |
Reset shared_future in FutureBucket when it's ready Reviewed-by: MonsieurNicolas
This should resolve #2298 -- some standard library implementations (libstdc++) store task lambdas in the shared state, which we used to keep alive for a while (specifically, until level spill which could take months for low level buckets). This change explicitly resets shared_future once it's done.
The second change is a minor update to catchup, to stop referencing buckets earlier (currently we drop bucket references at the very end of a potentially long CatchupWork)