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

Deprecating Memoizer as there is a better replacement in Java 8 #3091

Merged
merged 2 commits into from Oct 22, 2017

Conversation

5 participants
@jglick
Member

jglick commented Oct 16, 2017

Proposed changelog entries

None needed I think. Theoretically the replacement could avoid acquiring a lock in some cases, but practically speaking this would only matter during startup.

Desired reviewers

@reviewbybees

@reviewbybees

This comment has been minimized.

reviewbybees commented Oct 16, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@johnou

This comment has been minimized.

Member

johnou commented Oct 16, 2017

Depending on how often these methods are called you may consider first invoking get and then compute if absent for higher throughput.

@jglick

This comment has been minimized.

Member

jglick commented Oct 17, 2017

Well that is what Memoizer was doing. I trust that Doug Lea et al. have considered this concept and written an implementation that already performs as well as that would.

@jglick

This comment has been minimized.

Member

jglick commented Oct 17, 2017

Test failures seem to have been due to broken update center metadata at the time (CC @daniel-beck). Will trigger a fresh build to be sure.

@johnou

This comment has been minimized.

Member

johnou commented Oct 17, 2017

@jglick see orbit/orbit#253 (comment) for detailed information on performance impact of using computeIfAbsent vs get then computeIfAbsent.

@jglick

This comment has been minimized.

Member

jglick commented Oct 17, 2017

@johnou so is there some JDK-nnnnnnn tracker issue discussing the performance characteristics? If there is an acknowledged problem and a plan to fix it in Java 8 then we should just use the API as it was designed. If not, then this should just be closed and we will carry on with our weird custom utilities.

@johnou

This comment has been minimized.

Member

johnou commented Oct 18, 2017

@jglick no just what Ben mentioned in the other issue, that said I think this change does simplify the code a lot and if the methods are not invoked a great deal there might not even be a notable performance hit. I simply wanted to give a heads up, just in case.

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Oct 19, 2017

Right, while useful context to have if we ever need to investigate a performance problem related to this change, it's not worth worrying about the "what if" here. I think we should just go ahead with what we have here.

@johnou

johnou approved these changes Oct 19, 2017

@oleg-nenashev

🐝 though I agree that having a micro-benchmark results would be reasonable here

@oleg-nenashev oleg-nenashev merged commit a5388c9 into jenkinsci:master Oct 22, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@jglick

Sorry, just saw the most recent comments here.

Not sure about the others, but Jenkins.getExtensionList is called from performance-sensitive sites, so we could consider adding an optimistic get to that one at least, if it is thought to be useful under heavy load conditions.

@jglick jglick deleted the jglick:Memoizer branch Oct 23, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Dec 15, 2017

@jglick There is an issue report, which points to this PR: https://issues.jenkins-ci.org/browse/JENKINS-48505

@johnou

This comment has been minimized.

Member

johnou commented Dec 15, 2017

As I suggested an optimistic get should resolve that issue report, I will create a PR.

@johnou

This comment has been minimized.

Member

johnou commented Dec 15, 2017

@johnou

This comment has been minimized.

Member

johnou commented Dec 15, 2017

for reference, here is the bug report (only fixed in JDK9) JDK-8184907 : ConcurrentHashMap.computeIfAbsent acquires lock even when key present http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8184907

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment