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

[coleen.phillimore at oracle.com]

Hi Thomas,

On 5/15/20 3:34 AM, Thomas Stüfe wrote:
> Hi Coleen,
>
> thanks for looking at this!
>
> On Thu, May 14, 2020 at 9:48 PM <coleen.phillimore at oracle.com 
> <mailto:coleen.phillimore at oracle.com>> wrote:
>
>
>     I just spent time looking for a newIfAbsent in java.util.Hash, but
>     there
>     isn't one. 
>
>
> Yes, its a C++ specific; since j.u.Map does only contain references 
> for Values such a function is not needed.

But it doesn't call 'new' on the value, only allocates the hashtable 
entry itself, but so does compute_if_absent and also put_if_absent.
>
>     These three functions are eye-strainingly similar.
>
>
> Yes sorry. Maybe we can do something about that, see below.
>
>     Can you remove new_if_absent and have another compute_if_absent(key,
>     created) overload that calls
>
>     + V* compute_if_absent(K const& key, mapping_fun_t mapping_function,
>     bool* p_created = NULL) {
>
>
>     with a trivial mapping function, like your test.
>
>
> I'm not sure that would be better:
>
> new_if_absent(K) is clear in its naming.
> A "compute_if_absent(K)" would be misnamed since it does not compute 
> anything, and certainly does not behave as j.u.Map.computeIfAbsent().
>

I don't really like the compute_if_absent name either! I don't know what 
it's computing.  It's getting the value and putting it in the table if 
it's not present, which incidentally, also is what put_if_absent is doing!

I think all of these functions should be called put_if_absent().

This hashtable was supposed to have a super-simple API with minimal 
boilerplate and this makes it hard to see which one of these functions I 
want to call.   How about something like this:

http://cr.openjdk.java.net/~coleenp/2020/01/webrev

Or they could all be called 'insert' like the concurrentHashTable.hpp.

thanks,
Coleen

> I still could leave new_if_absent() and implement it atop 
> of compute_if_absent() using a custom mapping function which just 
> returns V(). This was my original prototype. But that means we first 
> create V(), then copy it twice : once as a return from the compute 
> function, then again when passing the value to the Node() constructor. 
> I also think new_if_absent() in its current form is clearer.
>
> I agree about the eye watering complaint though. Proposals:
>
> 1) I could just remove compute_if_absent(). Arguably, it is the most 
> cumbersome to use and its one use case in ClassLoaderStats could be 
> done with new_if_absent() just as well. That brings us down to just 
> put_if_absent() and new_if_absent().
> 2) Hide the common prologue and epilogue behind macros
>
>       And remove the default
>     parameters since I think none of the callers should have a default
>     parameter.
>
>
> Hm, okay. That would make the coding a bit simpler.
>
> I'll wait with a new webrev for your answer and feedback from the others.
>
> Cheers, Thomas
>
>
>     thanks,
>     Coleen
>
>     On 5/14/20 4:08 AM, Thomas Stüfe 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
>