RFR(xs): 8244733: Add ResourceHashtable::compute_if_absent

[David Holmes]

On 12/05/2020 3:15 pm, Thomas Stüfe wrote:
> On Tue, May 12, 2020 at 7:00 AM David Holmes <david.holmes at oracle.com 
> <mailto:david.holmes at oracle.com>> wrote:
> 
>     Hi Thomas,
> 
>     On 12/05/2020 2:25 pm, Thomas Stüfe wrote:
>      > Hi all,
>      >
>      > May I please have reviews for this enhancement:
>      >
>      > JBS: https://bugs.openjdk.java.net/browse/JDK-8244733
>      > Webrev:
>      >
>     http://cr.openjdk.java.net/~stuefe/webrevs/8244733--add-resourcehashtable--compute_if_absent/webrev.00/webrev/
>      >
>      > A common pattern when using ResourceHashTable is:
>      >
>      > if (table.get(k) == NULL) table.add(k,v);
>      >
>      > This runs the lookup code twice. By providing a new method which
>     combines
>      > these two operations we would save one lookup.
>      >
>      > The new method is named ResourceHashTable::compute_if_absent()
>     and behaves
>      > very similar to the similarly named variant in j.u.Map.
> 
>     The code to replace the pattern above would be putIfAbsent. The general
>     motivation for computeIfAbsent is to avoid the execution of an
>     expensive
>     mapping function if the mapping already exists. Perhaps you need both?
> 
> 
> The places I had in mind (ClassLoaderStats, CDS) all needed a 
> computeIfAbsent. I have not seen any places which would benefit from a 
> putIfAbsent, but I can look again.

Okay but the "common pattern" you used to motivate this matches 
putIfAbsent not computeIfAbsent. That pattern would require the 
introduction of a compute function object to use computeIfAbsent - as 
per your 8244777 example code. Stylistically I find the need to 
introduction a function object just to use this API somewhat grating.

> I would like to avoid adding functions proactively though. In my mind 
> putIfAbsent is less useful than computeIfAbsent, only to be preferred if 
> construction needs more information than a mapping function can provide.

I would flip that around and say that computeIfAbsent is only useful if 
concurrent puts are possible and you want to avoid computing the new 
value unnecessarily in each case. I'm not clear from the potential 
call-sites whether concurrent puts are in issue here or not - but if not 
then putIfAbsent is all you need.

David

> Thanks, Thomas
> 
>     David
> 
>      > Note: Actually replacing caller code will happen in a separate
>     RFE. The
>      > first caller to use this will be the ClassLoaderStats VM
>     operation - I was
>      > able to significantly reduce runtime for that vm op. But since
>     that is JFR
>      > territory and this is more of a runtime topic I wanted to keep
>     the review
>      > separate.
>      >
>      > Thanks, Thomas
>      >
>