RFR (S) 8247615: Initialize the bytes left for the heap sampler

[Jean Christophe Beyler]

Hello all,

Any way to get two reviews of the new version:
http://cr.openjdk.java.net/~jcbeyler/8247615/webrev.01/

Thanks and have a great evening!
Jc

On Tue, Jun 30, 2020 at 12:47 AM Martin Buchholz <martinrb at google.com>
wrote:

> 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 at 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 at 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 at 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 at 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 at 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
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200708/f4b9b0c9/attachment.htm>