Add tests to SingleThreadedSchedulerClient() and document the memory model #13247

skeees · 2018-05-16T19:49:16Z

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).

TheBlueMatt · 2018-05-16T19:50:16Z

Can you also note the memory model in validationinterface, since that's the "real" API here.

skeees · 2018-05-16T20:20:55Z

Updated

Empact · 2018-05-16T23:53:30Z

src/test/scheduler_tests.cpp

+    // the extra threads should effectively be doing nothing
+    // if they don't we'll get out of order behaviour
+    boost::thread_group threads;
+    for (int i = 0; i < 5; i++) {


nit: ++i is preferred according to the developer-notes.

Empact · 2018-05-17T00:10:40Z

src/test/scheduler_tests.cpp

+    threads.join_all();
+
+    BOOST_CHECK_EQUAL(counter1, 100);
+    BOOST_CHECK_EQUAL(counter2, 100);


Might want to label this a sanity check, to make it more clear that the 141/145 checks are the meat of the test.

Empact · 2018-05-17T00:14:46Z

src/validationinterface.h

+ * will receive callbacks in the same order. Furthermore, each ValidationInterface
+ * subscriber may assume that callbacks effectively run in a single thread with
+ * single-threaded memory consistency. That is, for a given ValidationInterface()
+ * instantiation, each callback will complete before


nit: whitespace

TheBlueMatt · 2018-05-17T15:32:55Z

src/validationinterface.h

+/**
+ * Implement this to subscribe to validation events
+ *
+ * Events are totally ordered - that is - all CValidationInterface() subscribers


The wording here seems to imply order between different subscribers.

Ensures ordering of callbacks within a SingleThreadedSchedulerClient with respect to each other

DrahtBot · 2018-07-22T23:49:19Z

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

utACK 9b6848e

TheBlueMatt · 2018-07-30T22:22:00Z

src/scheduler.h

- * at the same time.
+ * which are required to be run serially. Jobs may not be run on the
+ * same thread, but no two jobs will be executed
+ * at the same time and memory will be sequentially consistent.


Sequentially consistent of release-acquire? I think release-acquire is all we should guarantee.

updated, thanks

…he memory model

…ading and memory model

skeees · 2018-07-30T23:44:49Z

Updated to address @TheBlueMatt (diff is a comment change only)

skeees · 2018-07-31T17:30:08Z

Test failure was unrelated and now resolved

jamesob · 2018-07-31T17:48:28Z

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

fe7180c [trivial,doc] Fix memory consistency model in comment (Jesse Cohen) Pull request description: Updating a comment overlooked during review in #13247 Tree-SHA512: 0bd54ba1c265fdd77fd6e12ad0be46dd422348f7d926ce9abaca53fdb3a3c55c0d1cd90b4382321352076f4a81e2249c0014cd789f47a3637cb93bd983cb4657

… 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

… 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

… 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

skeees force-pushed the scheduler-tests branch from bef0b29 to 9c9e203 Compare May 16, 2018 20:38

fanquake added the Tests label May 16, 2018

Empact reviewed May 16, 2018

View reviewed changes

Empact reviewed May 17, 2018

View reviewed changes

TheBlueMatt reviewed May 17, 2018

View reviewed changes

Add Unit Test for SingleThreadedSchedulerClient

9994d01

Ensures ordering of callbacks within a SingleThreadedSchedulerClient with respect to each other

skeees force-pushed the scheduler-tests branch from 9c9e203 to 9b6848e Compare May 18, 2018 16:37

fanquake added this to the 0.17.0 milestone Jul 18, 2018

maflcko requested a review from TheBlueMatt July 22, 2018 01:25

DrahtBot closed this Jul 22, 2018

DrahtBot reopened this Jul 22, 2018

maflcko removed this from the 0.17.0 milestone Jul 30, 2018

jamesob approved these changes Jul 30, 2018

View reviewed changes

TheBlueMatt reviewed Jul 30, 2018

View reviewed changes

skeees added 2 commits July 30, 2018 19:41

Update documentation for SingleThreadedSchedulerClient() to specify t…

b296b42

…he memory model

Update ValidationInterface() documentation to explicitly specify thre…

cbeaa91

…ading and memory model

skeees force-pushed the scheduler-tests branch from 9b6848e to cbeaa91 Compare July 30, 2018 23:42

maflcko merged commit cbeaa91 into bitcoin:master Aug 1, 2018

skeees mentioned this pull request Aug 1, 2018

[trivial,doc] Fix memory consistency model in comment #13835

Merged

Bushstar mentioned this pull request Aug 13, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#358

Merged

maflcko mentioned this pull request Aug 24, 2018

Enable -fsanitize flags in Travis #12691

Closed

furszy mentioned this pull request Apr 2, 2021

[Validation][Test] Fix concurrency issues in ActivateBestChain + optimization PIVX-Project/PIVX#2290

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add tests to SingleThreadedSchedulerClient() and document the memory model #13247

Add tests to SingleThreadedSchedulerClient() and document the memory model #13247

Uh oh!

skeees commented May 16, 2018 •

edited by DrahtBot

Loading

Uh oh!

TheBlueMatt commented May 16, 2018

Uh oh!

skeees commented May 16, 2018

Uh oh!

Empact May 16, 2018

Uh oh!

Empact May 17, 2018

Uh oh!

Empact May 17, 2018

Uh oh!

TheBlueMatt May 17, 2018

Uh oh!

DrahtBot commented Jul 22, 2018

Uh oh!

jamesob left a comment

Uh oh!

TheBlueMatt Jul 30, 2018

Uh oh!

skeees Jul 30, 2018

Uh oh!

skeees commented Jul 30, 2018

Uh oh!

skeees commented Jul 31, 2018

Uh oh!

jamesob commented Jul 31, 2018

Uh oh!

Uh oh!

Add tests to SingleThreadedSchedulerClient() and document the memory model #13247

Add tests to SingleThreadedSchedulerClient() and document the memory model #13247

Uh oh!

Conversation

skeees commented May 16, 2018 • edited by DrahtBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented May 16, 2018

Uh oh!

skeees commented May 16, 2018

Uh oh!

Empact May 16, 2018

Choose a reason for hiding this comment

Uh oh!

Empact May 17, 2018

Choose a reason for hiding this comment

Uh oh!

Empact May 17, 2018

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt May 17, 2018

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 22, 2018

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

skeees Jul 30, 2018

Choose a reason for hiding this comment

Uh oh!

skeees commented Jul 30, 2018

Uh oh!

skeees commented Jul 31, 2018

Uh oh!

jamesob commented Jul 31, 2018

Uh oh!

Uh oh!

skeees commented May 16, 2018 •

edited by DrahtBot

Loading