[
https://issues.apache.org/jira/browse/GEODE-6410?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16775712#comment-16775712
]
Bill Burcham commented on GEODE-6410:
-------------------------------------
tl;dr we aren't fixing the lock contention in {{putIfAbsent()}} unless/until we
have clear evidence that doing so, improves performance of a high-importance
use case
{{putIfAbsent()}} (from {{Map}}) is used about 100 places in Geode, outside of
tests.
I looked at the first half of these usages with [~huynhja]. We focused on the
18/48 usages that occurred in classes that we felt had a high probability of
encountering high-concurrency. Of those:
* 8/18 were already optimized: {{get()}} was called, directly or indirectly,
before {{putIfAbsent()}}
* 3/18 were calling in to those 8/20 and so, required no additional
optimization effort
* 2/18 were unoptimized but were in non-hot code paths, and so, would be a
very low priority for optimization
* 5/18 were unoptimized and were in what appeared to potentially be fairly hot
code paths
See this spreadsheet for details:
[https://docs.google.com/spreadsheets/d/1S_3qw8oW7dwgBG9YCs1oy2pjvQEZIyzo9DyKlh_SLS8/edit#gid=0]
Assuming that our sample was random, we might expect to find another 5 usages
requiring optimization. So that might be 10 code locales in all.
We considered three possible approaches for a fix:
# create a new static method and call it from each of the 10 locations
(similar to the approach developed in the in early work on GEODE-6404 to fix
the same performance issue in {{computeIfAbsent()}})
# create a new subclass of {{ConcurrentHashMap}} with both methods improved,
and instantiate that new class instead of the original, everywhere in Geode:
## the subclass could have a naive implementation of the fix that just does a
{{get()}} before conditionally doing the {{super.putIfAbsent()}}
## alternately, the subclass could try to deliver the same (more
sophisticated) fix that is present in {{computeIfAbsent()}} in JDK11
(1) is targeted (good) but will increase code complexity because places where
it is called will look unorthodox, and new code will likely fail to use the
improved logic
(2.1) solves the "new code" problem but might result in redundant {{get()}}
calls in existing Geode code, decreasing performance by a factor of 2 in those
paths
(2.2) would perform like the JDK11 implementation, and would extend that to
both methods; it might perform better than (2.1) but who knows really; also
this approach might entail copy-pasting JDK11 source code since important parts
of the base class logic are in inner classes
With an idea of the scope and some different solution approaches, we spent some
time considering how we'd validate such a change. We might have saved some
effort had we started here.
While it's intuitive that the fix to {{computeIfAbsent()}} in JDK11 should also
be applied to {{putIfAbsent()}}—that intuition requires validation. Without a
clear and present scenario for where {{putIfAbsent()}} is hurting us, we are
left with the prospect of writing new performance tests to validate the
changes. But that opens a whole can of worms.
So we'll hold off on making changes around {{putIfAbsent()}} until we have
clear evidence that doing so, improves performance of a high-importance use case
> review use of putIfAbsent
> -------------------------
>
> Key: GEODE-6410
> URL: https://issues.apache.org/jira/browse/GEODE-6410
> Project: Geode
> Issue Type: Bug
> Components: general
> Reporter: Jason Huynh
> Assignee: Bill Burcham
> Priority: Major
>
> Usages of putIfAbsent in Geode on ConcurrentHashMap may not have realized the
> actual synchronized/atomic nature of the putIfAbsent. See
> [https://bugs.openjdk.java.net/browse/JDK-6737839]
> This ticket is for someone to review or possibly change the putIfAbsent
> usages to be more performant.
> Not sure if putIfAbsent is called enough and under contention enough for this
> to matter...
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
