Skip to content
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

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

Merged
merged 1 commit into from Nov 25, 2019

Conversation

franz1981
Copy link
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
Copy link

coveralls commented Nov 5, 2019

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
Copy link

lgtm-com bot commented Nov 5, 2019

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

new alerts:

  • 1 for Useless comparison test

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2019

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

new alerts:

  • 1 for Useless comparison test

@franz1981
Copy link
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
Copy link
Collaborator Author

franz1981 commented Nov 6, 2019

@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
Copy link
Collaborator Author

franz1981 commented Nov 6, 2019

@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
Copy link
Collaborator Author

franz1981 commented Nov 8, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants