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

[Thomas Stüfe]

Hi David,

thanks for looking at this. I am answering each of your mails separately,
even though this one is a bit out of date by the new webrev:

On Mon, May 18, 2020 at 4:04 AM David Holmes <david.holmes at oracle.com>
wrote:

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


> 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.)
>
>
yes.


> 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).
>
>
All this is correct. In the end, I found compute_if_absent to be too
cumbersome and too limited in use, since, as you say, passing context is
difficult. Once could pass an opaque pointer or use similar techniques, but
I opted for removing the function. Because most use cases where I want to
delay constructing the new value can be covered by
(new|put_default)_if_absent().


> 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.
>
>
I feel that a variant which delays value construction makes a lot of sense.


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

..Thomas


> Thanks,
> David
>
>