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

[Thomas Stüfe]

Hi Coleen,

thanks for looking at this!

On Thu, May 14, 2020 at 9:48 PM <coleen.phillimore at oracle.com> wrote:

>
> I just spent time looking for a newIfAbsent in java.util.Hash, but there
> isn't one.


Yes, its a C++ specific; since j.u.Map does only contain references for
Values such a function is not needed.


> These three functions are eye-strainingly similar.
>
>
Yes sorry. Maybe we can do something about that, see below.


> Can you remove new_if_absent and have another compute_if_absent(key,
> created) overload that calls
>
> + V* compute_if_absent(K const& key, mapping_fun_t mapping_function,
> bool* p_created = NULL) {
>
>
> with a trivial mapping function, like your test.


I'm not sure that would be better:

new_if_absent(K) is clear in its naming.
A "compute_if_absent(K)" would be misnamed since it does not compute
anything, and certainly does not behave as j.u.Map.computeIfAbsent().

I still could leave new_if_absent() and implement it atop
of compute_if_absent() using a custom mapping function which just returns
V(). This was my original prototype. But that means we first create V(),
then copy it twice : once as a return from the compute function, then again
when passing the value to the Node() constructor. I also think
new_if_absent() in its current form is clearer.

I agree about the eye watering complaint though. Proposals:

1) I could just remove compute_if_absent(). Arguably, it is the most
cumbersome to use and its one use case in ClassLoaderStats could be done
with new_if_absent() just as well. That brings us down to just
put_if_absent() and new_if_absent().
2) Hide the common prologue and epilogue behind macros


>   And remove the default
> parameters since I think none of the callers should have a default
> parameter.
>

Hm, okay. That would make the coding a bit simpler.

I'll wait with a new webrev for your answer and feedback from the others.

Cheers, Thomas


>
> thanks,
> Coleen
>
> On 5/14/20 4:08 AM, Thomas Stüfe 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
>
>