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

[Thomas Stüfe]

On Mon, May 18, 2020 at 9:56 PM Robbin Ehn <robbin.ehn at oracle.com> wrote:

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

Ah now I get it. Thanks for explaining.

You are right, put_(default)_if_absent(k) makes most sense for complex data
types with a cheapish default ctor and then later slow trickling in of
information. Typical use would be stat counters or similar. Like the use
cases in CDS:
http://cr.openjdk.java.net/~stuefe/webrevs/8244733--add-resourcehashtable--compute_if_absent/webrev.02/webrev/src/hotspot/share/classfile/classLoaderStats.cpp.udiff.html.
This variant forces you to have a default constructor; preventing you from
using const members and similar annoyances.

put_if_absent(k, v) makes sense for cases where copy construction is cheap,
as is obtaining the information in the first place. Like here:
http://cr.openjdk.java.net/~stuefe/webrevs/8244733--add-resourcehashtable--compute_if_absent/webrev.02/webrev/src/hotspot/share/classfile/bytecodeAssembler.cpp.udiff.html
 .

None of these are perfect, but they mimic the existing code (when in
Rome..) and I'd rather not extend the scope of this patch. It is difficult
enough to agree on one form.

That said, lets discuss your thought :)

IIUC, what you propose is a variant which, given a key, would allocate
backing memory for the value if it does not exist, but leave it
uninitialized it? To be then used later with placement new?

I'm not even sure how to do this in C++. Like this maybe:

class Node {
  ...
  char[sizeof(V)] value_memory;
}; ?

And the use case would be complex objects with non-trivial non-cheap
construction. But I do not see a pressing need for it. All use cases I
fixed up were either PODs or very simple value holder structs like in CDS.

Before doing any of this stuff I'd rather enhance ResourceHashTable in
other areas. For instance, that thing never resizes the bucket array. The
ClassLoaderStats hash table I worked recently on was 250 times overbooked
with near perfect hash! OTOH the default bucket array size is 256, which is
2K for 64 platforms, which is kind of a lot, depending on what you do with
it. So, a resizable hash table would be good - either automatic or manually
triggered.

Another though I have is that rather than avoiding the copy constructor, on
quite a few call sites it would make sense to embrace it and go fully
pass-by-value:
- bool put(K const& key, V const& value) {
+ bool put(K key, V value) {

since in many cases key or values are PODs. Sometimes key and value are
even smaller than a pointer, so passing by reference unnecessarily uses
64bit beside the address-of and dereferencing.

Just some thoughts. One wish expressed in these reviews was that
ResourceHashTable should stay simple, so this means none of these ideas may
be doable.

Thanks, Thomas


>