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

[coleen.phillimore at oracle.com]

On 5/18/20 1:27 PM, 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?

I think adding a functor would add more code, but why wouldn't this work:

+ // Look up the key.
+ // If an entry for the key exists, leave map unchanged and return a 
pointer to its value.
+ // If no entry for the key exists, create a new entry from key and 
value and return a
+ // pointer to the value.
+ // *p_created is new if entry was created, false if entry pre-existed.
+ V* put_if_absent(K const& key, V const& value, bool* p_created) {
+ unsigned hv = HASH(key);
+ Node** ptr = lookup_node(hv, key);
+ if (*ptr == NULL) {
+ *ptr = new (ALLOC_TYPE, MEM_TYPE) Node(hv, key, value);
+ *p_created = true;
+ } else {
+ *p_created = false;
+ }
+ return &(*ptr)->_value;
+ }
+ + V* put_if_absent(K const& key, bool* p_created) {
        return put_if_absent(key, V(), p_created);
    }

?

Otherwise the change seems nice and simple from the callers point of 
view.  put_default_if_absent makes me wonder what thing 'default' refers to.

Coleen

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