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

[Martin Buchholz]

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