Add support for ZGC logging #216

yanqilee · 2019-01-22T22:40:42Z

Added ZGC GC log in unified JVM logging format to GCViewer. Currently parsing all 3 STW pause events as well as all 7 concurrency events. These info are listed in [gc,phases] so we had to include that for parsing in ZGC. Memory info are displayed separately, the pre and post used heap sizes are only displayed every GC cycle, not at every pause. For total heap, we parsed the capacity block in [gc,heap]. Currently we are only parsing the capacity block, but there are information in the [gc,heap] section that could be useful in future improvements.

I had to add a line in the pom file on javadoc plugin because travis build is failing, it has recently started running JDK 11.0.2 (only that version fails, 11.0.1 works fine)
Also did some refactoring around the classes I have touched.

…macmms/GCViewer into feature/protoZGC

codecov-io · 2019-01-22T22:43:36Z

Codecov Report

Merging #216 into develop will increase coverage by 0.23%.
The diff coverage is 92.67%.

@@              Coverage Diff              @@
##             develop     #216      +/-   ##
=============================================
+ Coverage      69.94%   70.18%   +0.23%     
- Complexity      1479     1512      +33     
=============================================
  Files            145      145              
  Lines           8396     8500     +104     
  Branches        1374     1385      +11     
=============================================
+ Hits            5873     5966      +93     
- Misses          1948     1953       +5     
- Partials         575      581       +6

