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

Try to get the value first to avoid creating a lambda #68

Merged
merged 1 commit into from Apr 4, 2018

Conversation

guidomedina
Copy link
Contributor

@guidomedina guidomedina commented Mar 29, 2018

Try to get the value first which is most likely to be cached to avoid creating a lambda.

Similar to:
https://github.com/quickfix-j/quickfixj/blob/master/quickfixj-core/src/main/java/org/quickfixj/SimpleCache.java

I was going to submit this PR yesterday but I was so busy that got distracted and forgot.

@coveralls
Copy link

coveralls commented Mar 29, 2018

Coverage Status

Coverage remained the same at 82.412% when pulling bcdc6d4 on guidomedina:master into 249bf33 on svendiedrichsen:master.

Apply some IntelliJ inspection recommendations.
@svendiedrichsen
Copy link
Owner

@guidomedina I mad some comments in the code.

@guidomedina
Copy link
Contributor Author

@svendiedrichsen I can't see your comments, I don't know what to do, I even tried from an incognito session and your comments are not there.

@svendiedrichsen
Copy link
Owner

@guidomedina If you are logged in you can see them right in this conversation or when you are looking at the 'Files changed' tab.

final String key = valueHandler.getKey();
// Try to first get the value which is most likely cached to avoid creating a lambda.
final VALUE value = cachingMap.get(key);
return value != null ? value : cachingMap.computeIfAbsent(key, k -> valueHandler.createValue());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svendiedrichsen can you see this comment? I know where comments are made in the code, but trust me when I tell you that I cannot see your comments.

@svendiedrichsen
Copy link
Owner

Comments:

  1. What is the reason for calling getDeclaredConstructor Method?
  2. Why check cache first? Isn't this the exact purpose of the computIfAbsent Method?

@guidomedina
Copy link
Contributor Author

guidomedina commented Apr 4, 2018

  • In Java 9+ newInstance() is deprecated, newInstance() will get the declared constructor without parameters and create a new instance anyway (I can undo this part if you want, I'm running IntelliJ with Java 9 and it is shown as deprecated when I analysed the code)
  • computeIfAbsent will lock the bucket corresponding to that hash internally and create a new lambda because the ValueHandler function is not static but dynamic, even though Java is very effective at caching lambdas it is also a mechanism that it is easily exploited, for our case it is most likely that the value was computed before and hence avoid a lambda creation.

If the function were static and not a lambda dynamically created the difference would be very minimal; for caches with high reading pattern vs write calling first get is more effective.

For example, in SimpleCache there is a pre-defined loading function for the whole cache and not one created per key.

@svendiedrichsen svendiedrichsen merged commit c24d663 into svendiedrichsen:master Apr 4, 2018
@svendiedrichsen
Copy link
Owner

Ok, sounds good to me then. Thanks for the work.

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