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

[Robbin Ehn]

Hi Thomas,

The Node constructor uses V copy constructor here:
     Node(unsigned hash, K const& key, V const& value) :
         _hash(hash), _key(key), _value(value), _next(NULL) {}

So we create value to pass in and and then create a new _value, so two 
objects of type V are created.

// Create a node with a default-constructed value.
Node(unsigned hash, K const& key) :
     _hash(hash), _key(key), _value(), _next(NULL) {}

Here we call the V default-constructor.
If we manually later populate V we only need one object.
But most often we already have written that code in the constructor.
I would use placement new on that piece of memory when running V normal
constructor instead. This would also remove the need for V having a
default constructor.

So my question was, if we are trying to avoid creating unnecessary
objects of type V, do really want to use copy constructor here?
(not your patch's doing)

And manually populating V or writing a special method for this
case to populate V feels odd?

(note these are just questions, I guess most objects are trivial that 
uses this anyways)

Thanks, Robbin

On 2020-05-18 19:27, Thomas Stüfe wrote:
> 
> 
> On Mon, May 18, 2020 at 5:53 PM Robbin Ehn <robbin.ehn at oracle.com 
> <mailto: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>
>      > <mailto: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>
>     <mailto: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
>      >      >>
>      >      >>
>      >      >>
>      >
>