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

[Thomas Stüfe]

Thank you David.

I'll go and fix callsites and then we will see if additional functions are
needed.

Cheers, Thomas

On Tue, May 12, 2020 at 8:38 AM David Holmes <david.holmes at oracle.com>
wrote:

> Sorry ignore this.
>
> There are different tradeoffs with different patterns in relation to
> computing the value conditionally or unconditionally.
>
> Consider this reviewed.
>
> Thanks,
> David
> -----
>
> On 12/05/2020 4:32 pm, David Holmes wrote:
> > 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
> >>      >
> >>
>