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

[David Holmes]

Hi Thomas,

A bit of a long winded reply as I walk through the motivation here ...

On 14/05/2020 6:08 pm, 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.

The advantage of only doing the lookup once is achieved with a simple 
put_if_absent method.

The downside of put_if_absent is that you have to precompute the new 
value, even if you don't need it. Hence the desire for compute_if_absent.

But in C++ compute_if_absent needs to be able to execute a mapping 
function which can be expressed based only on global state and the key 
that was passed in. (Whereas in Java we can create an inner class that 
implements the functional interface, and it has implicit access to the 
enclosing instance to use any state needed in the calculation.)

Hence you introduced new_if_absent() as well - though I'd be tempted to 
call it put_default_if_absent(). The new_if_absent function knows how to 
create a default instance of the value and returns a pointer to that 
instance - and the calling code can then compute and set the real value.

So walking through BytecodeConstantPool::find_or_add as an example:

   u2 BytecodeConstantPool::find_or_add(BytecodeCPEntry const& bcpe) {
     u2 index;
     u2* probe = _indices.get(bcpe);
     if (probe == NULL) {
       index = _entries.length();
       _entries.append(bcpe);
       _indices.put(bcpe, index);

We can use put_if_absent quite easily here:

   u2 BytecodeConstantPool::find_or_add(BytecodeCPEntry const& bcpe) {
     bool created = false;
     u2 index = _entries.length();
     u2* probe = _indices.put_if_absent(bcpe, index, &created);
     if (created) {
       _entries.append(bcpe);
     }

But lets assume _entries.length() requires a traversal to calculate 
(rather than a simple lookup) in which case you want to avoid 
calculating it unnecessarily. So we want compute_if_absent() except we 
don't have a way to factor computing this->_entries.length() into a 
function object. So we use new_if_absent():

  u2 BytecodeConstantPool::find_or_add(BytecodeCPEntry const& bcpe) {
    u2 index;
    bool created = false;
    u2* probe = _indices.new_if_absent(bcpe, &created);  // set default 
value
    if (created) {
      index = _entries.length();
      _entries.append(bcpe);
      *probe = index; // set real value

So if you accept the premise that it is undesirable to pre-determine the 
value if you don't end up adding the map entry, then the need for 
compute_if_absent and thus new_if_absent becomes clear. If you don't 
accept that premise then put_if_absent is all you need**.

** And put_if_absent can also partly avoid the computation overhead if 
it can split the creation of a default value and the calculation and 
setting of the true value (not applicable if the value is a primitive 
type of course).

I'm not clear in all the examples in the webrev whether avoiding the 
computation makes any practical difference. If _entries.length() just 
loads a field then I would argue that new_if_absent is overkill for that 
case. OTOH avoiding construction of a 
ClassLoaderStats/DumpTimeSharedClassInfo instance may be more 
significant as we have to free it again if not used.

So overall I understand the motivation for the more elaborate API and 
the way you have applied it so far.

Thanks,
David

> Ran tests tonight in our CI, all went through.
> 
> Thanks, Thomas
> 
>