RFR(xs): 8244777: ClassLoaderStats VM Op uses constant hash value

[Thomas Stüfe]

Hi Jaroslav,

thanks for the review!

For the basic functionality I think there exists at least one, possibly two
tests (one tests JFR, one tests jcmd VM.classloader_stats). So I think the
basics are covered. A performance test would be more difficult since to get
any noticeable slowdown you need a very high number of loaders, which in
turn costs a lot of memory (my test with 50K loaders runs up tp almost 5G
of metaspace).

Thanks, Thomas


On Tue, May 12, 2020 at 6:41 PM Jaroslav Bachorík <
jaroslav.bachorik at datadoghq.com> wrote:

> Hi Thomas,
>
> The change looks good to me.
> Would it be possible to have an accompanying test? Not a blocking
> request, though.
>
> Thanks!
>
> -JB-
>
>
> On Tue, May 12, 2020 at 3:35 PM Daniel D. Daugherty
> <daniel.daugherty at oracle.com> wrote:
> >
> > Adding hotspot-jfr-dev at ... since this bug is filed against
> hotspot/jfr...
> >
> > Dan
> >
> >
> > On 5/12/20 2:33 AM, Thomas Stüfe wrote:
> > > Hi,
> > >
> > > may I get reviews for this small fix:
> > >
> > > bug: https://bugs.openjdk.java.net/browse/JDK-8244777
> > > webrev:
> > >
> http://cr.openjdk.java.net/~stuefe/webrevs/8244777--classloaderstats-vm-op-uses-constant-hash-value/webrev.00/webrev/
> > >
> > > This fixes runtime issues with the ClassLoaderStats VM Operation -
> with 50K
> > > classloaders it currently takes 1.5s, with this patch it takes ~13 ms.
> This
> > > issue was reported at [1].
> > >
> > > ClassLoaderStats iterates the CLDG and accumulates statistics per class
> > > loader over all CLDs. It uses a temporary ResourceHashTable for that.
> > >
> > > The hash function is wrong though. It has two errors which
> independently
> > > from each other cause the hash value to be constant, so the
> > > ResourceHashTable degenerates to a linked list.
> > >
> > > static unsigned oop_hash(oop const& s1) {
> > >     unsigned hash = (unsigned)((uintptr_t)&s1);
> > >     return hash ^ (hash >> LogMinObjAlignment);
> > > }
> > >
> > > - we use &s1, so we calculate the hash based on the address of the key
> > > carrier not the key, which is always the same
> > > - we use LogMinObjAlignment instead of LogMinObjAlignmentInBytes,
> which is
> > > usually 0, so we xor out the hash at the end.
> > >
> > > After playing with the corrected version I got better results; then I
> > > swapped this implementation with a common integer hash function we
> also use
> > > in zgc, and got even better results. For a hash table with 256 buckets
> and
> > > 50000 entries I now get a max chain len of 230, which is pretty close
> to
> > > the optimal ~200.
> > >
> > > Thanks, Thomas
> > >
> > > [1]
> > >
> https://mail.openjdk.java.net/pipermail/hotspot-jfr-dev/2020-May/001430.html
> >
>