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.

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

Fix for negative promotion calculation #237

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

Conversation

theobisproject
Copy link

Fixes #235

@codecov-io
Copy link

codecov-io commented Dec 8, 2019

Codecov Report

Merging #237 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #237      +/-   ##
=============================================
+ Coverage      70.43%   70.46%   +0.02%     
- Complexity      1518     1519       +1     
=============================================
  Files            145      145              
  Lines           8596     8597       +1     
  Branches        1389     1390       +1     
=============================================
+ Hits            6055     6058       +3     
+ Misses          1961     1958       -3     
- Partials         580      581       +1
Impacted Files Coverage Δ Complexity Δ
...java/com/tagtraum/perf/gcviewer/model/GCModel.java 86.37% <100%> (+0.03%) 139 <0> (+1) ⬆️
...gtraum/perf/gcviewer/ctrl/impl/GcSeriesLoader.java 83.14% <0%> (+2.24%) 20% <0%> (ø) ⬇️

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 b583416...f20ebd2. Read the comment docs.

- (event.getPreUsed() - event.getPostUsed())
);
int promo = (youngEvent.getPreUsed() - youngEvent.getPostUsed()) - (event.getPreUsed() - event.getPostUsed());
if (promo >= 0) {

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.

"[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

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Total promotion can get negative
3 participants