RFR(s): 8244733: Add ResourceHashtable::xxx_if_absent
Thomas Stüfe
thomas.stuefe at gmail.com
Wed May 20 13:36:20 UTC 2020
Hi David,
yes, that was the last version. Thank you!
..Thomas
On Wed, May 20, 2020 at 8:56 AM David Holmes <david.holmes at oracle.com>
wrote:
> 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