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

[Robbin Ehn]

Hi Thomas,

On 2020-05-18 15:12, Thomas Stüfe wrote:
> Hi Robbin,
> 
> On Mon, May 18, 2020 at 9:19 AM Robbin Ehn <robbin.ehn at oracle.com 
> <mailto: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.

Put the reference in a functor? You many options here.

So the value's copy constructor and default constructor are assumed to 
be cheap? Or why do we not care about the creating values using them? 
(since the reason of having two methods is to avoid creating values)

Thanks, Robbin

> 
> 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 <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
>      >>
>      >>
>      >>
>