Skip to content

Save producerIndex load while polling a pooled chunk on mpmc xadd q#269

Merged
nitsanw merged 1 commit into
JCTools:masterfrom
franz1981:mpmc_xadd_poll
Nov 25, 2019
Merged

Save producerIndex load while polling a pooled chunk on mpmc xadd q#269
nitsanw merged 1 commit into
JCTools:masterfrom
franz1981:mpmc_xadd_poll

Conversation

@franz1981

@franz1981 franz1981 commented Nov 5, 2019

Copy link
Copy Markdown
Collaborator

The change on poll introduced by fc29784 has caused a performance regression due to an increased number of producerIndex loads.
This PR is using consumerBuffer::index to save some producerIndex loads, while preserving correctness of fast fail (ie return null) during a rotation if there are no other elements but the first one in the next chunk.

Some results by running

$ java -jar microbenchmarks.jar org.jctools.jmh.throughput.QueueThroughputBackoffNone -f 10 -p qType=MpmcUnboundedXaddArrayQueue,MpmcArrayQueue -p qCapacity=131072 -tg 5,5

master:

Benchmark                                    (qCapacity)                      (qType)   Mode  Cnt   Score   Error   Units
QueueThroughputBackoffNone.tpt                    131072  MpmcUnboundedXaddArrayQueue  thrpt  100  18.042 ± 0.290  ops/us
QueueThroughputBackoffNone.tpt:offer              131072  MpmcUnboundedXaddArrayQueue  thrpt  100  14.393 ± 0.279  ops/us
QueueThroughputBackoffNone.tpt:offersFailed       131072  MpmcUnboundedXaddArrayQueue  thrpt  100     ≈ 0          ops/us
QueueThroughputBackoffNone.tpt:offersMade         131072  MpmcUnboundedXaddArrayQueue  thrpt  100  14.410 ± 0.280  ops/us
QueueThroughputBackoffNone.tpt:poll               131072  MpmcUnboundedXaddArrayQueue  thrpt  100   3.649 ± 0.048  ops/us
QueueThroughputBackoffNone.tpt:pollsFailed        131072  MpmcUnboundedXaddArrayQueue  thrpt  100   0.001 ± 0.001  ops/us
QueueThroughputBackoffNone.tpt:pollsMade          131072  MpmcUnboundedXaddArrayQueue  thrpt  100   3.654 ± 0.048  ops/us
QueueThroughputBackoffNone.tpt                    131072               MpmcArrayQueue  thrpt  100   8.911 ± 0.270  ops/us
QueueThroughputBackoffNone.tpt:offer              131072               MpmcArrayQueue  thrpt  100   4.460 ± 0.141  ops/us
QueueThroughputBackoffNone.tpt:offersFailed       131072               MpmcArrayQueue  thrpt  100   0.029 ± 0.031  ops/us
QueueThroughputBackoffNone.tpt:offersMade         131072               MpmcArrayQueue  thrpt  100   4.435 ± 0.145  ops/us
QueueThroughputBackoffNone.tpt:poll               131072               MpmcArrayQueue  thrpt  100   4.451 ± 0.134  ops/us
QueueThroughputBackoffNone.tpt:pollsFailed        131072               MpmcArrayQueue  thrpt  100   0.045 ± 0.020  ops/us
QueueThroughputBackoffNone.tpt:pollsMade          131072               MpmcArrayQueue  thrpt  100   4.410 ± 0.144  ops/us

this PR:

