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

[coleen.phillimore at oracle.com]

Hi, I think we've discussed this patch enough and it's fine as is. 
Actually, it's a nice improvement.  Ship it!

On 5/19/20 4:16 AM, Thomas Stüfe wrote:
> Hi Robbin,
>
> On Tue, May 19, 2020 at 8:56 AM Robbin Ehn <robbin.ehn at oracle.com 
> <mailto:robbin.ehn at oracle.com>> wrote:
>
>     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>
>     > <mailto: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!
>
>
> Great, thanks!
>
>     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);
>         }
>
>
> That would not work since it would heave the default ctor out of the 
> "if not found" condition and makes us pay upfront. She suggested to 
> implement put_if_absent(K const& key, bool* p_created) using the 
> former "compute_if_absent" but I removed that one since it seemed not 
> worth the complexity.
You're right.  It's not a lot of duplication, it's fine.
Thanks,
Coleen
>
>     Thanks, Robbin
>
>
> Thanks, Thomas
>
>