geode-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Burcham (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (GEODE-6410) review use of putIfAbsent
Date Sat, 23 Feb 2019 00:23:00 GMT

    [ 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)

Mime
View raw message