[BlockSTM] Lazy (potentially iterable) txns input to BlockSTM #14568
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
⏱️ 1h 40m total CI duration on this PR
|
manudhundi
requested review from
msmouse,
lightmark,
grao1991,
gelash,
zekun000,
sasha8,
danielxiangzl,
davidiw,
wrwg,
vgao1996 and
georgemitenkov
as code owners
manudhundi
force-pushed
the
manu/BlockSTM_iterator
branch
from
f8ed8c1
to
8ba7e89
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
manudhundi
force-pushed
the
manu/BlockSTM_iterator
branch
from
8ba7e89
to
ba24673
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
manudhundi
changed the title
gelash
reviewed
Sep 25, 2024
gelash
reviewed
Sep 26, 2024
| match *status { | ||
| BlockingTransactionStatus::Ready(ref txn) => txn.clone(), | ||
| BlockingTransactionStatus::Waiting => { | ||
| status = txn.cvar.wait(status).unwrap(); |
There was a problem hiding this comment.
from the documentation of condvar wait:
Note that this function is susceptible to spurious wakeups. Condition variables normally have a boolean predicate associated with them, and the predicate must always be checked each time this function returns to protect against spurious wakeups.
This can lead to panic on Line 70 below. A typical pattern is to do it in a loop, we have an example here:
DependencyCondvar defined here:
I think what we can do here is have separate condvar with a status lock, use in a loop, and separately the (option of) transaction (or ExplicitSyncWrapper of Option if it needs sync bounds), so we can have an interface for naked &.
An Arc isn't really a big issue but in general I want us to try to reduce reliance on them in cases they aren't actually needed (lifetime guaranteed and reference counting not needed), and this seems like a good example of such a case.
There was a problem hiding this comment.
@ziaptos: minor question here out of curiosity, if it's not too easy to change to non-arc, would it be better to return &Arc? (or triopmhe arc?)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
manudhundi
force-pushed
the
manu/BlockSTM_iterator
branch
from
01d817c
to
22df6f2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Currently BlockSTM takes in a block (vec) of txns and executes them. This commits adds a capability where we don't need to provide all the txns in the block upfront, rather provide them as per any desired logic in the system. The commit has a default implementation 'DefaultTxnProvider' where all txns are provided upfront as per current logic, and also a reference implementation of 'BlockingTxnsProvider' where txns can be provided after BlockSTM starts execution. Note: One should be careful while using 'BlockingTxnsProvider' because if BlockSTM chooses to execute a txn that is not yet provided, then that thread gets blocked until such a txn is provided. This could lead to performance degradation.
manudhundi
force-pushed
the
manu/BlockSTM_iterator
branch
from
22df6f2
to
2815641
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
rahxephon89
pushed a commit
that referenced
this pull request
Nov 15, 2024
* [BlockSTM] Iterable txns input to BlockSTM Currently BlockSTM takes in a block (vec) of txns and executes them. This commits adds a capability where we don't need to provide all the txns in the block upfront, rather provide them as per any desired logic in the system. The commit has a default implementation 'DefaultTxnProvider' where all txns are provided upfront as per current logic, and also a reference implementation of 'BlockingTxnsProvider' where txns can be provided after BlockSTM starts execution. This is done by rust's OnceCell<>. Note: One should be careful while using 'BlockingTxnsProvider' because if BlockSTM chooses to execute a txn that is not yet provided, then that thread gets blocked until such a txn is provided. This could lead to performance degradation. * Address issues that can arise from spurious wakeups in Condvar * In BlockingTransaction use OnceCell<> instead of Mutex and Cvar * Keep BlockingTransaction internal to BlockingTxnProvider * Remove BlockingTransaction struct; instead use OnceCell directly
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently BlockSTM takes in a block (vec) of txns and executes them.
This commits adds a capability where we don't need to provide all the
txns in the block upfront, rather provide them as per any desired logic
in the system.
The commit has a default implementation 'DefaultTxnProvider' where all
txns are provided upfront as per current logic, and also a reference
implementation of 'BlockingTxnsProvider' where txns can be provided
after BlockSTM starts execution.
Note: One should be careful while using 'BlockingTxnsProvider' because
if BlockSTM chooses to execute a txn that is not yet provided, then that
thread gets blocked until such a txn is provided. This could lead to
performance degradation.
Description
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
Checklist