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 <marti...@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 > <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 > -- Thanks, Jc