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

[Thomas Stüfe]

On Mon, May 18, 2020 at 5:53 PM Robbin Ehn <robbin.ehn at oracle.com> wrote:

> 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)
>
>
Sorry, can you explain what you mean, or which part of my patch you refer
to?

Thanks, Thomas


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