Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix for negative promotion calculation #237

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix for negative promotion calculation #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/main/java/com/tagtraum/perf/gcviewer/model/GCModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,9 +651,10 @@ private void updatePromotion(GCEvent event) {
}

if (youngEvent != null) {
promotion.add((youngEvent.getPreUsed() - youngEvent.getPostUsed())
- (event.getPreUsed() - event.getPostUsed())
);
int promo = (youngEvent.getPreUsed() - youngEvent.getPostUsed()) - (event.getPreUsed() - event.getPostUsed());
if (promo >= 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably only hide the error and still not produce correct promotion numbers.

promotion.add(promo);
}
}
}
}
Expand Down
28 changes: 28 additions & 0 deletions src/test/java/com/tagtraum/perf/gcviewer/imp/TestDataReaderUJLG1JDK11.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,4 +180,32 @@ public void testPauseYoungConcurrentStartMetadataGcThreshold() throws Exception
assertThat("event type", model.get(0).getExtendedType().getType(), is(Type.UJL_PAUSE_YOUNG));
assertThat("total heap", model.get(0).getTotal(), is(256 * 1024));
}

@Test
public void testPauseNoPromotion() throws IOException {
TestLogHandler handler = new TestLogHandler();
handler.setLevel(Level.WARNING);
GCResource gcResource = new GcResourceFile("byteArray");
gcResource.getLogger().addHandler(handler);
InputStream in = new ByteArrayInputStream(
("[12.029s][info][gc,start ] GC(9) Pause Young (Normal) (G1 Evacuation Pause)\n" +
"[12.029s][info][gc,task ] GC(9) Using 8 workers of 8 for evacuation\n" +
"[12.034s][info][gc,phases ] GC(9) Pre Evacuate Collection Set: 0.0ms\n" +
"[12.034s][info][gc,phases ] GC(9) Evacuate Collection Set: 4.4ms\n" +
"[12.034s][info][gc,phases ] GC(9) Post Evacuate Collection Set: 0.5ms\n" +
"[12.034s][info][gc,phases ] GC(9) Other: 0.2ms\n" +
"[12.034s][info][gc,heap ] GC(9) Eden regions: 143->0(142)\n" +
Copy link

@cmoetzing cmoetzing Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your test data is not correct. At least the parser does not read it correctly. If you add following lines top your test you will see that pre used of young is 0 but should be 143M.

assertThat("pre", model.get(0).getPreUsed(), is(162*1024)); // pass
assertThat("pre", model.get(0).getPostUsed(), is(19*1024)); // pass

AbstractGCEvent<?> event = model.get(0).getPhases().get(0);
assertThat("young pre", event.getGeneration(), is(AbstractGCEvent.Generation.YOUNG)); // pass
assertThat("young pre", event.getPreUsed(), is(143*1024)); // fail
assertThat("young pre", event.getPostUsed(), is(0L)); // pass

Your example would not produce the desired error if parsed correctly:

int young = 143 - 0; // 143
int all = 162 - 19; // 143
int promotion = young - all; // 0

The format in this test differs from the format used in your issue #235 that produces the error in "real life".

Edit: The formats are the same. The test does still not correctly parse the entry. Maybe it is missing some header like [0.011s][info][gc,heap] Heap region size: 1M?

"[12.034s][info][gc,heap ] GC(9) Survivor regions: 10->11(20)\n" +
"[12.034s][info][gc,heap ] GC(9) Old regions: 8->8\n" +
"[12.034s][info][gc,heap ] GC(9) Humongous regions: 2->2\n" +
"[12.034s][info][gc,metaspace ] GC(9) Metaspace: 40737K->40737K(1085440K)\n" +
"[12.034s][info][gc ] GC(9) Pause Young (Normal) (G1 Evacuation Pause) 162M->19M(256M) 5.351ms\n" +
"[12.034s][info][gc,cpu ] GC(9) User=0.00s Sys=0.00s Real=0.01s")
.getBytes());

DataReader reader = new DataReaderUnifiedJvmLogging(gcResource, in);
GCModel model = reader.read();

assertThat("promotion", model.getPromotion().getSum(), is(0L));
}
}