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

[Jean Christophe Beyler]

Ping on this to get a final review :)

Webrev:
http://cr.openjdk.java.net/~jcbeyler/8247615/webrev.01/

The bug is here:
https://bugs.openjdk.java.net/browse/JDK-8247615

Thanks!
Jc

On Thu, Jul 9, 2020 at 12:03 AM Jean Christophe Beyler <jcbeyler at google.com>
wrote:

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


-- 

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