-
Notifications
You must be signed in to change notification settings - Fork 721
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
DTFJ Javacore enhancements #16142
base: master
Are you sure you want to change the base?
DTFJ Javacore enhancements #16142
Conversation
cade0de
to
d0a62d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of my comments apply throughout:
- use
JAVA_SPEC_VERSION
- missing
@Override
annotations - formatting
- missing or incomplete javadoc
- ...
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaHeap.java
Outdated
Show resolved
Hide resolved
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaHeap.java
Outdated
Show resolved
Hide resolved
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaHeap.java
Outdated
Show resolved
Hide resolved
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaHeap.java
Outdated
Show resolved
Hide resolved
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaRuntime.java
Outdated
Show resolved
Hide resolved
...9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemorySectionParser.java
Show resolved
Hide resolved
...9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemorySectionParser.java
Outdated
Show resolved
Hide resolved
...penj9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemoryTagParser.java
Outdated
Show resolved
Hide resolved
...lasses/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistorySectionParser.java
Outdated
Show resolved
Hide resolved
.../openj9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/title/TitleTagParser.java
Show resolved
Hide resolved
...re/classes/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistoryTagParser.java
Outdated
Show resolved
Hide resolved
...re/classes/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistoryTagParser.java
Outdated
Show resolved
Hide resolved
06aba74
to
cabc6c1
Compare
Some of the dates / times in javacore do not have a time zone, which is a problem if the javacore reader is running in a different time zone from the JVM. The code gets the correct javacore time It also attempts to correct a time from The JVM start time from |
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaHeap.java
Outdated
Show resolved
Hide resolved
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaRuntime.java
Outdated
Show resolved
Hide resolved
...penj9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemoryTagParser.java
Outdated
Show resolved
Hide resolved
cabc6c1
to
ee427c7
Compare
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaHeap.java
Outdated
Show resolved
Hide resolved
ee427c7
to
68b92f8
Compare
Do I need to do any more on this pull request? |
Sorry, I have been busy. I don't expect to get to this until the new year. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the merge conflict (which should just be adopting the new copyright date style).
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaHeap.java
Outdated
Show resolved
Hide resolved
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaHeap.java
Outdated
Show resolved
Hide resolved
...penj9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/common/TraceTimeParser.java
Outdated
Show resolved
Hide resolved
...penj9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/common/TraceTimeParser.java
Outdated
Show resolved
Hide resolved
.../share/classes/com/ibm/dtfj/javacore/parser/j9/section/gchistory/GCHistorySectionParser.java
Outdated
Show resolved
Hide resolved
...9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemorySectionParser.java
Outdated
Show resolved
Hide resolved
...lasses/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistorySectionParser.java
Outdated
Show resolved
Hide resolved
...lasses/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistorySectionParser.java
Outdated
Show resolved
Hide resolved
...lasses/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistorySectionParser.java
Show resolved
Hide resolved
// The name of the history is on a single line. | ||
// 1XECTHTYPE Current thread history (J9VMThread:0x0000000000725A00) | ||
consumeUntilFirstMatch(CommonPatternMatchers.whitespace); | ||
addAllCharactersAsTokenAndConsumeFirstMatch(THREADHISTORY_NAME, CommonPatternMatchers.generateMatcher("\\s*\\(")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please extract CommonPatternMatchers.generateMatcher("\\s*\\(")
to a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using CommonPatternMatchers.open_paren
instead of "\\s*\\("
is not equivalent: the former doesn't match whitespace before the opening parenthesis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consumeUntilFirstMatch will also consume the white space, so CommonPatternMatchers.open_paren is sufficient.
"The pattern itself is also consumed"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the use of consumeUntilFirstMatch()
will consume the spaces before the name, but addAllCharactersAsTokenAndConsumeFirstMatch()
returns the following characters up to, but not including, the part that matches the given pattern. That pattern doesn't match the space(s) before the opening parenthesis which will then be included in THREADHISTORY_NAME
.
8627a5e
to
66ee51f
Compare
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaRuntime.java
Outdated
Show resolved
Hide resolved
jcl/src/openj9.dtfj/share/classes/com/ibm/dtfj/java/javacore/JCJavaRuntime.java
Outdated
Show resolved
Hide resolved
...penj9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/common/TraceTimeParser.java
Outdated
Show resolved
Hide resolved
.../share/classes/com/ibm/dtfj/javacore/parser/j9/section/gchistory/GCHistorySectionParser.java
Outdated
Show resolved
Hide resolved
...share/classes/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/IThreadHistoryTypes.java
Outdated
Show resolved
Hide resolved
...lasses/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistorySectionParser.java
Outdated
Show resolved
Hide resolved
...lasses/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistorySectionParser.java
Outdated
Show resolved
Hide resolved
...re/classes/com/ibm/dtfj/javacore/parser/j9/section/threadhistory/ThreadHistoryTagParser.java
Outdated
Show resolved
Hide resolved
.../openj9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/title/TitleTagParser.java
Show resolved
Hide resolved
...9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemorySectionParser.java
Outdated
Show resolved
Hide resolved
...9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemorySectionParser.java
Outdated
Show resolved
Hide resolved
f2557c7
to
afc82ad
Compare
...9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemorySectionParser.java
Outdated
Show resolved
Hide resolved
* Also uses the times in the thread history to fix the JVM start time | ||
* and the Javacore creation time which have been parsed elsewhere | ||
* with an ambiguous time zone. | ||
* The start time and creation times do not have a time zone, so have been parsed according | ||
* to the parser's time zone, which might differ from the time zone when creating the Javacore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps a better place to do this would be in TitleSectionParser
; this section need not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't do this in TitleSectionParser. The title section has the creation time of the Javacore, but pre issue #12397 it has an unknown timezone, so is parsed in the timezone of the program running DTFJ, which might be different from the timezone of the target process. The title section parser cannot see later sections.
Later in the Javacore we can see some times (GC & thread trace) which are in UTC and presume these are close to, but before the writing of the Javacore. We then use those times to adjust the hours (and minutes in 15 minute steps) of the Javacore time. If the GC & thread trace sections are missing, no correction of the Javacore time can be made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we need to deal with javacores that don't identify the timezone in the title section? If so, the fix here should be conditional so it only applies when the timezone is not in the title section.
afc82ad
to
d58f035
Compare
...9.dtfj/share/classes/com/ibm/dtfj/javacore/parser/j9/section/memory/MemorySectionParser.java
Outdated
Show resolved
Hide resolved
a217129
to
080f6ae
Compare
if (!fCreationTimeUTCSet) | ||
{ | ||
fCreationTime = creationTime; | ||
fCreationTimeSet = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems inappropriate. There's no way to distinguish whether getCreationTime()
returns a value in UTC or some other time zone. It may break the interpretation of other times which are relative to the local time captured in the dump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about trying to cope when the javacore is parsed in a different time zone to when it was created.
DTFJ API: com.ibm.dtfj.image.Image.getCreationTime()
the image creation time in milliseconds since 1970
So this should and does return a UTC based offset, suitable for constructing a Date(). Trying to define it to be anything else would be confusing.
The existing code takes the javacore time (in the JVM time zone), parses it according to the current (DTFJ process) time zone, converts to UTC, and stores the UTC milliseconds. This is okay unless the time zones differ.
The new code takes the 1TIDATETIMEUTC time if available, parses and stores that as UTC. That seems the right thing to do.
The rest of the new code is an attempt to correct the time where there is no 1TIDATETIMEUTC time.
The flag fCreationTimeUTCSet
means the time has been set by 1TIDATETIMEUTC and so is accurate and so shouldn't be modified by the fix-up code calling setCreationTime(). There are other ways this could be done, for example adding a method to com.ibm.dtfj.image.javacore.JCImage, but then the fix-up code would need to cast the com.ibm.dtfj.image.Image to a com.ibm.dtfj.image.javacore.JCImage.
Other times in the javacore:
1STGCHTYPE GC History
3STHSTTYPE 14:54:08:695155215 GMT j9mm.101
This is GMT (UTC) and is used for the creation time correction, but is otherwise not used except for a direct return in a trace buffer.
1XECTHTYPE Current thread history (J9VMThread:0x00000000004E4A00)
3XEHSTTYPE 14:54:16:094112611 GMT j9dmp.9 -
Similarly, this is GMT (UTC) and is used for the creation time correction, but is otherwise not used except for a direct return in a trace buffer.
This doesn't seem useful:
1HKINTERFACE MM_OMRHookInterface
NULL ------------------------------------------------------------------------
2HKEVENTID 1
3HKCALLCOUNT 189
3HKTOTALTIME 1205us
3HKLAST Last Callback
4HKCALLSITE c:\workspace\openjdk-build\workspace\build\src\openj9\runtime\jcl\common\mgmtinit.c:146
4HKSTARTTIME Start Time: 1970-01-16T07:17:51.968
I hadn't considered this one:
1CISTARTTIME JVM start time: 2023/03/02 at 15:12:54:988
1CISTARTNANO JVM start nanotime: 718659523066800
but that code is unchanged and is parsed in the current time zone into UTC and stored and returned as UTC.
That is okay if the javacore time zone is the same as the time zone of the DTFJ process.
Changes suggested for eclipse-openj9#16110 Add UTC fix for creation time. Parse heap memory sections. Add GC trace and thread trace. Fix the JVM start time if there were time zone problems. Signed-off-by: Andrew Johnson <andrew_johnson@uk.ibm.com>
080f6ae
to
edeb1c9
Compare
Changes suggested for #16110
Add UTC fix for creation time.
Parse heap memory sections.
Add GC trace and thread trace
Signed-off-by: Andrew Johnson andrew_johnson@uk.ibm.com