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 <jcbey...@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 <m...@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 <jcbey...@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 <dthom...@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 <m...@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
