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

[Thomas Stüfe]

Great, thank you all!

On Tue, May 19, 2020 at 4:39 PM <coleen.phillimore at oracle.com> wrote:

>
> 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> 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.
>
> You're right.  It's not a lot of duplication, it's fine.
> Thanks,
> Coleen
>
>
>
>> Thanks, Robbin
>>
>
> Thanks, Thomas
>
>>
>>
>