-
Notifications
You must be signed in to change notification settings - Fork 971
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for -XX:+PrintGCCause (will be on by default in Java 8) #66
Comments
Hi Ryan I'll have to look into this feature. Do you have a sample log file Regards, Jörg |
Here is one where it has the things squashed in along with G1 and the adaptive size policy
|
I've personally also seen a few lines where it has the cause "(G1 Humongous Allocation)" instead of "(G1 Evacuation Pause)" in theory, there could be a number of other options for each of the given types but I would have to walk the source a lot to get the complete list of possible combinations |
Hi Ryan, Thank you for the sample. I have generated a few of my own to get a better impression on the possible changes the new flag introduces. The straight forward place to extend is AbstractGCEvent and add the new Type constants. Probably not the best idea because a lot of new events might be introduced that look very much like duplicates of existing ones. Probably the best place to introduce this change is AbstractDataReaderSun (Line 369). If Should that assumption be wrong, the next best place would probably be to extend AbstractGCEvent.Type to support more than one representation of an event and register all representations with the internal TYPE_MAP. Then the Type#parse method would still be very fast looking up the correct event. It should be well checked in this solution that What do you think about this approach? Regards, Jörg |
I believe the assumption about -XX:+PrintGCCause is correct for Java 7 and 8 The assumption that you have regarding the PrintGCCause always outputting the cause the way it does now I believe is valid. I think there is one more assumption that could help simplify things. If PrintGCCause is enabled, it should always have a GCCause line - when parsing the first GC log line if it detects that there is a GC Cause field in the log line (this could be detected by trying to parse the event, and if it gets back null have it try to parse the event with the first set of parentheses removed - if that returns something valid, assume that it is using The approach of having it pick which case is the default to try first is just an optimization on the approach you described - we could try that first and then add in an optimization around it later. Keeping the full text of the cause (making the type mutable) would be valuable - it would help retain that GCCause information and make it easy to see if a certain type of GC has a different duration based on what causes it. |
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7166894 Actually you can see here a discussion which gives some backgrounds: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2012-May/thread.html And here the commited changeset for JDK8, seems to be the same (but default off) for JDK7: http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/9d679effd28c |
@ecki: Thanks for the links, they are very good input! @ryangardner: Your optimisation to my approach sounds valid. I'd first Regards, Jörg |
I have done some preliminary work on this - it's up on a branch - but it isn't done yet. Making the "type" string field immutable brings with it another problem: all of the Type objects are reused. I realized this when I got it working for G1 and for CMS tests with / without the GCCause, but I saw a bunch of unrelated test failures such as:
For a test that had nothing to do the PrintGCCause testing - mutating that field will change it for all of the instances that re-use it. To handle parsing logs that have PrintGCCause in them and simply ignoring the PrintGCCause strings - this solution is simple, just remove the line that sets it and it would be parsed as the correct type. I assume that the reason for using immutable instances of the Type was to avoid a new instance of the Type object every time a GC log line is parsed... |
I just updated the branch so it is useable (it doesn't track the GCCause at the moment, but it will parse a file that has it in there and ignore it) |
I haven't thought of the immutable Type objects beeing reused, but it Maybe we need to add the name of the event to the AbstractGCEvent class |
Hi Ryan, May I pull in what you committed to your branch even if it doesn't cover all features we discussed? I think the implementation as it is is far better than nothing. Regards, Jörg |
Sure. Want me to send a pull request or do you just want to pull it Ryan
|
Hi Ryan, Thanks! I'll just pull it in directly within the next few days. Regards, Jörg |
I had to play around with some stuff today anyway, so I fixed a few more
|
Hi Ryan, I have merged your pull request #77. Now "only" the cause is missing. I'll leave this issue open until we have the cause displayed. Thank you very much for your contribution! Regards, Jörg |
Hi Ryan, I have pushed a commit to show the cause. Does this look like you expect it to? Regards, Jörg |
Hi Ryan, I close this issue, since it works for me. If it doesn't for you, please let me hear what you are missing. Regards, Jörg |
In java 8, this will be on by default. In the next non-security Java 7 release, it is going to be available via the -XX:+PrintGCCause flag (it's currently available in ea build)
The AbstractGCEvent.parse method currently barfs when hit with a GC log message that comes out of something with the PrintGCCause information in it:
the log entries look like:
"GC pause (G1 Evacuation Pause) (young)"
This will not be just for G1, this additional detail will be provided for all collectors - it makes the parsing in AbstractGCEvent.parse a bit trickier because it will have to know that
"GC pause (G1 Evacuation Pause) (young)"
"GC pause (G1 Humongous Allocation) (young)"
both map to:
"GC pause (young)"
Looking at the JDK7 / JDK8 sources - the constants are defined in the gcCause.cpp file:
Java 7 has
instead of "Metadata GC Threshold"
I can help with a patch if you have an idea of where this might be easiest to wedge in
The text was updated successfully, but these errors were encountered: