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

Improve Archive Region Calculation (#211) #369

Merged
merged 6 commits into from
Aug 6, 2024

Conversation

kkdlau
Copy link
Contributor

@kkdlau kkdlau commented Jul 15, 2024

Previously, GCToolkit recognized archive regions but did not use this data in its calculations. This change improves the accuracy of tenured generation metrics for G1GCPause events.

@karianna karianna requested review from kcpeppe and dsgrieve July 18, 2024 02:26
Copy link
Member

@karianna karianna left a comment

Choose a reason for hiding this comment

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

Syntactic changes LGTM - @kcpeppe or @dsgrieve to confirm semantic changes. I'm also curious as to whether this change should have triggered a test failure or if we're missing coverage.

@kkdlau
Copy link
Contributor Author

kkdlau commented Jul 23, 2024

Syntactic changes LGTM - @kcpeppe or @dsgrieve to confirm semantic changes. I'm also curious as to whether this change should have triggered a test failure or if we're missing coverage.

I can implement some test cases for this PR

@dsgrieve
Copy link
Member

@kcpeppe and I had some discussion about this. We want to verify whether archive regions are counted as old gen regions which may mean that these regions are being accounted for twice.

@kkdlau
Copy link
Contributor Author

kkdlau commented Aug 2, 2024

@kcpeppe and I had some discussion about this. We want to verify whether archive regions are counted as old gen regions which may mean that these regions are being accounted for twice.

Hi, just checked with the parse rules:

GCParseRule REGION_SUMMARY = new GCParseRule("REGION_SUMMARY", "(Eden|Survivor|Old|Humongous|Archive) regions: " + REGION_MEMORY_BLOCK);

And the way of how the tool kit processes the log:

public void regionSummary(GCLogTrace trace, String line) {
RegionSummary summary = trace.regionSummary();
switch (trace.getGroup(1)) {
case "Eden":
forwardReference.setEdenRegionSummary(summary);
break;
case "Survivor":
forwardReference.setSurvivorRegionSummary(summary);
break;
case "Old":
forwardReference.setOldRegionSummary(summary);
break;
case "Humongous":
forwardReference.setHumongousRegionSummary(summary);
break;
case "Archive":
forwardReference.setArchiveRegionSummary(summary);
break;
default:
notYetImplemented(trace, line);
}
}

it seems it will differentitate the type of the region, and set the region summary accordingly

@dsgrieve
Copy link
Member

dsgrieve commented Aug 2, 2024

@kkdlau - I found "OpenArchive" and "CloseArchive" as region types in jdk-11 and jdk-17. But I don't see an archive region type in jdk-8 or jdk-21. The only evidence I see of "Archive" appearing in a log trace is in the remembered set stats, but only in jdk-17.

Do you have a log file with the pattern "Archive regions:"?

@kkdlau
Copy link
Contributor Author

kkdlau commented Aug 2, 2024

@dsgrieve one of examples I found from <repo root>/gclogs/rolling/jdk14/rollinglogs/short_restart_4.log.1 line 106:

[15.034s][info ][gc,heap ] GC(0) Archive regions: 2->2

But it is quite surprising to know that the way JDK handles the archive region is changing over generations. I will also look at OpenJDK's G1CollectedHeap implementation to understand more about the issue.

@kkdlau
Copy link
Contributor Author

kkdlau commented Aug 3, 2024

@dsgrieve I just walked through OpenJDK and found that Archive Regions are only available in JDK 14 and 17.

JDK 14: https://github.com/openjdk/jdk/blob/ae39310243b0486f5a6f1049c6ec5f29db31170c/src/hotspot/share/gc/g1/g1HeapTransition.cpp#L174-L177
JDK 17: https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/hotspot/share/gc/g1/g1HeapTransition.cpp#L175-L178

  log_info(gc, heap)("Archive regions: " SIZE_FORMAT "->" SIZE_FORMAT,
                     _before._archive_length, after._archive_length);
  log_trace(gc, heap)(" Used: " SIZE_FORMAT "K, Waste: " SIZE_FORMAT "K",
      usage._archive_used / K, ((after._archive_length * HeapRegion::GrainBytes) - usage._archive_used) / K);

JDK 8 doesn't have Archive Region type:
https://github.com/openjdk/jdk/blob/9a9add8825a040565051a09010b29b099c2e7d49/hotspot/src/share/vm/gc_implementation/g1/g1HRPrinter.hpp#L48-L56

  typedef enum {
    Unset,
    Eden,
    Survivor,
    Old,
    SingleHumongous,
    StartsHumongous,
    ContinuesHumongous
  } RegionType;

When I look at how the JVM collects region info, archive regions are not counted as old regions:
https://github.com/openjdk/jdk/blob/dfacda488bfbe2e11e8d607a6d08527710286982/src/hotspot/share/gc/g1/g1HeapTransition.cpp#L89-L94

    if (r->is_old()) {
      _usage._old_used += r->used();
      _usage._old_region_count++;
    } else if (r->is_archive()) {
      _usage._archive_used += r->used();
      _usage._archive_region_count++;

I think, even though archive regions are only available in JDK 14 and 17, we can still subtract the size of archive regions from the total. Since the size of archive regions is 0 in other versions, and there is no double counting for old regions and archive regions.

Copy link
Member

@karianna karianna left a comment

Choose a reason for hiding this comment

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

Can you add a comments in the code at the appropriate place covering the versions of the JDK that archive regions are supported in (gclog wise).

@kcpeppe & @dsgrieve - longer tern should we consider tagging rules / patterns with hte java versions that support them (even point releases).

@kkdlau kkdlau requested a review from karianna August 6, 2024 07:54
@karianna karianna merged commit aaeb875 into microsoft:main Aug 6, 2024
8 checks passed
@dsgrieve dsgrieve linked an issue Aug 8, 2024 that may be closed by this pull request
@karianna karianna mentioned this pull request Oct 1, 2024
@karianna karianna mentioned this pull request Dec 8, 2024
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.

G1GC Archive regions
3 participants