Benchmark                                    (qCapacity)                      (qType)   Mode  Cnt   Score   Error   Units
QueueThroughputBackoffNone.tpt                    131072  MpmcUnboundedXaddArrayQueue  thrpt  100  24.164 ± 0.660  ops/us
QueueThroughputBackoffNone.tpt:offer              131072  MpmcUnboundedXaddArrayQueue  thrpt  100  19.203 ± 0.559  ops/us
QueueThroughputBackoffNone.tpt:offersFailed       131072  MpmcUnboundedXaddArrayQueue  thrpt  100     ≈ 0          ops/us
QueueThroughputBackoffNone.tpt:offersMade         131072  MpmcUnboundedXaddArrayQueue  thrpt  100  19.223 ± 0.559  ops/us
QueueThroughputBackoffNone.tpt:poll               131072  MpmcUnboundedXaddArrayQueue  thrpt  100   4.961 ± 0.124  ops/us
QueueThroughputBackoffNone.tpt:pollsFailed        131072  MpmcUnboundedXaddArrayQueue  thrpt  100   0.001 ± 0.001  ops/us
QueueThroughputBackoffNone.tpt:pollsMade          131072  MpmcUnboundedXaddArrayQueue  thrpt  100   4.968 ± 0.124  ops/us
QueueThroughputBackoffNone.tpt                    131072               MpmcArrayQueue  thrpt  100   9.535 ± 0.251  ops/us
QueueThroughputBackoffNone.tpt:offer              131072               MpmcArrayQueue  thrpt  100   4.764 ± 0.130  ops/us
QueueThroughputBackoffNone.tpt:offersFailed       131072               MpmcArrayQueue  thrpt  100   0.001 ± 0.002  ops/us
QueueThroughputBackoffNone.tpt:offersMade         131072               MpmcArrayQueue  thrpt  100   4.766 ± 0.131  ops/us
QueueThroughputBackoffNone.tpt:poll               131072               MpmcArrayQueue  thrpt  100   4.771 ± 0.121  ops/us
QueueThroughputBackoffNone.tpt:pollsFailed        131072               MpmcArrayQueue  thrpt  100   0.018 ± 0.008  ops/us
QueueThroughputBackoffNone.tpt:pollsMade          131072               MpmcArrayQueue  thrpt  100   4.756 ± 0.128  ops/us

These results show that saving unecessary producerIndex loads on rotation can make a lot of difference, hence #265 could be very beneficial to be implemented for a future release.

@coveralls

coveralls commented Nov 5, 2019 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 580

  • 15 of 18 (83.33%) changed or added relevant lines in 1 file are covered.
  • 19 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.02%) to 85.421%
Changes Missing Coverage Covered Lines Changed/Added Lines %
jctools-core/src/main/java/org/jctools/queues/MpmcUnboundedXaddArrayQueue.java 15 18 83.33%
Files with Coverage Reduction New Missed Lines %
jctools-core/src/main/java/org/jctools/maps/NonBlockingHashMap.java 1 79.65%
jctools-core/src/main/java/org/jctools/maps/NonBlockingIdentityHashMap.java 5 76.31%
jctools-core/src/main/java/org/jctools/maps/NonBlockingHashMapLong.java 13 78.89%
Totals Coverage Status
Change from base Build 577: 0.02%
Covered Lines: 4781
Relevant Lines: 5597
💛 - Coveralls

@lgtm-com

lgtm-com Bot commented Nov 5, 2019

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 687a3fb into 671c0a8 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test

@lgtm-com

lgtm-com Bot commented Nov 5, 2019

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 75793bb into 671c0a8 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test

@franz1981

franz1981 commented Nov 6, 2019

Copy link
Copy Markdown
Collaborator Author

@nitsanw this is ready to be reviewed/merged
Sorry for this sudden change, but I haven't had available boxes until yesterday to perform some reliable regression tests

@franz1981

franz1981 commented Nov 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@nitsanw the travis build is pending, but I see on the CI that d30b4d1 (validated) has the same content of 3322e9f (this PR), any idea what's happening?

@franz1981

franz1981 commented Nov 6, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@nitsanw Wait a sec: I see this

INFO] Running org.jctools.queues.MpqSanityTestMpscUnboundedXadd
Exception in thread "Thread-274" java.lang.AssertionError
	at org.jctools.queues.MpscUnboundedXaddArrayQueue.relaxedPeek(MpscUnboundedXaddArrayQueue.java:590)
	at org.jctools.queues.MpqSanityTest$6.run(MpqSanityTest.java:488)
	at java.lang.Thread.run(Thread.java:748)

This hasn't happened before, but seems present from the beginning, I've alread sent #270 to fix it

@franz1981

franz1981 commented Nov 8, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

@nitsanw Despite what CI says (see comment above), this is ready to be reviewed/merged

@franz1981 franz1981 mentioned this pull request Nov 16, 2019
Merged
@nitsanw nitsanw merged commit b178dfd into JCTools:master Nov 25, 2019
@franz1981 franz1981 mentioned this pull request Dec 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@franz1981 @coveralls @nitsanw