[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (S) 8247615: Initialize the bytes left for the heap sampler
From:       Martin Buchholz <martinrb () google ! com>
Date:       2020-06-30 7:47:43
Message-ID: CA+kOe09mBFCmMMN-PTU4J68c0PpXyFHjOo9bFSqTFaMB0jwaag () mail ! gmail ! com
[Download RAW message or body]

Looks good to me, but of course I'm not qualified to review.

On Mon, Jun 29, 2020 at 3:26 PM Jean Christophe Beyler
<jcbeyler@google.com> wrote:
> 
> Agreed; missed a hg qrefresh; link is now updated:
> http://cr.openjdk.java.net/~jcbeyler/8247615/webrev.01/
> 
> > )
> Jc
> 
> On Mon, Jun 29, 2020 at 2:28 PM Man Cao <manc@google.com> wrote:
> > 
> > Looks good.
> > 
> > > though adding the change that Man wants might make it more flaky so I added \
> > > your numThreads / 2 in case
> > I don't see the "numThreads / 2" in webrev.01 though. No need for a webrev for \
> > this fix. 
> > -Man
> > 
> > 
> > On Mon, Jun 29, 2020 at 1:10 PM Jean Christophe Beyler <jcbeyler@google.com> \
> > wrote:
> > > 
> > > Hi all,
> > > 
> > > Sorry it took time to get back to this; could I get a new review from:
> > > http://cr.openjdk.java.net/~jcbeyler/8247615/webrev.01/
> > > 
> > > The bug is here:
> > > https://bugs.openjdk.java.net/browse/JDK-8247615
> > > 
> > > Note, this passed the submit repo testing.
> > > 
> > > Thanks and have a great day!
> > > Jc
> > > 
> > > Ps: explicit inlined Acks/Done are below:
> > > 
> > > Sorry it took time to get back to this:
> > > @Martin:
> > > - done the typo
> > > - about the sampling test: No you won't get samples due to how the system is \
> > > done, since we know we only will be allocating one object for the thread, it \
> > > dies out before a sample is required... though adding the change that Man wants \
> > >                 might make it more flaky so I added your numThreads / 2 in case
> > > - done for the always in the description
> > > 
> > > 
> > > On Thu, Jun 25, 2020 at 6:54 PM Derek Thomson <dthomson@google.com> wrote:
> > > > 
> > > > > It could also avoid the problem where every thread deterministically \
> > > > > allocates the same object at 512K, although this is unlikely.
> > > > 
> > > > I've recently discovered that with certain server frameworks that this \
> > > > actually becomes quite likely! So I'd strongly recommend using \
> > > > pick_next_sample.
> > > 
> > > 
> > > Ack, done :)
> > > 
> > > > 
> > > > 
> > > > On Thu, Jun 25, 2020 at 4:56 PM Man Cao <manc@google.com> wrote:
> > > > > 
> > > > > Thanks for fixing this!
> > > > > 
> > > > > > 53 ThreadHeapSampler() : _bytes_until_sample(get_sampling_interval())  {
> > > > > 
> > > > > Does this work better? (It has to be done after the initialization of \
> > > > > _rnd.) _bytes_until_sample = pick_next_sample();
> > > > > 
> > > > > It could avoid completely missing to sample the first 512K allocation.
> > > > > It could also avoid the problem where every thread
> > > 
> > > 
> > > Done.
> > > 
> > > 
> > > > > 
> > > > > deterministically allocates the same object at 512K, although this is \
> > > > > unlikely. 
> > > > > -Man
> > > 
> > > 
> > > 
> > > --
> > > 
> > > Thanks,
> > > Jc
> 
> 
> 
> --
> 
> Thanks,
> Jc


[prev in list] [next in list] [prev in thread] [next in thread]