[jira] [Commented] (GEODE-6410) review use of putIfAbsent

["Bill Burcham (JIRA)" <ji...@apache.org>]

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