RFR(s): 8244733: Add ResourceHashtable::xxx_if_absent (was: ResourceHashtable::compute_if_absent)

[David Holmes]

Hi Thomas,

FYI my previous email was written Friday/Saturday but got stuck until 
this morning. :(

On 16/05/2020 6:33 pm, Thomas Stüfe wrote:
> Hi all.
> 
> new version: 
> http://cr.openjdk.java.net/~stuefe/webrevs/8244733--add-resourcehashtable--compute_if_absent/webrev.02/webrev/
> 
> with Coleens feedback worked in:
> 
> I removed the compute_if_absent() variant since of the new three methods 
> it seems the least useful one; this leaves us with a variant which 
> default-creates the value if not found, and one with puts a caller 
> provided value if not found. I also removed the default argument for the 
> p_created output.
> 
> As per Coleen's request, I named the methods both "put_if_absent". See 
> comments at the method; I hope the intent is still clear.

I'd personally prefer put_default_if_absent() rather than relying on 
overloading. But not a blocker.

I'm unclear why the ClassLoaderStats APIs were changed from using 
pointers to objects/references ?

src/hotspot/share/utilities/resourceHash.hpp

+   // *p_created is new if entry was created, false if entry pre-existed.

s/new/true/  - two occurrences.

Thanks,
David
-----

> I also fixed up the callers and the test.
> 
> Thanks, Thomas
> 
> 
> 
> On Thu, May 14, 2020 at 10:08 AM Thomas Stüfe <thomas.stuefe at gmail.com 
> <mailto:thomas.stuefe at gmail.com>> wrote:
> 
>     Hi,
> 
>     This is a continuation of the review thread:
>     https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-May/039444.html
> 
>     I changed the title of the bug somewhat hence the new thread.
> 
>     JBS: https://bugs.openjdk.java.net/browse/JDK-8244733
>     Webrev:
>     http://cr.openjdk.java.net/~stuefe/webrevs/8244733--add-resourcehashtable--compute_if_absent/webrev.01/webrev/
> 
>     This now adds three new functions to ResourceHashTable:
> 
>     new_if_absent(), compute_if_absent(), put_if_absent()
> 
>     which work very similar to their equivalents in j.u.Map.
> 
>     The advantage they offer is that for a typical "if map does not
>     contain X, add X+value" pattern we only do the lookup once.
> 
>     --
> 
>     So I extended the scope of the item. After playing around with
>     various call sites I realized that David's proposal of
>     put_if_absent() would be useful, as well as a variant which just
>     adds a default constructed value. All three are tested in gtest and
>     used in real call sites.
> 
>     Ran tests tonight in our CI, all went through.
> 
>     Thanks, Thomas
> 
>