Low-Overhead Heap Profiling
JC Beyler
jcbeyler at google.com
Mon Oct 9 22:57:45 UTC 2017
Dear all,
Thread-safety is back!! Here is the update webrev:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.10_11/
Full webrev is here:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.11/
In order to really test this, I needed to add this so thought now was a
good time. It required a few changes here for the creation to ensure
correctness and safety. Now we keep the static pointer but clear the data
internally so on re-initialize, it will be a bit more costly than before. I
don't think this is a huge use-case so I did not think it was a problem. I
used the internal MutexLocker, I think I used it well, let me know.
I also added three tests:
1) Stack depth test:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.10_11/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStackDepthTest.java.patch
This test shows that the maximum stack depth system is working.
2) Thread safety:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.10_11/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorThreadTest.java.patch
The test creates 24 threads and they all allocate at the same time. The
test then checks it does find samples from all the threads.
3) Thread on/off safety
http://cr.openjdk.java.net/~rasbold/8171119/webrev.10_11/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorThreadOnOffTest.java.patch
The test creates 24 threads that all allocate a bunch of memory. Then
another thread turns the sampling on/off.
Btw, both tests 2 & 3 failed without the locks.
As I worked on this, I saw a lot of places where the tests are doing very
similar things, I'm going to clean up the code a bit and make a
HeapAllocator class that all tests can call directly. This will greatly
simplify the code.
Thanks for any comments/criticisms!
Jc
On Mon, Oct 2, 2017 at 8:52 PM, JC Beyler <jcbeyler at google.com> wrote:
> Dear all,
>
> Small update to the webrev:
> http://cr.openjdk.java.net/~rasbold/8171119/webrev.09_10/
>
> Full webrev is here:
> http://cr.openjdk.java.net/~rasbold/8171119/webrev.10/
>
> I updated a bit of the naming, removed a TODO comment, and I added a test
> for testing the sampling rate. I also updated the maximum stack depth to
> 1024, there is no reason to keep it so small. I did a micro benchmark that
> tests the overhead and it seems relatively the same.
>
> I compared allocations from a stack depth of 10 and allocations from a
> stack depth of 1024 (allocations are from the same helper method in
> http://cr.openjdk.java.net/~rasbold/8171119/webrev.10/
> raw_files/new/test/hotspot/jtreg/serviceability/jvmti/
> HeapMonitor/MyPackage/HeapMonitorStatRateTest.java):
> - For an array of 1 integer allocated in a loop; stack depth
> 1024 vs stack depth 10: 1% slower
> - For an array of 200k integers allocated in a loop; stack depth
> 1024 vs stack depth 10: 3% slower
>
> So basically now moving the maximum stack depth to 1024 but we only copy
> over the stack depths actually used.
>
> For the next webrev, I will be adding a stack depth test to show that it
> works and probably put back the mutex locking so that we can see how
> difficult it is to keep thread safe.
>
> Let me know what you think!
> Jc
>
>
>
> On Mon, Sep 25, 2017 at 3:02 PM, JC Beyler <jcbeyler at google.com> wrote:
>
>> Forgot to say that for my numbers:
>> - Not in the test are the actual numbers I got for the various array
>> sizes, I ran the program 30 times and parsed the output; here are the
>> averages and standard deviation:
>> 1000: 1.28% average; 1.13% standard deviation
>> 10000: 1.59% average; 1.25% standard deviation
>> 100000: 1.26% average; 1.26% standard deviation
>>
>> The 1000/10000/100000 are the sizes of the arrays being allocated. These
>> are allocated 100k times and the sampling rate is 111 times the size of the
>> array.
>>
>> Thanks!
>> Jc
>>
>>
>> On Mon, Sep 25, 2017 at 3:01 PM, JC Beyler <jcbeyler at google.com> wrote:
>>
>>> Hi all,
>>>
>>> After a bit of a break, I am back working on this :). As before, here
>>> are two webrevs:
>>>
>>> - Full change set: http://cr.openjdk.java.ne
>>> t/~rasbold/8171119/webrev.09/
>>> - Compared to version 8: http://cr.openjdk.java.net/
>>> ~rasbold/8171119/webrev.08_09/
>>> (This version is compared to version 8 I last showed but ported to
>>> the new folder hierarchy)
>>>
>>> In this version I have:
>>> - Handled Thomas' comments from his email of 07/03:
>>> - Merged the logging to be standard
>>> - Fixed up the code a bit where asked
>>> - Added some notes about the code not being thread-safe yet
>>> - Removed additional dead code from the version that modifies
>>> interpreter/c1/c2
>>> - Fixed compiler issues so that it compiles with
>>> --disable-precompiled-header
>>> - Tested with ./configure --with-boot-jdk=<jdk8>
>>> --with-debug-level=slowdebug --disable-precompiled-headers
>>>
>>> Additionally, I added a test to check the sanity of the sampler:
>>> HeapMonitorStatCorrectnessTest (http://cr.openjdk.java.net/~r
>>> asbold/8171119/webrev.08_09/test/hotspot/jtreg/serviceabilit
>>> y/jvmti/HeapMonitor/MyPackage/HeapMonitorStatCorrectnessTest.java.patch)
>>> - This allocates a number of arrays and checks that we obtain the
>>> number of samples we want with an accepted error of 5%. I tested it 100
>>> times and it passed everytime, I can test more if wanted
>>> - Not in the test are the actual numbers I got for the various array
>>> sizes, I ran the program 30 times and parsed the output; here are the
>>> averages and standard deviation:
>>> 1000: 1.28% average; 1.13% standard deviation
>>> 10000: 1.59% average; 1.25% standard deviation
>>> 100000: 1.26% average; 1.26% standard deviation
>>>
>>> What this means is that we were always at about 1~2% of the number of
>>> samples the test expected.
>>>
>>> Let me know what you think,
>>> Jc
>>>
>>>
>>>
>>> On Wed, Jul 5, 2017 at 9:31 PM, JC Beyler <jcbeyler at google.com> wrote:
>>>
>>>> Hi all,
>>>>
>>>> I apologize, I have not yet handled your remarks but thought this new
>>>> webrev would also be useful to see and comment on perhaps.
>>>>
>>>> Here is the latest webrev, it is generated slightly different than the
>>>> others since now I'm using webrev.ksh without the -N option:
>>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.08/
>>>>
>>>> And the webrev.07 to webrev.08 diff is here:
>>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.07_08/
>>>>
>>>> (Let me know if it works well)
>>>>
>>>> It's a small change between versions but it:
>>>> - provides a fix that makes the average sample rate correct (more on
>>>> that below).
>>>> - fixes the code to actually have it play nicely with the fast tlab
>>>> refill
>>>> - cleaned up a bit the JVMTI text and now use jvmtiFrameInfo
>>>> - moved the capability to be onload solo
>>>>
>>>> With this webrev, I've done a small study of the random number
>>>> generator we use here for the sampling rate. I took a small program and it
>>>> can be simplified to:
>>>>
>>>> for (outer loop)
>>>> for (inner loop)
>>>> int[] tmp = new int[arraySize];
>>>>
>>>> - I've fixed the outer and inner loops to being 800 for this
>>>> experiment, meaning we allocate 640000 times an array of a given array
>>>> size.
>>>>
>>>> - Each program provides the average sample size used for the whole
>>>> execution
>>>>
>>>> - Then, I ran each variation 30 times and then calculated the average
>>>> of the average sample size used for various array sizes. I selected the
>>>> array size to be one of the following: 1, 10, 100, 1000.
>>>>
>>>> - When compared to 512kb, the average sample size of 30 runs:
>>>> 1: 4.62% of error
>>>> 10: 3.09% of error
>>>> 100: 0.36% of error
>>>> 1000: 0.1% of error
>>>> 10000: 0.03% of error
>>>>
>>>> What it shows is that, depending on the number of samples, the average
>>>> does become better. This is because with an allocation of 1 element per
>>>> array, it will take longer to hit one of the thresholds. This is seen by
>>>> looking at the sample count statistic I put in. For the same number of
>>>> iterations (800 * 800), the different array sizes provoke:
>>>> 1: 62 samples
>>>> 10: 125 samples
>>>> 100: 788 samples
>>>> 1000: 6166 samples
>>>> 10000: 57721 samples
>>>>
>>>> And of course, the more samples you have, the more sample rates you
>>>> pick, which means that your average gets closer using that math.
>>>>
>>>> Thanks,
>>>> Jc
>>>>
>>>> On Thu, Jun 29, 2017 at 10:01 PM, JC Beyler <jcbeyler at google.com>
>>>> wrote:
>>>>
>>>>> Thanks Robbin,
>>>>>
>>>>> This seems to have worked. When I have the next webrev ready, we will
>>>>> find out but I'm fairly confident it will work!
>>>>>
>>>>> Thanks agian!
>>>>> Jc
>>>>>
>>>>> On Wed, Jun 28, 2017 at 11:46 PM, Robbin Ehn <robbin.ehn at oracle.com>
>>>>> wrote:
>>>>>
>>>>>> Hi JC,
>>>>>>
>>>>>> On 06/29/2017 12:15 AM, JC Beyler wrote:
>>>>>>
>>>>>>> B) Incremental changes
>>>>>>>
>>>>>>
>>>>>> I guess the most common work flow here is using mq :
>>>>>> hg qnew fix_v1
>>>>>> edit files
>>>>>> hg qrefresh
>>>>>> hg qnew fix_v2
>>>>>> edit files
>>>>>> hg qrefresh
>>>>>>
>>>>>> if you do hg log you will see 2 commits
>>>>>>
>>>>>> webrev.ksh -r -2 -o my_inc_v1_v2
>>>>>> webrev.ksh -o my_full_v2
>>>>>>
>>>>>>
>>>>>> In your .hgrc you might need:
>>>>>> [extensions]
>>>>>> mq =
>>>>>>
>>>>>> /Robbin
>>>>>>
>>>>>>
>>>>>>> Again another newbiew question here...
>>>>>>>
>>>>>>> For showing the incremental changes, is there a link that explains
>>>>>>> how to do that? I apologize for my newbie questions all the time :)
>>>>>>>
>>>>>>> Right now, I do:
>>>>>>>
>>>>>>> ksh ../webrev.ksh -m -N
>>>>>>>
>>>>>>> That generates a webrev.zip and send it to Chuck Rasbold. He then
>>>>>>> uploads it to a new webrev.
>>>>>>>
>>>>>>> I tried commiting my change and adding a small change. Then if I
>>>>>>> just do ksh ../webrev.ksh without any options, it seems to produce a
>>>>>>> similar page but now with only the changes I had (so the 06-07 comparison
>>>>>>> you were talking about) and a changeset that has it all. I imagine that is
>>>>>>> what you meant.
>>>>>>>
>>>>>>> Which means that my workflow would become:
>>>>>>>
>>>>>>> 1) Make changes
>>>>>>> 2) Make a webrev without any options to show just the differences
>>>>>>> with the tip
>>>>>>> 3) Amend my changes to my local commit so that I have it done with
>>>>>>> 4) Go to 1
>>>>>>>
>>>>>>> Does that seem correct to you?
>>>>>>>
>>>>>>> Note that when I do this, I only see the full change of a file in
>>>>>>> the full change set (Side note here: now the page says change set and not
>>>>>>> patch, which is maybe why Serguei was having issues?).
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Jc
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Jun 28, 2017 at 1:12 AM, Robbin Ehn <robbin.ehn at oracle.com
>>>>>>> <mailto:robbin.ehn at oracle.com>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 06/28/2017 12:04 AM, JC Beyler wrote:
>>>>>>>
>>>>>>> Dear Thomas et al,
>>>>>>>
>>>>>>> Here is the newest webrev:
>>>>>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/ <
>>>>>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> You have some more bits to in there but generally this looks
>>>>>>> good and really nice with more tests.
>>>>>>> I'll do and deep dive and re-test this when I get back from my
>>>>>>> long vacation with whatever patch version you have then.
>>>>>>>
>>>>>>> Also I think it's time you provide incremental (v06->07 changes)
>>>>>>> as well as complete change-sets.
>>>>>>>
>>>>>>> Thanks, Robbin
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thomas, I "think" I have answered all your remarks. The
>>>>>>> summary is:
>>>>>>>
>>>>>>> - The statistic system is up and provides insight on what
>>>>>>> the heap sampler is doing
>>>>>>> - I've noticed that, though the sampling rate is at the
>>>>>>> right mean, we are missing some samples, I have not yet tracked out why
>>>>>>> (details below)
>>>>>>>
>>>>>>> - I've run a tiny benchmark that is the worse case: it is a
>>>>>>> very tight loop and allocated a small array
>>>>>>> - In this case, I see no overhead when the system is
>>>>>>> off so that is a good start :)
>>>>>>> - I see right now a high overhead in this case when
>>>>>>> sampling is on. This is not a really too surprising but I'm going to see if
>>>>>>> this is consistent with our
>>>>>>> internal implementation. The benchmark is really allocation
>>>>>>> stressful so I'm not too surprised but I want to do the due diligence.
>>>>>>>
>>>>>>> - The statistic system up is up and I have a new test
>>>>>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/test/s
>>>>>>> erviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatTes
>>>>>>> t.java.patch
>>>>>>> <http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/test/
>>>>>>> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorStatTe
>>>>>>> st.java.patch>
>>>>>>> - I did a bit of a study about the random generator
>>>>>>> here, more details are below but basically it seems to work well
>>>>>>>
>>>>>>> - I added a capability but since this is the first time
>>>>>>> doing this, I was not sure I did it right
>>>>>>> - I did add a test though for it and the test seems to
>>>>>>> do what I expect (all methods are failing with the
>>>>>>> JVMTI_ERROR_MUST_POSSESS_CAPABILITY error).
>>>>>>> - http://cr.openjdk.java.net/~ra
>>>>>>> sbold/8171119/webrev.07/test/serviceability/jvmti/HeapMonito
>>>>>>> r/MyPackage/HeapMonitorNoCapabilityTest.java.patch
>>>>>>> <http://cr.openjdk.java.net/~rasbold/8171119/webrev.07/test/
>>>>>>> serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorNoCapa
>>>>>>> bilityTest.java.patch>
>>>>>>>
>>>>>>> - I still need to figure out what to do about the
>>>>>>> multi-agent vs single-agent issue
>>>>>>>
>>>>>>> - As far as measurements, it seems I still need to look
>>>>>>> at:
>>>>>>> - Why we do the 20 random calls first, are they
>>>>>>> necessary?
>>>>>>> - Look at the mean of the sampling rate that the random
>>>>>>> generator does and also what is actually sampled
>>>>>>> - What is the overhead in terms of memory/performance
>>>>>>> when on?
>>>>>>>
>>>>>>> I have inlined my answers, I think I got them all in the new
>>>>>>> webrev, let me know your thoughts.
>>>>>>>
>>>>>>> Thanks again!
>>>>>>> Jc
>>>>>>>
>>>>>>>
>>>>>>> On Fri, Jun 23, 2017 at 3:52 AM, Thomas Schatzl <
>>>>>>> thomas.schatzl at oracle.com <mailto:thomas.schatzl at oracle.com>
>>>>>>> <mailto:thomas.schatzl at oracle.com
>>>>>>>
>>>>>>> <mailto:thomas.schatzl at oracle.com>>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Wed, 2017-06-21 at 13:45 -0700, JC Beyler wrote:
>>>>>>> > Hi all,
>>>>>>> >
>>>>>>> > First off: Thanks again to Robbin and Thomas for
>>>>>>> their reviews :)
>>>>>>> >
>>>>>>> > Next, I've uploaded a new webrev:
>>>>>>> > http://cr.openjdk.java.net/~ra
>>>>>>> sbold/8171119/webrev.06/ <http://cr.openjdk.java.net/~r
>>>>>>> asbold/8171119/webrev.06/>
>>>>>>> <http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/ <
>>>>>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/>>
>>>>>>>
>>>>>>> >
>>>>>>> > Here is an update:
>>>>>>> >
>>>>>>> > - @Robbin, I forgot to say that yes I need to look at
>>>>>>> implementing
>>>>>>> > this for the other architectures and testing it
>>>>>>> before it is all
>>>>>>> > ready to go. Is it common to have it working on all
>>>>>>> possible
>>>>>>> > combinations or is there a subset that I should be
>>>>>>> doing first and we
>>>>>>> > can do the others later?
>>>>>>> > - I've tested slowdebug, built and ran the JTreg
>>>>>>> tests I wrote with
>>>>>>> > slowdebug and fixed a few more issues
>>>>>>> > - I've refactored a bit of the code following Thomas'
>>>>>>> comments
>>>>>>> > - I think I've handled all the comments from
>>>>>>> Thomas (I put
>>>>>>> > comments inline below for the specifics)
>>>>>>>
>>>>>>> Thanks for handling all those.
>>>>>>>
>>>>>>> > - Following Thomas' comments on statistics, I want to
>>>>>>> add some
>>>>>>> > quality assurance tests and find that the easiest way
>>>>>>> would be to
>>>>>>> > have a few counters of what is happening in the
>>>>>>> sampler and expose
>>>>>>> > that to the user.
>>>>>>> > - I'll be adding that in the next version if no
>>>>>>> one sees any
>>>>>>> > objections to that.
>>>>>>> > - This will allow me to add a sanity test in JTreg
>>>>>>> about number of
>>>>>>> > samples and average of sampling rate
>>>>>>> >
>>>>>>> > @Thomas: I had a few questions that I inlined below
>>>>>>> but I will
>>>>>>> > summarize the "bigger ones" here:
>>>>>>> > - You mentioned constants are not using the right
>>>>>>> conventions, I
>>>>>>> > looked around and didn't see any convention except
>>>>>>> normal naming then
>>>>>>> > for static constants. Is that right?
>>>>>>>
>>>>>>> I looked through https://wiki.openjdk.java.net/
>>>>>>> display/HotSpot/StyleGui <https://wiki.openjdk.java.net
>>>>>>> /display/HotSpot/StyleGui>
>>>>>>> <https://wiki.openjdk.java.net/display/HotSpot/StyleGui <
>>>>>>> https://wiki.openjdk.java.net/display/HotSpot/StyleGui>>
>>>>>>> de and the rule is to "follow an existing pattern and
>>>>>>> must have a
>>>>>>> distinct appearance from other names". Which does not
>>>>>>> help a lot I
>>>>>>> guess :/ The GC team started using upper camel case,
>>>>>>> e.g.
>>>>>>> SomeOtherConstant, but very likely this is probably not
>>>>>>> applied
>>>>>>> consistently throughout. So I am fine with not adding
>>>>>>> another style
>>>>>>> (like kMaxStackDepth with the "k" in front with some
>>>>>>> unknown meaning)
>>>>>>> is fine.
>>>>>>>
>>>>>>> (Chances are you will find that style somewhere used
>>>>>>> anyway too,
>>>>>>> apologies if so :/)
>>>>>>>
>>>>>>>
>>>>>>> Thanks for that link, now I know where to look. I used the
>>>>>>> upper camel case in my code as well then :) I should have gotten them all.
>>>>>>>
>>>>>>>
>>>>>>> > PS: I've also inlined my answers to Thomas below:
>>>>>>> >
>>>>>>> > On Tue, Jun 13, 2017 at 8:03 AM, Thomas Schatzl
>>>>>>> <thomas.schatzl at oracl
>>>>>>> > e.com <http://e.com> <http://e.com>> wrote:
>>>>>>> > > Hi all,
>>>>>>> > >
>>>>>>> > > On Mon, 2017-06-12 at 11:11 -0700, JC Beyler wrote:
>>>>>>> > > > Dear all,
>>>>>>> > > >
>>>>>>> > > > I've continued working on this and have done the
>>>>>>> following
>>>>>>> > > webrev:
>>>>>>> > > > http://cr.openjdk.java.net/~ra
>>>>>>> sbold/8171119/webrev.05/ <http://cr.openjdk.java.net/~r
>>>>>>> asbold/8171119/webrev.05/>
>>>>>>> <http://cr.openjdk.java.net/~rasbold/8171119/webrev.05/ <
>>>>>>> http://cr.openjdk.java.net/~rasbold/8171119/webrev.05/>>
>>>>>>>
>>>>>>> > >
>>>>>>> > > [...]
>>>>>>> > > > Things I still need to do:
>>>>>>> > > > - Have to fix that TLAB case for the
>>>>>>> FastTLABRefill
>>>>>>> > > > - Have to start looking at the data to see
>>>>>>> that it is
>>>>>>> > > consistent and does gather the right samples,
>>>>>>> right frequency, etc.
>>>>>>> > > > - Have to check the GC elements and what that
>>>>>>> produces
>>>>>>> > > > - Run a slowdebug run and ensure I fixed all
>>>>>>> those issues you
>>>>>>> > > saw > Robbin
>>>>>>> > > >
>>>>>>> > > > Thanks for looking at the webrev and have a
>>>>>>> great week!
>>>>>>> > >
>>>>>>> > > scratching a bit on the surface of this change,
>>>>>>> so apologies for
>>>>>>> > > rather shallow comments:
>>>>>>> > >
>>>>>>> > > - macroAssembler_x86.cpp:5604: while this is
>>>>>>> compiler code, and I
>>>>>>> > > am not sure this is final, please avoid littering
>>>>>>> the code with
>>>>>>> > > TODO remarks :) They tend to be candidates for
>>>>>>> later wtf moments
>>>>>>> > > only.
>>>>>>> > >
>>>>>>> > > Just file a CR for that.
>>>>>>> > >
>>>>>>> > Newcomer question: what is a CR and not sure I have
>>>>>>> the rights to do
>>>>>>> > that yet ? :)
>>>>>>>
>>>>>>> Apologies. CR is a change request, this suggests to
>>>>>>> file a bug in the
>>>>>>> bug tracker. And you are right, you can't just create a
>>>>>>> new account in
>>>>>>> the OpenJDK JIRA yourselves. :(
>>>>>>>
>>>>>>>
>>>>>>> Ok good to know, I'll continue with my own todo list but
>>>>>>> I'll work hard on not letting it slip in the webrevs anymore :)
>>>>>>>
>>>>>>>
>>>>>>> I was mostly referring to the "... but it is a TODO"
>>>>>>> part of that
>>>>>>> comment in macroassembler_x86.cpp. Comments about the
>>>>>>> why of the code
>>>>>>> are appreciated.
>>>>>>>
>>>>>>> [Note that I now understand that this is to some degree
>>>>>>> still work in
>>>>>>> progress. As long as the final changeset does no
>>>>>>> contain TODO's I am
>>>>>>> fine (and it's not a hard objection, rather their use
>>>>>>> in "final" code
>>>>>>> is typically limited in my experience)]
>>>>>>>
>>>>>>> 5603 // Currently, if this happens, just set back the
>>>>>>> actual end to
>>>>>>> where it was.
>>>>>>> 5604 // We miss a chance to sample here.
>>>>>>>
>>>>>>> Would be okay, if explaining "this" and the "why" of
>>>>>>> missing a chance
>>>>>>> to sample here would be best.
>>>>>>>
>>>>>>> Like maybe:
>>>>>>>
>>>>>>> // If we needed to refill TLABs, just set the actual
>>>>>>> end point to
>>>>>>> // the end of the TLAB again. We do not sample here
>>>>>>> although we could.
>>>>>>>
>>>>>>> Done with your comment, it works well in my mind.
>>>>>>>
>>>>>>> I am not sure whether "miss a chance to sample" meant
>>>>>>> "we could, but
>>>>>>> consciously don't because it's not that useful" or "it
>>>>>>> would be
>>>>>>> necessary but don't because it's too complicated to
>>>>>>> do.".
>>>>>>>
>>>>>>> Looking at the original comment once more, I am also
>>>>>>> not sure if that
>>>>>>> comment shouldn't referring to the "end" variable (not
>>>>>>> actual_end)
>>>>>>> because that's the variable that is responsible for
>>>>>>> taking the sampling
>>>>>>> path? (Going from the member description of
>>>>>>> ThreadLocalAllocBuffer).
>>>>>>>
>>>>>>>
>>>>>>> I've moved this code and it no longer shows up here but the
>>>>>>> rationale and answer was:
>>>>>>>
>>>>>>> So.. Yes, end is the variable provoking the sampling. Actual
>>>>>>> end is the actual end of the TLAB.
>>>>>>>
>>>>>>> What was happening here is that the code is resetting _end
>>>>>>> to point towards the end of the new TLAB. Because, we now have the end for
>>>>>>> sampling and _actual_end for
>>>>>>> the actual end, we need to update the actual_end as well.
>>>>>>>
>>>>>>> Normally, were we to do the real work here, we would
>>>>>>> calculate the (end - start) offset, then do:
>>>>>>>
>>>>>>> - Set the new end to : start + (old_end - old_start)
>>>>>>> - Set the actual end like we do here now where it because it
>>>>>>> is the actual end.
>>>>>>>
>>>>>>> Why is this not done here now anymore?
>>>>>>> - I was still debating which path to take:
>>>>>>> - Do it in the fast refill code, it has its perks:
>>>>>>> - In a world where fast refills are happening all
>>>>>>> the time or a lot, we can augment there the code to do the sampling
>>>>>>> - Remember what we had as an end before leaving the
>>>>>>> slowpath and check on return
>>>>>>> - This is what I'm doing now, it removes the need
>>>>>>> to go fix up all fast refill paths but if you remain in fast refill paths,
>>>>>>> you won't get sampling. I
>>>>>>> have to think of the consequences of that, maybe a future
>>>>>>> change later on?
>>>>>>> - I have the statistics now so I'm going to
>>>>>>> study that
>>>>>>> -> By the way, though my statistics are
>>>>>>> showing I'm missing some samples, if I turn off FastTlabRefill, it is the
>>>>>>> same loss so for now, it seems
>>>>>>> this does not occur in my simple test.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> But maybe I am only confused and it's best to just
>>>>>>> leave the comment
>>>>>>> away. :)
>>>>>>>
>>>>>>> Thinking about it some more, doesn't this not-sampling
>>>>>>> in this case
>>>>>>> mean that sampling does not work in any collector that
>>>>>>> does inline TLAB
>>>>>>> allocation at the moment? (Or is inline TLAB alloc
>>>>>>> automatically
>>>>>>> disabled with sampling somehow?)
>>>>>>>
>>>>>>> That would indeed be a bigger TODO then :)
>>>>>>>
>>>>>>>
>>>>>>> Agreed, this remark made me think that perhaps as a first
>>>>>>> step the new way of doing it is better but I did have to:
>>>>>>> - Remove the const of the ThreadLocalBuffer remaining and
>>>>>>> hard_end methods
>>>>>>> - Move hard_end out of the header file to have a bit more
>>>>>>> logic there
>>>>>>>
>>>>>>> Please let me know what you think of that and if you prefer
>>>>>>> it this way or changing the fast refills. (I prefer this way now because it
>>>>>>> is more incremental).
>>>>>>>
>>>>>>>
>>>>>>> > > - calling HeapMonitoring::do_weak_oops() (which
>>>>>>> should probably be
>>>>>>> > > called weak_oops_do() like other similar methods)
>>>>>>> only if string
>>>>>>> > > deduplication is enabled (in
>>>>>>> g1CollectedHeap.cpp:4511) seems wrong.
>>>>>>> >
>>>>>>> > The call should be at least around 6 lines up outside
>>>>>>> the if.
>>>>>>> >
>>>>>>> > Preferentially in a method like
>>>>>>> process_weak_jni_handles(), including
>>>>>>> > additional logging. (No new (G1) gc phase without
>>>>>>> minimal logging
>>>>>>> > :)).
>>>>>>> > Done but really not sure because:
>>>>>>> >
>>>>>>> > I put for logging:
>>>>>>> > log_develop_trace(gc,
>>>>>>> freelist)("G1ConcRegionFreeing [other] : heap
>>>>>>> > monitoring");
>>>>>>>
>>>>>>> I would think that "gc, ref" would be more appropriate
>>>>>>> log tags for
>>>>>>> this similar to jni handles.
>>>>>>> (I am als not sure what weak reference handling has to
>>>>>>> do with
>>>>>>> G1ConcRegionFreeing, so I am a bit puzzled)
>>>>>>>
>>>>>>>
>>>>>>> I was not sure what to put for the tags or really as the
>>>>>>> message. I cleaned it up a bit now to:
>>>>>>> log_develop_trace(gc, ref)("HeapSampling [other] : heap
>>>>>>> monitoring processing");
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> > Since weak_jni_handles didn't have logging for me to
>>>>>>> be inspired
>>>>>>> > from, I did that but unconvinced this is what should
>>>>>>> be done.
>>>>>>>
>>>>>>> The JNI handle processing does have logging, but only in
>>>>>>> ReferenceProcessor::process_discovered_references(). In
>>>>>>> process_weak_jni_handles() only overall time is
>>>>>>> measured (in a G1
>>>>>>> specific way, since only G1 supports disabling
>>>>>>> reference procesing) :/
>>>>>>>
>>>>>>> The code in ReferenceProcessor prints both time taken
>>>>>>> referenceProcessor.cpp:254, as well as the count, but
>>>>>>> strangely only in
>>>>>>> debug VMs.
>>>>>>>
>>>>>>> I have no idea why this logging is that unimportant to
>>>>>>> only print that
>>>>>>> in a debug VM. However there are reviews out for
>>>>>>> changing this area a
>>>>>>> bit, so it might be useful to wait for that
>>>>>>> (JDK-8173335).
>>>>>>>
>>>>>>>
>>>>>>> I cleaned it up a bit anyway and now it returns the count of
>>>>>>> objects that are in the system.
>>>>>>>
>>>>>>>
>>>>>>> > > - the change doubles the size of
>>>>>>> > > CollectedHeap::allocate_from_tlab_slow() above the
>>>>>>> "small and nice"
>>>>>>> > > threshold. Maybe it could be refactored a bit.
>>>>>>> > Done I think, it looks better to me :).
>>>>>>>
>>>>>>> In ThreadLocalAllocBuffer::handle_sample() I think the
>>>>>>> set_back_actual_end()/pick_next_sample() calls could
>>>>>>> be hoisted out of
>>>>>>> the "if" :)
>>>>>>>
>>>>>>>
>>>>>>> Done!
>>>>>>>
>>>>>>>
>>>>>>> > > - referenceProcessor.cpp:261: the change should add
>>>>>>> logging about
>>>>>>> > > the number of references encountered, maybe after
>>>>>>> the corresponding
>>>>>>> > > "JNI weak reference count" log message.
>>>>>>> > Just to double check, are you saying that you'd like
>>>>>>> to have the heap
>>>>>>> > sampler to keep in store how many sampled objects
>>>>>>> were encountered in
>>>>>>> > the HeapMonitoring::weak_oops_do?
>>>>>>> > - Would a return of the method with the number of
>>>>>>> handled
>>>>>>> > references and logging that work?
>>>>>>>
>>>>>>> Yes, it's fine if HeapMonitoring::weak_oops_do() only
>>>>>>> returned the
>>>>>>> number of processed weak oops.
>>>>>>>
>>>>>>>
>>>>>>> Done also (but I admit I have not tested the output yet) :)
>>>>>>>
>>>>>>>
>>>>>>> > - Additionally, would you prefer it in a separate
>>>>>>> block with its
>>>>>>> > GCTraceTime?
>>>>>>>
>>>>>>> Yes. Both kinds of information is interesting: while
>>>>>>> the time taken is
>>>>>>> typically more important, the next question would be
>>>>>>> why, and the
>>>>>>> number of references typically goes a long way there.
>>>>>>>
>>>>>>> See above though, it is probably best to wait a bit.
>>>>>>>
>>>>>>>
>>>>>>> Agreed that I "could" wait but, if it's ok, I'll just
>>>>>>> refactor/remove this when we get closer to something final. Either,
>>>>>>> JDK-8173335
>>>>>>> has gone in and I will notice it now or it will soon and I
>>>>>>> can change it then.
>>>>>>>
>>>>>>>
>>>>>>> > > - threadLocalAllocBuffer.cpp:331: one more "TODO"
>>>>>>> > Removed it and added it to my personal todos to look
>>>>>>> at.
>>>>>>> > > >
>>>>>>> > > - threadLocalAllocBuffer.hpp:
>>>>>>> ThreadLocalAllocBuffer class
>>>>>>> > > documentation should be updated about the sampling
>>>>>>> additions. I
>>>>>>> > > would have no clue what the difference between
>>>>>>> "actual_end" and
>>>>>>> > > "end" would be from the given information.
>>>>>>> > If you are talking about the comments in this file, I
>>>>>>> made them more
>>>>>>> > clear I hope in the new webrev. If it was somewhere
>>>>>>> else, let me know
>>>>>>> > where to change.
>>>>>>>
>>>>>>> Thanks, that's much better. Maybe a note in the comment
>>>>>>> of the class
>>>>>>> that ThreadLocalBuffer provides some sampling facility
>>>>>>> by modifying the
>>>>>>> end() of the TLAB to cause "frequent" calls into the
>>>>>>> runtime call where
>>>>>>> actual sampling takes place.
>>>>>>>
>>>>>>>
>>>>>>> Done, I think it's better now. Added something about the
>>>>>>> slow_path_end as well.
>>>>>>>
>>>>>>>
>>>>>>> > > - in heapMonitoring.hpp: there are some random
>>>>>>> comments about some
>>>>>>> > > code that has been grabbed from
>>>>>>> "util/math/fastmath.[h|cc]". I
>>>>>>> > > can't tell whether this is code that can be used
>>>>>>> but I assume that
>>>>>>> > > Noam Shazeer is okay with that (i.e. that's all
>>>>>>> Google code).
>>>>>>> > Jeremy and I double checked and we can release that
>>>>>>> as I thought. I
>>>>>>> > removed the comment from that piece of code entirely.
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>> > > - heapMonitoring.hpp/cpp static constant naming
>>>>>>> does not correspond
>>>>>>> > > to Hotspot's. Additionally, in Hotspot static
>>>>>>> methods are cased
>>>>>>> > > like other methods.
>>>>>>> > I think I fixed the methods to be cased the same way
>>>>>>> as all other
>>>>>>> > methods. For static constants, I was not sure. I
>>>>>>> fixed a few other
>>>>>>> > variables but I could not seem to really see a
>>>>>>> consistent trend for
>>>>>>> > constants. I made them as variables but I'm not sure
>>>>>>> now.
>>>>>>>
>>>>>>> Sorry again, style is a kind of mess. The goal of my
>>>>>>> suggestions here
>>>>>>> is only to prevent yet another style creeping in.
>>>>>>>
>>>>>>> > > - in heapMonitoring.cpp there are a few cryptic
>>>>>>> comments at the top
>>>>>>> > > that seem to refer to internal stuff that should
>>>>>>> probably be
>>>>>>> > > removed.
>>>>>>> > Sorry about that! My personal todos not cleared out.
>>>>>>>
>>>>>>> I am happy about comments, but I simply did not
>>>>>>> understand any of that
>>>>>>> and I do not know about other readers as well.
>>>>>>>
>>>>>>> If you think you will remember removing/updating them
>>>>>>> until the review
>>>>>>> proper (I misunderstood the review situation a little
>>>>>>> it seems).
>>>>>>>
>>>>>>> > > I did not think through the impact of the TLAB
>>>>>>> changes on collector
>>>>>>> > > behavior yet (if there are). Also I did not check
>>>>>>> for problems with
>>>>>>> > > concurrent mark and SATB/G1 (if there are).
>>>>>>> > I would love to know your thoughts on this, I think
>>>>>>> this is fine. I
>>>>>>>
>>>>>>> I think so too now. No objects are made live out of
>>>>>>> thin air :)
>>>>>>>
>>>>>>> > see issues with multiple threads right now hitting
>>>>>>> the stack storage
>>>>>>> > instance. Previous webrevs had a mutex lock here but
>>>>>>> we took it out
>>>>>>> > for simplificity (and only for now).
>>>>>>>
>>>>>>> :) When looking at this after some thinking I now
>>>>>>> assume for this
>>>>>>> review that this code is not MT safe at all. There
>>>>>>> seems to be more
>>>>>>> synchronization missing than just the one for the
>>>>>>> StackTraceStorage. So
>>>>>>> no comments about this here.
>>>>>>>
>>>>>>>
>>>>>>> I doubled checked a bit (quickly I admit) but it seems that
>>>>>>> synchronization in StackTraceStorage is really all you need (all methods
>>>>>>> lead to a StackTraceStorage one
>>>>>>> and can be multithreaded outside of that).
>>>>>>> There is a question about the initialization where the
>>>>>>> method HeapMonitoring::initialize_profiling is not thread safe.
>>>>>>> It would work (famous last words) and not crash if there was
>>>>>>> a race but we could add a synchronization point there as well (and
>>>>>>> therefore on the stop as well).
>>>>>>>
>>>>>>> But anyway I will really check and do this once we add back
>>>>>>> synchronization.
>>>>>>>
>>>>>>>
>>>>>>> Also, this would require some kind of specification of
>>>>>>> what is allowed
>>>>>>> to be called when and where.
>>>>>>>
>>>>>>>
>>>>>>> Would we specify this with the methods in the jvmti.xml
>>>>>>> file? We could start by specifying in each that they are not thread safe
>>>>>>> but I saw no mention of that for
>>>>>>> other methods.
>>>>>>>
>>>>>>>
>>>>>>> One potentially relevant observation about locking
>>>>>>> here: depending on
>>>>>>> sampling frequency, StackTraceStore::add_trace() may be
>>>>>>> rather
>>>>>>> frequently called. I assume that you are going to do
>>>>>>> measurements :)
>>>>>>>
>>>>>>>
>>>>>>> Though we don't have the TLAB implementation in our code,
>>>>>>> the compiler generated sampler uses 2% of overhead with a 512k sampling
>>>>>>> rate. I can do real measurements
>>>>>>> when the code settles and we can see how costly this is as a
>>>>>>> TLAB implementation.
>>>>>>> However, my theory is that if the rate is 512k, the
>>>>>>> memory/performance overhead should be minimal since it is what we saw with
>>>>>>> our code/workloads (though not called
>>>>>>> the same way, we call it essentially at the same rate).
>>>>>>> If you have a benchmark you'd like me to test, let me know!
>>>>>>>
>>>>>>> Right now, with my really small test, this does use a bit of
>>>>>>> overhead even for a 512k sample size. I don't know yet why, I'm going to
>>>>>>> see what is going on.
>>>>>>>
>>>>>>> Finally, I think it is not reasonable to suppose the
>>>>>>> overhead to be negligible if the sampling rate used is too low. The user
>>>>>>> should know that the lower the rate,
>>>>>>> the higher the overhead (documentation TODO?).
>>>>>>>
>>>>>>>
>>>>>>> I am not sure what the expected usage of the API is, but
>>>>>>> StackTraceStore::add_trace() seems to be able to grow
>>>>>>> without bounds.
>>>>>>> Only a GC truncates them to the live ones. That in
>>>>>>> itself seems to be
>>>>>>> problematic (GCs can be *wide* apart), and of course
>>>>>>> some of the API
>>>>>>> methods add to that because they duplicate that
>>>>>>> unbounded array. Do you
>>>>>>> have any concerns/measurements about this?
>>>>>>>
>>>>>>>
>>>>>>> So, the theory is that yes add_trace can be able to grow
>>>>>>> without bounds but it grows at a sample per 512k of allocated space. The
>>>>>>> stacks it gathers are currently
>>>>>>> maxed at 64 (I'd like to expand that to an option to the
>>>>>>> user though at some point). So I have no concerns because:
>>>>>>>
>>>>>>> - If really this is taking a lot of space, that means the
>>>>>>> job is keeping a lot of objects in memory as well, therefore the entire
>>>>>>> heap is getting huge
>>>>>>> - If this is the case, you will be triggering a GC at some
>>>>>>> point anyway.
>>>>>>>
>>>>>>> (I'm putting under the rug the issue of "What if we set the
>>>>>>> rate to 1 for example" because as you lower the sampling rate, we cannot
>>>>>>> guarantee low overhead; the
>>>>>>> idea behind this feature is to have a means of having
>>>>>>> meaningful allocated samples at a low overhead)
>>>>>>>
>>>>>>> I have no measurements really right now but since I now have
>>>>>>> some statistics I can poll, I will look a bit more at this question.
>>>>>>>
>>>>>>> I have the same last sentence than above: the user should
>>>>>>> expect this to happen if the sampling rate is too small. That probably can
>>>>>>> be reflected in the
>>>>>>> StartHeapSampling as a note : careful this might impact your
>>>>>>> performance.
>>>>>>>
>>>>>>>
>>>>>>> Also, these stack traces might hold on to huge arrays.
>>>>>>> Any
>>>>>>> consideration of that? Particularly it might be the
>>>>>>> cause for OOMEs in
>>>>>>> tight memory situations.
>>>>>>>
>>>>>>>
>>>>>>> There is a stack size maximum that is set to 64 so it should
>>>>>>> not hold huge arrays. I don't think this is an issue but I can double check
>>>>>>> with a test or two.
>>>>>>>
>>>>>>>
>>>>>>> - please consider adding a safepoint check in
>>>>>>> HeapMonitoring::weak_oops_do to prevent accidental
>>>>>>> misuse.
>>>>>>>
>>>>>>> - in struct StackTraceStorage, the public fields may
>>>>>>> also need
>>>>>>> underscores. At least some files in the runtime
>>>>>>> directory have structs
>>>>>>> with underscored public members (and some don't). The
>>>>>>> runtime team
>>>>>>> should probably comment on that.
>>>>>>>
>>>>>>>
>>>>>>> Agreed I did not know. I looked around and a lot of structs
>>>>>>> did not have them it seemed so I left it as is. I will happily change it if
>>>>>>> someone prefers (I was not
>>>>>>> sure if you really preferred or not, your sentence seemed to
>>>>>>> be more a note of "this might need to change but I don't know if the
>>>>>>> runtime team enforces that", let
>>>>>>> me know if I read that wrongly).
>>>>>>>
>>>>>>>
>>>>>>> - In StackTraceStorage::weak_oops_do(), when examining
>>>>>>> the
>>>>>>> StackTraceData, maybe it is useful to consider having a
>>>>>>> non-NULL
>>>>>>> reference outside of the heap's reserved space an
>>>>>>> error. There should
>>>>>>> be no oop outside of the heap's reserved space ever.
>>>>>>>
>>>>>>> Unless you allow storing random values in
>>>>>>> StackTraceData::obj, which I
>>>>>>> would not encourage.
>>>>>>>
>>>>>>>
>>>>>>> I suppose you are talking about this part:
>>>>>>> if ((value != NULL && Universe::heap()->is_in_reserved(value))
>>>>>>> &&
>>>>>>> (is_alive == NULL ||
>>>>>>> is_alive->do_object_b(value))) {
>>>>>>>
>>>>>>> What you are saying is that I could have something like:
>>>>>>> if (value != my_non_null_reference &&
>>>>>>> (is_alive == NULL ||
>>>>>>> is_alive->do_object_b(value))) {
>>>>>>>
>>>>>>> Is that what you meant? Is there really a reason to do so?
>>>>>>> When I look at the code, is_in_reserved seems like a O(1) method call. I'm
>>>>>>> not even sure we can have a
>>>>>>> NULL value to be honest. I might have to study that to see
>>>>>>> if this was not a paranoid test to begin with.
>>>>>>>
>>>>>>> The is_alive code has now morphed due to the comment below.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> - HeapMonitoring::weak_oops_do() does not seem to use
>>>>>>> the
>>>>>>> passed AbstractRefProcTaskExecutor.
>>>>>>>
>>>>>>>
>>>>>>> It did use it:
>>>>>>> size_t HeapMonitoring::weak_oops_do(
>>>>>>> AbstractRefProcTaskExecutor *task_executor,
>>>>>>> BoolObjectClosure* is_alive,
>>>>>>> OopClosure *f,
>>>>>>> VoidClosure *complete_gc) {
>>>>>>> assert(SafepointSynchronize::is_at_safepoint(), "must
>>>>>>> be at safepoint");
>>>>>>>
>>>>>>> if (task_executor != NULL) {
>>>>>>> task_executor->set_single_threaded_mode();
>>>>>>> }
>>>>>>> return StackTraceStorage::storage()->weak_oops_do(is_alive,
>>>>>>> f, complete_gc);
>>>>>>> }
>>>>>>>
>>>>>>> But due to the comment below, I refactored this, so this is
>>>>>>> no longer here. Now I have an always true closure that is passed.
>>>>>>>
>>>>>>>
>>>>>>> - I do not understand allowing to call this method with
>>>>>>> a NULL
>>>>>>> complete_gc closure. This would mean that objects
>>>>>>> referenced from the
>>>>>>> object that is referenced by the StackTraceData are not
>>>>>>> pulled, meaning
>>>>>>> they would get stale.
>>>>>>>
>>>>>>> - same with is_alive parameter value of NULL
>>>>>>>
>>>>>>>
>>>>>>> So these questions made me look a bit closer at this code.
>>>>>>> This code I think was written this way to have a very small impact on the
>>>>>>> file but you are right, there
>>>>>>> is no reason for this here. I've simplified the code by
>>>>>>> making in referenceProcessor.cpp a process_HeapSampling method that handles
>>>>>>> everything there.
>>>>>>>
>>>>>>> The code allowed NULLs because it depended on where you were
>>>>>>> coming from and how the code was being called.
>>>>>>>
>>>>>>> - I added a static always_true variable and pass that now to
>>>>>>> be more consistent with the rest of the code.
>>>>>>> - I moved the complete_gc into process_phaseHeapSampling now
>>>>>>> (new method) and handle the task_executor and the complete_gc there
>>>>>>> - Newbie question: in our code we did a
>>>>>>> set_single_threaded_mode but I see that process_phaseJNI does it right
>>>>>>> before its call, do I need to do it for the
>>>>>>> process_phaseHeapSample?
>>>>>>> That API is much cleaner (in my mind) and is consistent with
>>>>>>> what is done around it (again in my mind).
>>>>>>>
>>>>>>>
>>>>>>> - heapMonitoring.cpp:590: I do not completely
>>>>>>> understand the purpose of
>>>>>>> this code: in the end this results in a fixed value
>>>>>>> directly dependent
>>>>>>> on the Thread address anyway? In the end this results
>>>>>>> in a fixed value
>>>>>>> directly dependent on the Thread address anyway?
>>>>>>> IOW, what is special about exactly 20 rounds?
>>>>>>>
>>>>>>>
>>>>>>> So we really want a fast random number generator that has a
>>>>>>> specific mean (512k is the default we use). The code uses the thread
>>>>>>> address as the start number of the
>>>>>>> sequence (why not, it is random enough is rationale). Then
>>>>>>> instead of just starting there, we prime the sequence and really only start
>>>>>>> at the 21st number, it is
>>>>>>> arbitrary and I have not done a study to see if we could do
>>>>>>> more or less of that.
>>>>>>>
>>>>>>> As I have the statistics of the system up and running, I'll
>>>>>>> run some experiments to see if this is needed, is 20 good, or not.
>>>>>>>
>>>>>>>
>>>>>>> - also I would consider stripping a few bits of the
>>>>>>> threads' address as
>>>>>>> initialization value for your rng. The last three bits
>>>>>>> (and probably
>>>>>>> more, check whether the Thread object is allocated on
>>>>>>> special
>>>>>>> boundaries) are always zero for them.
>>>>>>> Not sure if the given "random" value is random enough
>>>>>>> before/after,
>>>>>>> this method, so just skip that comment if you think
>>>>>>> this is not
>>>>>>> required.
>>>>>>>
>>>>>>>
>>>>>>> I don't know is the honest answer. I think what is important
>>>>>>> is that we tend towards a mean and it is random "enough" to not fall in
>>>>>>> pitfalls of only sampling a
>>>>>>> subset of objects due to their allocation order. I added
>>>>>>> that as test to do to see if it changes the mean in any way for the 512k
>>>>>>> default value and/or if the first
>>>>>>> 1000 elements look better.
>>>>>>>
>>>>>>>
>>>>>>> Some more random nits I did not find a place to put
>>>>>>> anywhere:
>>>>>>>
>>>>>>> - ThreadLocalAllocBuffer::_extra_space does not seem
>>>>>>> to be used
>>>>>>> anywhere?
>>>>>>>
>>>>>>>
>>>>>>> Good catch :).
>>>>>>>
>>>>>>>
>>>>>>> - Maybe indent the declaration of
>>>>>>> ThreadLocalAllocBuffer::_bytes_until_sample to align below the
>>>>>>> other members of that group.
>>>>>>>
>>>>>>>
>>>>>>> Done moved it up a bit to have non static members together
>>>>>>> and static separate.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Thomas
>>>>>>>
>>>>>>>
>>>>>>> Thanks for your review!
>>>>>>> Jc
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20171009/33dd3574/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list