Registering new ZGC classes

[jlittle-ptc]

Starting to look at the ZGC update in GCToolkit 3.5.0 and have a question:

Is there a specific reason that the new ZGC classes (Full/Major/MinorZGCCycle) don't inherit from a common "ZGC" root class like the other collectors?

Ultimately it means I have to register a method for each event type specifically which isn't a big deal. It just seems that it's a slight deviation from the way the other Event classes are defined.

Also, the hierarchy chart in DEVELOPERS_GUIDE should be updated to account for the new ZGC classes.

[dsgrieve]

@j-bahr contributed the changes and can answer the question authoritatively.

There could be a ZGCEvent that hoists the ZGCMemorySummary and GC id.

[j-bahr]

@jlittle-ptc

The ZGC collector supports legacy ZGC and Generational ZGC. Legacy is going away in newer versions of Java but I made an effort to leave support for it here. The structure of the events between versions are largely the same except that generational now has multiple phases, young only, young and old. The cycle data is largely the same between legacy (full gc) and the phases young and old. To accommodate this with the least amount of effort we have the structure shown below, FullZGCCycle, MajorZGCCycle and MinorZGCCycle which all extend from GCEvent. ZGCCycle is just an internal data class that holds the information from the cycle whether it's full, young or old generational data.

┌─────────────┐
│    Full     │
│             │      ┌────────┐
│  delegate ──┼─────▶│ZGCCycle│
│             │      └────────┘
└─────────────┘
┌─────────────┐
│    Major    │
│             │      ┌────────┐
│    young ───┼─────▶│ZGCCycle│
│             │      ├────────┤
│     old  ───┼─────▶│ZGCCycle│
└─────────────┘      └────────┘
┌─────────────┐
│    Minor    │
│             │     ┌────────┐
│    young ───┼────▶│ZGCCycle│
│             │     └────────┘
└─────────────┘

If you're interested in registering for a single event type you'll need to check instanceof on the GCEvent object, otherwise you'll need to register for all events.

Register for single event, but must use instanceof

  public ZgcAggregator(ZgcAggregation aggregation) {
    super(aggregation);
    register(GCEvent.class, this::processEvent);
  }

Registering for all events:

  public ZgcAggregator(ZgcAggregation aggregation) {
    super(aggregation);
    register(MinorZGCCycle.class, this::processMinorCycle);
    register(MajorZGCCycle.class, this::processMajorCycle);
    register(FullZGCCycle.class, this::processFullCycle);
  }

[kcpeppe]

I've been going back and forth on the idea of events representing a whole GC cycle or having it represent major phases of the GC cycle with the ultimate level of granularity being a young or full collection. This is the approach taken with G1 and CMS. For the first implementation of the parser for ZGC I decided to try to have 1 event cover an entire GCID. This was before generational ZGC was even considered. As genZ was being developed I was weighing the option of following the G1 model of a different event for each phase but instead was aiming for a single event for each GCID. Obviously this comes with some tradeoffs. That said I think we can inject a superclass that would allow for a single event registration. Maybe I'm wrong, but I don't think there is a clean API that would make the single registration useful. IOWs, I believe that @j-bahr has created a very reasonable separation that makes sense in the context of how ZGC works. And I really appreciate his detailed explanation of this thinking. If you believe there is a refactoring that could make his work even more useful then I'm very happy to discuss. I myself am planning on making some small adjustments to the patch to address a couple of small usability issues that have revealed themselves as I've been altering tooling to migrate from the older ZGC event to these new and much improved ones.

[jlittle-ptc]

Thanks @kcpeppe @j-bahr for your responses.

I've got no issues with the model @j-bahr has built, I just wanted to make sure it was intentional and not overlooked. Generational and G1 have "collector type" events (GenerationalGCEvent and G1GCEvent respectively) but the ZGC events extended from GCEvent.

In truth, given the difference in data potentially available with each event type, it does make sense to deal with them on a Major/Minor/Full basis instead of as a collective group.

Thanks everyone!

[kcpeppe]

@jlittle-ptc, yeah, short answer is, @j-bahr was following what I'd started and that was intentional. And yeah, the semantic difference is enough that one would want to manage them differently.