You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by "jhuynh1 (GitHub)" <gi...@apache.org> on 2019/02/14 22:00:26 UTC
[GitHub] [geode] jhuynh1 opened pull request #3196: GEODE-6404: work
around possible sync issue with computeIfAbsent
…Absent
* Tries to do the get() outside of a lock before computing
Please suggest a better name than JavaWorkarounds... I just haven't thought of anything good...
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on issue #3196:
GEODE-6404: work around possible sync issue with computeIfAbsent
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Given that Java 9+ has fixed this I’d suggest a check here for the JVM version and disable if 9+.
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 closed pull request #3196: GEODE-6404: work
around possible sync issue with computeIfAbsent
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
[ pull request closed by jhuynh1 ]
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #3196:
GEODE-6404: work around possible sync issue with computeIfAbsent
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
Maybe out of scope here, but this is the second set of performance tests that test concurrency in this way (generate load in setup before testing). I wonder if there is an opportunity to create a shared component which abstracts away/shares some of this logic?
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on issue #3196: GEODE-6404: work
around possible sync issue with computeIfAbsent
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
Using the added benchmark on my local mac I had the following results:
Java 1.8
Old way (straight computeIfAbsent)
ComputeIfAbsentBenchmark.monitorQuery thrpt 25 597,050.898 ± 23260.717 ops/s
ComputeIfAbsentBenchmark.monitorQuery thrpt 10 851,289.826 ± 27422.826 ops/s
New way (doing get)
ComputeIfAbsentBenchmark.monitorQuery thrpt 10 33,338,554.778 ± 2,533,816.655 ops/s
(Ran a few more times but forgot to log the results, they were consistent with being exponentially faster)
Java 11 (compiled against java 1.8)
Old way (straight computeIfAbsent)
ComputeIfAbsentBenchmark.monitorQuery thrpt 10 17,292,751.710 ± 1,975,003.721 ops/s
ComputeIfAbsentBenchmark.monitorQuery thrpt 10 19,575,401.472 ± 682464.230 ops/s
ComputeIfAbsentBenchmark.monitorQuery thrpt 10 20,644,562.716 ± 511267.928 ops/s
New way (doing get)
ComputeIfAbsentBenchmark.monitorQuery thrpt 10 32,063,248.194 ± 831,342.655 ops/s
ComputeIfAbsentBenchmark.monitorQuery thrpt 10 31,672,072.486 ± 1095001.901 ops/s
ComputeIfAbsentBenchmark.monitorQuery thrpt 10 32,283,562.515 ± 1837351.361 ops/s
It looks like jdk 11 has vastly improved computeIfAbsent but the retrieval of computeIfAbsent is slightly slower than the get still. I've removed the check for the jdk version for now based on these results...
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #3196:
GEODE-6404: work around possible sync issue with computeIfAbsent
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Do you have a JMH benchmark, or similar, that shows this is really an improvement? I think it is obviously an improvement if the entry is expected to be there for the vast majority of the calls. This will have a more negative impact though if the use case expects the entry to not be there. In that use case we go through the get path twice.
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #3196:
GEODE-6404: work around possible sync issue with computeIfAbsent
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
If we are reasonably confident that all theses cases are read heavy then it’s all good.
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #3196: GEODE-6404:
work around possible sync issue with computeIfAbsent
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
Any suggestions for the class name?
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #3196: GEODE-6404:
work around possible sync issue with computeIfAbsent
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
I'll still add the JMH tests since it's probably good to test some of these other areas out. I'll follow up on the other calls to try to reason if the change has a negative impact. I just realized my last comment probably felt like I was trying to cut the convo off.. whoops.
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] mcmellawatt commented on pull request #3196:
GEODE-6404: work around possible sync issue with computeIfAbsent
Posted by "mcmellawatt (GitHub)" <gi...@apache.org>.
I'd maybe change the verbage here from "not being concurrent" to "unnecessarily taking out a write lock in the case where the entry exists already and only a read is necessary". Maybe a bit more verbose but gets the point across.
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] jhuynh1 commented on pull request #3196: GEODE-6404:
work around possible sync issue with computeIfAbsent
Posted by "jhuynh1 (GitHub)" <gi...@apache.org>.
This was discovered in an adhoc test where we were unable to scale client queries. The ticket has the stack from the thread dumps which helped identify the issue.
The cost to compute the first time will increase slightly as you stated, but if it's not called very often to get the benefit of a cache, is the performance degrade enough to 'matter'?
I can limit this diff to only affect the two calls that I can see hampering our most common use cases, I just thought making them all the same would make it more consistent across the code base?
A JMH benchmark can be written to hammer these methods directly, but that will just prove that the issues with computeIfAbsent (in Pre Java 9) are blocking/slower when the value is expected to be present.
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org
[GitHub] [geode] pivotal-jbarrett commented on pull request #3196:
GEODE-6404: work around possible sync issue with computeIfAbsent
Posted by "pivotal-jbarrett (GitHub)" <gi...@apache.org>.
Can we classify our use cases of this new wrapper and ensure that they are of the case where the expectation is that there is a value more often than not?
[ Full content available at: https://github.com/apache/geode/pull/3196 ]
This message was relayed via gitbox.apache.org for notifications@geode.apache.org