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

[Thomas Stüfe]

Hi Robbin,

On Mon, May 18, 2020 at 9:19 AM Robbin Ehn <robbin.ehn at oracle.com> wrote:

> Hi Thomas,
>
> You have two copies of put_if_absent, if you e.g. add a private one
> instead:
>
>    V* priv_put_if_absent(K const& key, const V * value, bool* p_created)
> ...
>        *ptr = value != NULL ? new (ALLOC_TYPE, MEM_TYPE) Node(hv, key,
> *value) : new (ALLOC_TYPE, MEM_TYPE) Node(hv, key);
>
> You only need one.
>

The small gripe I have about this is that I would have to translate from
reference to pointer and back to reference -  somehow this seems less
clearer to me than that bit of duplication that is left now.

Thanks, Thomas


>
> /Robbin
>
> On 2020-05-16 10:33, 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 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>
> > 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
> >>
> >>
> >>
>