RFR(s): 8244733: Add ResourceHashtable::xxx_if_absent
David Holmes
david.holmes at oracle.com
Wed May 20 06:56:37 UTC 2020
Hi Thomas,
Did we still land with this version:
http://cr.openjdk.java.net/~stuefe/webrevs/8244733--add-resourcehashtable--compute_if_absent/webrev.02/webrev/
? Sorry finding it hard to track. If that is still the version then okay
from me.
Thanks,
David
-----
On 20/05/2020 12:41 am, Thomas Stüfe wrote:
> Great, thank you all!
>
> On Tue, May 19, 2020 at 4:39 PM <coleen.phillimore at oracle.com
> <mailto: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
>> <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
>>
>>
>
More information about the hotspot-runtime-dev
mailing list