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

[Thomas Stüfe]

Hi Robbin,

On Tue, May 19, 2020 at 8:56 AM Robbin Ehn <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>> 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.


> Thanks, Robbin
>

Thanks, Thomas

>
>