Hi,

On 06/21/2017 10:45 PM, 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/~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 will change my mind here and proposed a different approach.
Right now the only compiler platform change you have is for the FastTLABRefill.
FastTLABRefill works for all gc except G1 (CMS is deprecated) but only in C1 
compiled code.
The performance angle of this is not really relevant, since then you will have 
C2 compiled code.

So I suggested that we should remove FastTLABRefill completely, I will try to 
makes this happen but most likely not before my long vacation.

That means you can avoid all platform specific code (as patch is now), correct?

If so just keep the x86 support in there and we remove that if I succeed, plus 
you can support all platforms when FastTLABRefill=false.

Sounds good?

If you had any other questions I missed please let me know.
I'll look at your latest webrev, and response to that also.

Thanks!

/Robbin


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

The biggest addition to this webrev is that there is more testing, I've added 
two tests for looking specifically at the two garbage sampling data Recent vs 
Frequent:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorRecentTest.java.patch
    and
http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorFrequentTest.java.patch

I've also refactored the JNI library a bit: 
http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.patch.
    - Side question: I was looking at trying to make this a multi-file library 
so you would not have all the code in one spot. It seems this is not really 
done?
       - Is there a solution to this? What I really wanted was:
         - The core library that will help testing be easier
         - The JNI code for each Java class separate and calling into the core 
library

- 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?
    - You mentioned putting the weak_oops_do in a separate method and logging, 
I inlined my answer below and would appreciate your feedback there too.

Thanks again for your reviews and patience!
Jc

PS: I've also inlined my answers to Thomas below:

On Tue, Jun 13, 2017 at 8:03 AM, Thomas Schatzl <thomas.scha...@oracle.com 
<mailto:thomas.scha...@oracle.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/~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 ? 
:)

    - 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");

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.


    Seeing the changes in referenceProcess.cpp, you need to add the call to
    HeapMonitoring::do_weak_oops() exactly similar to
    process_weak_jni_handles() in case there is no reference processing
    done.

    - psParallelCompact.cpp:2172 similar to other places where the change
    adds the call to HeapMonitoring::do_weak_oops(), remove the empty line.


Done from what I can tell in the whole webrev. (The only empty lines I still see are when I maintain an empty line, exception being http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/src/share/vm/gc/shared/collectedHeap.cpp.patch where I think it was "nicer")


    - 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 :).


    - referenceProcessor.cpp:40: please make sure that the includes are
    sorted alphabetically (I did not check the other files yet).


Done and checked all other files normally.


    - 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?
    - Additionally, would you prefer it in a separate block with its 
GCTraceTime?


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


    - threadLocalAllocBuffer.hpp:130: extra whitespace ;)


Fixed :)


    - some copyrights :)


I think I fixed all the issues, if you see specific issues, let me know.

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


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


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


    Thanks,
       Thomas


Reply via email to