Impacted Files	Coverage Δ	Complexity Δ
...a/com/tagtraum/perf/gcviewer/model/GCEventUJL.java	`77.77% <100%> (+17.77%)`	`6 <2> (+2)`	⬆️
...om/tagtraum/perf/gcviewer/imp/DataReaderTools.java	`88.88% <100%> (ø)`	`15 <0> (ø)`	⬇️
.../tagtraum/perf/gcviewer/model/AbstractGCEvent.java	`85.11% <100%> (+1.02%)`	`74 <5> (+5)`	⬆️
...perf/gcviewer/imp/DataReaderUnifiedJvmLogging.java	`80.73% <85.29%> (+0.26%)`	`62 <14> (+15)`	⬆️
...tagtraum/perf/gcviewer/view/ModelDetailsPanel.java	`94.59% <85.71%> (-0.35%)`	`5 <0> (+1)`
...java/com/tagtraum/perf/gcviewer/model/GCModel.java	`86.58% <96.34%> (+0.72%)`	`139 <21> (+9)`	⬆️
...va/com/tagtraum/perf/gcviewer/view/GCDocument.java	`33.97% <0%> (-0.97%)`	`23% <0%> (ø)`
...agtraum/perf/gcviewer/model/ConcurrentGCEvent.java	`48.71% <0%> (+5.12%)`	`9% <0%> (+1%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99ceac8...4793b3d. Read the comment docs.

Marysunithajoseph

Will do one more sweep to check and see if we've added something ZGC specific

src/main/java/com/tagtraum/perf/gcviewer/imp/DataReaderUnifiedJvmLogging.java

src/test/java/com/tagtraum/perf/gcviewer/imp/TestDataReaderUJLZGC.java

Marysunithajoseph

LGTM!

yanqilee · 2019-01-24T15:32:46Z

Tried a couple things on a different branch, seems like the code need a major refactor to pass the Better Code Hub check.

@chewiebug please look at the PR when you have a chance and let me know of any concerns. Thanks!

chewiebug

In general: Thank you very much for the pull request! I looks quite good already! Don't bother too much about the failing "Better Code Hub" check. I am not sure, if those checks are really a good match for GCViewer. I like the concept, but it might be too much for a grown code base like GCViewer to introduce at this stage.

I'll currently set the status to "request changes". My main concern is to understand to right model. For that, please see my comment / questions in the issue. After these questions have good answers, the necessary changes in the code will be more clear.

chewiebug · 2019-02-13T14:55:37Z

src/main/java/com/tagtraum/perf/gcviewer/imp/DataReaderUnifiedJvmLogging.java

-
-
+    /** Pattern - for ZGC phases - that are to be included for parsing */
+    private static final Pattern PATTERN_INCLUDE_STRINGS_PHASE = Pattern.compile("\\[(gc,phases[ ]*)][ ]GC\\(([0-9]+)\\)[ ](?<type>[-a-zA-Z1 ()]+)(([ ]([0-9]{1}.*))|$)");


Could this pattern not be made part of the INCLUDE_STRINGS and later, the "gc,phases" lines could be treated as AbstractGCEvent.GcPattern.GC_PAUSE? Looking at the included sample log, I am not sure, if this needs a new regex.

See my comments below on parsing gc,phases.
It is a solution to the problem but personally I don't like having something this ZGC specific in there. Any suggestions to improve this?

chewiebug · 2019-02-13T14:56:15Z

src/main/java/com/tagtraum/perf/gcviewer/imp/DataReaderUnifiedJvmLogging.java

@@ -140,7 +174,7 @@ public GCModel read() throws IOException {
        getLogger().info("Reading Oracle / OpenJDK unified jvm logging format...");

        try {
-            // some information shared accross several lines of parsing...
+            // some information shared across several lines of parsing...


Thanks :-)!

chewiebug · 2019-02-13T14:57:03Z

src/main/java/com/tagtraum/perf/gcviewer/imp/DataReaderUnifiedJvmLogging.java

@@ -185,46 +218,84 @@ private ParseContext parseEvent(ParseContext context) {
        AbstractGCEvent<?> returnEvent = event;
        switch (tags) {


looks nice to have this switch shortened and the code moved to separate methods!

chewiebug · 2019-02-13T14:59:35Z

src/main/java/com/tagtraum/perf/gcviewer/imp/DataReaderUnifiedJvmLogging.java

@@ -401,6 +513,17 @@ private boolean lineContainsParseableEvent(ParseContext context) {
        return false;
    }

+    private boolean isParseablePhaseEvent(String line) {


Are there also "gc,phases" events, that are not parseable?

So what was happening is that [gc,phases] events were not included at all previously, when I added [gc,phases] to INCLUDE_STRINGS, it broke many existing test cases because there are [gc,phases] lines in the test files that were ignored and the test cases asserts 0 warnings. When those lines got included, the assertions failed. So this is a solution to only include ZGC [gc,phases] to not break the build and/or affect existing functionalities.

chewiebug · 2019-02-13T15:01:49Z

src/main/java/com/tagtraum/perf/gcviewer/view/ModelDetailsPanel.java

@@ -38,6 +38,10 @@
    private DoubleDataMapModel concurrentGcEventModel;

    private DoubleDataMapTable vmOperationTable;
+
+    /** GC Causes Model and table */
+    private DoubleDataMapModel gcCausesModel;


Please see my comment in the issue. I don't understand the concept of the "causes", yet and am not sure, if this is the right way to go.

I have commented on the issue, will likely scrap causes section all together, please let me know what you think.

chewiebug · 2019-02-13T15:09:51Z

src/main/java/com/tagtraum/perf/gcviewer/model/AbstractGCEvent.java

@@ -24,6 +24,10 @@
    private int preUsed;
    /** Used after GC in KB */
    private int postUsed;
+    /** Percentage used before GC */
+    private int preUsedPercent;


Is it really worthwhile to store this information? Can't it be calculated, if needed?

You are right, I don't think it's worthwhile to have this, I will remove this.

chewiebug · 2019-02-13T15:13:23Z

src/main/java/com/tagtraum/perf/gcviewer/imp/DataReaderTools.java

-            Matcher parenMatcher = parenthesesPattern.matcher(lookupTypeName);
-            if (parenMatcher.find()) {
-                gcType = AbstractGCEvent.Type.lookup(parenMatcher.replaceFirst(""));
+            // try to parse it again with the parents removed


Is "parents" an established abbrevation for "parenthesis"? If not, I'd prefer the renaming to go to "parenthesisMatcher" to avoid confusion.

Good catch! I think we got confused too while renaming this, will fix this.

chewiebug · 2019-02-13T15:17:35Z

src/main/java/com/tagtraum/perf/gcviewer/view/ModelDetailsPanel.java

+            remove(gcCausesTable);
+        }
+        else {
+        	gcCausesModel.setModel(model.getGcEventCauses(), totalPause, false);


Since, currently, there is no pause time in the causes, this line might cause the weird numbers. But first, let's clarify on the right model (see my comment in the issue).

chewiebug · 2019-02-24T16:51:31Z

pom.xml

@@ -327,6 +327,7 @@
                        <links>
                            <link>http://docs.oracle.com/javase/8/docs/api/</link>
                        </links>
+                        <source>${jdk.source.version}</source>


Ah, this fixes the build failure on the develop branch - thank you very much!

yanqilee · 2019-03-04T22:16:07Z

Hi @chewiebug,

I have made the suggested changes, please have a look.
Attached are the screenshots for the various panels after parsing ZGC log.

chewiebug · 2019-03-05T21:26:38Z

Hi @yanquilee Thank you very much for updating the pull request! At first glance it looks good. Nice chart and event detail information! I'll probably have time to to have a closer look in about two weeks and hopefully merge your pull request. I see, that the phases contain stop-the-world parts and concurrent parts - as this is the concept of ZGC. GCViewer is not aware of the concurrent parts like this. Let me think about it. Best regards, Jörg

yanqilee · 2019-03-05T21:46:07Z

Thanks @chewiebug!

Looking forward to your comments.

chewiebug · 2019-03-18T20:40:19Z

Hi @yanquilee, I am currently experimenting with moving the concurrent phase events to the concurrent events in the GCModel and adding gc,phases support for the other gc algorithms. I think, I'll merge your pull request soon. Best regards, Jörg

… Shenandoah collectors

chewiebug · 2019-06-10T16:41:44Z

Hi @yanqilee

I have finally merged your pull request after adding support for parsing phase events for the other gc algorithms. Thanks again for your contribution! By the way: I have added your name to the list of contributors (see in the "AboutDialog": Help -> About GCViewer).

Best regards,
Jörg

yanqilee · 2019-06-10T17:43:21Z

Woohoo! Awesome! Thanks @chewiebug!
Please do also add @Marysunithajoseph to the list of contributors as she helped with the development of this feature.

Cheers,
Yanqi

chewiebug · 2019-06-11T19:42:32Z

:-) @Marysunithajoseph: I am very sorry for having missed you in the first try. You are now also part of the list of contributors. @yanquilee: Thank you very much for the hint!

Marysunithajoseph · 2019-06-12T01:12:49Z

@chewiebug @yanqilee Thank you, it was great to have worked on this!

…S, G1 and Shenandoah collectors

Marysunithajoseph and others added 17 commits January 12, 2019 19:47

Include GC Phases from ZGC for parsing

81dcfcf

Corrected phase name

dda185a

Parsing Phases for ZGC

9643bd8

Removed unnecessary print outs

84205d6

Added test cases for default and memory matchers

1a05f67

Added tests for gc all logging

d7e2405

fixed formatting

66e9c1b

Fixed a typo

75f83bb

Seperating GC Causes and its memory info

1256031

Merge branch 'feature/fullstackZGCintegration' of https://github.com/…

238050f

…macmms/GCViewer into feature/protoZGC

Gc causes no longer show total sum percentage

9b7aad5

Fixed up gc causes model

8b00e90

Fixed some formatting

b3fc7f6

Fixed up comment for build

5a9b562

Fixed maven javadoc plugin failing on jdk 11.0.2

1f681bd

initial code refactor

1fdb0a0

refactored addGcEvent

52b4111

yanqilee mentioned this pull request Jan 22, 2019

Support for ZGC on jdk11 #211

Open

yanqilee added 4 commits January 22, 2019 17:47

Remove unused imports

ba10ced

Fixed Codacy issues

03284fc

No longer alter event parameter in method

70598e9

Added missed null check on isParseablePhaseEvent

2576e5d

Marysunithajoseph reviewed Jan 23, 2019

View reviewed changes

src/main/java/com/tagtraum/perf/gcviewer/imp/DataReaderUnifiedJvmLogging.java Outdated Show resolved Hide resolved

src/test/java/com/tagtraum/perf/gcviewer/imp/TestDataReaderUJLZGC.java Outdated Show resolved Hide resolved

Refactored tests and updated method params

18d4275

Marysunithajoseph approved these changes Jan 23, 2019

View reviewed changes

yanqilee added 2 commits January 23, 2019 14:32

update method naming

8925464

Added percentage assertions to test cases

d29aa3c

chewiebug requested changes Feb 13, 2019

View reviewed changes

chewiebug reviewed Feb 24, 2019

View reviewed changes

yanqilee added 10 commits March 1, 2019 10:24

Removed ZGC phase parsing rule and commented out warning assertion

2cb14fb

Fixed parentheses variable names

75c7f6e

Removed memory capture and added phases start exclusion

e8ea44e

Fixed comments and code style

ad1ca4b

Added notion of phases in AbstractGcEvent

0eb52d4

Changed test names to be more general

53355b4

Added phases to parser, model and UI

3ecd294

Minor changes for styles, etc

ac4f08b

Single line import on TestGCEventUJL

907a3be

Made GCEventUJL private in test class

4793b3d

chewiebug merged commit 4793b3d into chewiebug:develop Jun 10, 2019

chewiebug added a commit that referenced this pull request Jun 10, 2019

#216 also support parsing of phases for Serial, Parallel, CMS, G1 and…

f893bbd

… Shenandoah collectors

chewiebug added a commit that referenced this pull request Jun 10, 2019

#216 add Yanqi Li to the list of contributors

559eeb8

sparrowzoo pushed a commit to sparrowzoo/GCViewer that referenced this pull request Dec 28, 2020

chewiebug#216 also support parsing of phases for Serial, Parallel, CM…

2a2535c

…S, G1 and Shenandoah collectors

sparrowzoo pushed a commit to sparrowzoo/GCViewer that referenced this pull request Dec 28, 2020

chewiebug#216 add Yanqi Li to the list of contributors

515dfbe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ZGC logging #216

Add support for ZGC logging #216

yanqilee commented Jan 22, 2019

codecov-io commented Jan 22, 2019 •

edited

Loading

Marysunithajoseph left a comment

Marysunithajoseph left a comment

yanqilee commented Jan 24, 2019

chewiebug left a comment

chewiebug Feb 13, 2019

yanqilee Feb 18, 2019

chewiebug Feb 13, 2019

chewiebug Feb 13, 2019

chewiebug Feb 13, 2019

yanqilee Feb 18, 2019

chewiebug Feb 13, 2019

yanqilee Feb 18, 2019

chewiebug Feb 13, 2019

yanqilee Feb 18, 2019

chewiebug Feb 13, 2019

yanqilee Feb 18, 2019

chewiebug Feb 13, 2019

chewiebug Feb 24, 2019

yanqilee commented Mar 4, 2019

chewiebug commented Mar 5, 2019 via email

yanqilee commented Mar 5, 2019

chewiebug commented Mar 18, 2019 via email

chewiebug commented Jun 10, 2019

yanqilee commented Jun 10, 2019

chewiebug commented Jun 11, 2019 via email

Marysunithajoseph commented Jun 12, 2019



		/** Pattern - for ZGC phases - that are to be included for parsing */
		private static final Pattern PATTERN_INCLUDE_STRINGS_PHASE = Pattern.compile("\\[(gc,phases[ ])][ ]GC\\(([0-9]+)\\)[ ](?<type>[-a-zA-Z1 ()]+)(([ ]([0-9]{1}.))\|$)");

		@@ -185,46 +218,84 @@ private ParseContext parseEvent(ParseContext context) {
		AbstractGCEvent<?> returnEvent = event;
		switch (tags) {

Add support for ZGC logging #216

Add support for ZGC logging #216

Conversation

yanqilee commented Jan 22, 2019

codecov-io commented Jan 22, 2019 • edited Loading

Codecov Report

Marysunithajoseph left a comment

Choose a reason for hiding this comment

Marysunithajoseph left a comment

Choose a reason for hiding this comment

yanqilee commented Jan 24, 2019

chewiebug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanqilee commented Mar 4, 2019

chewiebug commented Mar 5, 2019 via email

yanqilee commented Mar 5, 2019

chewiebug commented Mar 18, 2019 via email

chewiebug commented Jun 10, 2019

yanqilee commented Jun 10, 2019

chewiebug commented Jun 11, 2019 via email

Marysunithajoseph commented Jun 12, 2019

codecov-io commented Jan 22, 2019 •

edited

Loading