-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Add tests to SingleThreadedSchedulerClient() and document the memory model #13247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Can you also note the memory model in validationinterface, since that's the "real" API here. |
|
Updated |
src/test/scheduler_tests.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ++i is preferred according to the developer-notes.
src/test/scheduler_tests.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to label this a sanity check, to make it more clear that the 141/145 checks are the meat of the test.
src/validationinterface.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: whitespace
src/validationinterface.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording here seems to imply order between different subscribers.
Ensures ordering of callbacks within a SingleThreadedSchedulerClient with respect to each other
| The last travis run for this pull request was 65 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened. |
jamesob
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 9b6848e
src/scheduler.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sequentially consistent of release-acquire? I think release-acquire is all we should guarantee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thanks
…ading and memory model
|
Updated to address @TheBlueMatt (diff is a comment change only) |
|
Test failure was unrelated and now resolved |
|
re-utACK cbeaa91 |
…nt the memory model cbeaa91 Update ValidationInterface() documentation to explicitly specify threading and memory model (Jesse Cohen) b296b42 Update documentation for SingleThreadedSchedulerClient() to specify the memory model (Jesse Cohen) 9994d01 Add Unit Test for SingleThreadedSchedulerClient (Jesse Cohen) Pull request description: As discussed in #13023 I've split this test out into a separate pr This test (and documentation update) makes explicit the guarantee (previously undefined, but implied by the 'SingleThreaded' in `SingleThreadedSchedulerClient()`) - that callbacks pushed to the `SingleThreadedSchedulerClient()` obey the single threaded model for memory and execution - specifically, the callbacks are executed fully and in order, and even in cases where a subsequent callback is executed by a different thread, sequential consistency of memory for all threads executing these callbacks is maintained. Maintaining memory consistency should make the api more developer friendly - especially for users of the validationinterface. To the extent that there are performance implications from this decision, these are not currently present in practice because all use of this scheduler happens on a single thread currently, furthermore the lock should guarantee consistency across callback executions even when callbacks are executed by multiple threads (as the test does). Tree-SHA512: 5d95a7682c402e5ad76b05bc9dfbca99ca64105f62ab9e78f6fc0f6ea8c5277aa399fbb94298e35cc677b0c2181ff17259584bb7ae230e38aa68b85ecbc22856
… document the memory model cbeaa91 Update ValidationInterface() documentation to explicitly specify threading and memory model (Jesse Cohen) b296b42 Update documentation for SingleThreadedSchedulerClient() to specify the memory model (Jesse Cohen) 9994d01 Add Unit Test for SingleThreadedSchedulerClient (Jesse Cohen) Pull request description: As discussed in bitcoin#13023 I've split this test out into a separate pr This test (and documentation update) makes explicit the guarantee (previously undefined, but implied by the 'SingleThreaded' in `SingleThreadedSchedulerClient()`) - that callbacks pushed to the `SingleThreadedSchedulerClient()` obey the single threaded model for memory and execution - specifically, the callbacks are executed fully and in order, and even in cases where a subsequent callback is executed by a different thread, sequential consistency of memory for all threads executing these callbacks is maintained. Maintaining memory consistency should make the api more developer friendly - especially for users of the validationinterface. To the extent that there are performance implications from this decision, these are not currently present in practice because all use of this scheduler happens on a single thread currently, furthermore the lock should guarantee consistency across callback executions even when callbacks are executed by multiple threads (as the test does). Tree-SHA512: 5d95a7682c402e5ad76b05bc9dfbca99ca64105f62ab9e78f6fc0f6ea8c5277aa399fbb94298e35cc677b0c2181ff17259584bb7ae230e38aa68b85ecbc22856
…mment fe7180c [trivial,doc] Fix memory consistency model in comment (Jesse Cohen) Pull request description: Updating a comment overlooked during review in bitcoin#13247 Tree-SHA512: 0bd54ba1c265fdd77fd6e12ad0be46dd422348f7d926ce9abaca53fdb3a3c55c0d1cd90b4382321352076f4a81e2249c0014cd789f47a3637cb93bd983cb4657
…tChain + optimization 50dbec5 Add unit tests for signals generated by ProcessNewBlock() (furszy) f68251d Validation: rename one of the two instances using "bad-prevblk" to its correct description of "prevblk-not-found" (furszy) a51a755 Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen) 198f435 Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo) 8d15cf5 Optimize ActivateBestChain for long chains (Pieter Wuille) 8640be1 Fix fast-shutdown crash if genesis block was not loaded (Matt Corallo) ef24337 Hold cs_main while calling UpdatedBlockTip() and ui.NotifyBlockTip (Jesse Cohen) f9d2ab3 Update ValidationInterface() documentation to explicitly specify threading and memory model (Jesse Cohen) cb9bb25 Update documentation for SingleThreadedSchedulerClient() to specify the memory model (Jesse Cohen) 4ea2048 Add Unit Test for SingleThreadedSchedulerClient (Jesse Cohen) Pull request description: Made some back ports and adaptations to validate further the work introduced in #2203 and #2209. Now that the scheduler thread is processing most of the chain events, the callbacks execution order must be verified to dispatch the events serially, ensuring a consistent memory state in each event processing invocation. There are some concurrency issues solved, as well a an optimization for larger chains for the ABC workflow. Each commit details them well. List: bitcoin#7917 (only fb8fad1) bitcoin#12988 bitcoin#13023 bitcoin#13247 bitcoin#13835 This is built on top of #2284 (and the reason that made me notice that problem). So, 2284 comes first, then this one. ACKs for top commit: Fuzzbawls: ACK 50dbec5 random-zebra: ACK 50dbec5 and merging... Tree-SHA512: 573144cc96d2caa49ed2f0887dc8c53b5aca0efd54b6a25626063ccb780da426f56b3c6b7491648e7abefc1960c82b1354a4ff2c51425bfe99adaa4394562608
As discussed in #13023 I've split this test out into a separate pr
This test (and documentation update) makes explicit the guarantee (previously undefined, but implied by the 'SingleThreaded' in
SingleThreadedSchedulerClient()) - that callbacks pushed to theSingleThreadedSchedulerClient()obey the single threaded model for memory and execution - specifically, the callbacks are executed fully and in order, and even in cases where a subsequent callback is executed by a different thread, sequential consistency of memory for all threads executing these callbacks is maintained.Maintaining memory consistency should make the api more developer friendly - especially for users of the validationinterface. To the extent that there are performance implications from this decision, these are not currently present in practice because all use of this scheduler happens on a single thread currently, furthermore the lock should guarantee consistency across callback executions even when callbacks are executed by multiple threads (as the test does).