Closed guidomedina closed 6 years ago
@guidomedina I mad some comments in the code.
@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.
@guidomedina If you are logged in you can see them right in this conversation or when you are looking at the 'Files changed' tab.
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?
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.
Ok, sounds good to me then. Thanks for the work.
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.