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

[Robbin Ehn]

Hi Thomas,

On 2020-05-19 07:32, Thomas Stüfe wrote:
> 
> 
> On Mon, May 18, 2020 at 9:56 PM Robbin Ehn <robbin.ehn at oracle.com 
> <mailto: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.

Ok

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

Yes and if you have a complex type you often don't want it to have same 
life-cycle as the Node. So V would be a pointer to the type, thus 
copy-by value is also fine.

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

Ok, sure!

Patch looks fine! But I would consider Coleen's suggestion:

+  V* put_if_absent(K const& key, bool* p_created) {
        return put_if_absent(key, V(), p_created);
    }

Thanks, Robbin

> 
> Thanks, Thomas
> 
>