Should ConcurrentModeFailure and ConcurrentModeInterrupted implement CMSPhase ?
loyispa opened this issue · 8 comments
Seems they are all related to the CMS garbage collector?
ConcurrentModeFailure and ConcurrentModeInterrupted are both full collections. These signal a failure of the CMS cycle to complete before an allocation was experienced so I haven't considered them to be part of the CMSPhase. @loyispa, is there a reason to mark them as a CMSPhase?
@kcpeppe It's not a necessary reason, i used to think they were part of CMS, and just want to quickly classify CMS events from GenerationalGCEvent. Perhaps this is not reasonable :)
I like your reasoning and these are CMS events (even though they are Full GCs) so unless @dsgrieve or someone else can point out harm, I'd say let us mark them as a CMSPhase.
@kcpeppe I'm inclined to disagree. When I think "CMS Phase" I think "concurrent phase", not a pause event. But then there is public abstract class CMSPauseEvent extends GenerationalGCPauseEvent implements CMSPhase which runs counter to my argument. I'll just call that broken. :)
Wouldn't you also want a way to classify events for other generational collectors? Why not an interface CMSEvent that can be used to mark events that are CMS events, along with a Parallel/G1GC/Serial/ZGC/Event? And maybe there needs to be an interface for PauseEvent (with API for getting timestamp, gc type, gc cause, and duration) and an interface for ConcurrentEvent (with API for getting timestamp, gc type, gc cause, duration, cpu time and wallclock time). JVMEvent should be an interface and all these other event 'marker' interface would extend the JVMEvent interface.
I think this issue is symptomatic of a larger problem with the event hierarchy.
"I'm inclined to disagree. When I think "CMS Phase" I think "concurrent phase", not a pause event. But then there is public abstract class CMSPauseEvent extends GenerationalGCPauseEvent implements CMSPhase which runs counter to my argument. I'll just call that broken. :)"
Let's break this down...
-
A normal CMS cycle contains 6 phases, an initial mark (pause), a concurrent mark, a concurrent preclean (with an optional abortable preclean), a remark (pause), concurrent sweep, and finally a reset (no pause). Additionally, there is a failure mode for an allocation in tenured that fails. This comes in two flavours, Concurrent Mode Failure for when the failure occurs within a concurrent phase and Concurrent Mode Interrupted for when the failure occurs outside of a concurrent phase but within a concurrent cycle. The behaviour of a failure is to perform an orderly abort the concurrent cycle and start a Full GC. This is why it could and likely should be a CMSPhase (albeit a failure).
-
abstract class CMSPauseEvent extends GenerationalGCPauseEvent implements CMSPhase models a real event in the domain. I'm failing to see how that is a failure but, as always, I'm willing to entertain arguments.
"JVMEvent should be an interface and all these other event 'marker' interface would extend the JVMEvent interface."
A JVMEvent is something that happened in the JVM (not application) at a point in time and lasted for a duration. All GC events, concurrent and pause have this property. They've all happened at some point in time and they all lasted for the duration of time. Thus, I believe having JVMEvent as an abstract class at the top of the hierarchy properly captures this property in the model.
"Wouldn't you also want a way to classify events for other generational collectors? Why not an interface CMSEvent that can be used to mark events that are CMS events, along with a Parallel/G1GC/Serial/ZGC/Event?"
I don't believe this is needed for the serial and parallel generational collectors. G1GC is already defined in a way that makes it easier to sort events in a way that makes sense for G1. Since it's different than CMS, it's a different modeling. As for ZGC, I'm convinced that how it is currently modeled isn't as useful as it could be.
"And maybe there needs to be an interface for PauseEvent (with API for getting timestamp, gc type, gc cause, and duration) and an interface for ConcurrentEvent (with API for getting timestamp, gc type, gc cause, duration, cpu time and wallclock time)."
As GC type is captured in the class name/type gc type (legacy) having it as a field is redundant. It should be deprecated and removed.
All GC events come which is covered by a GCEvent extending a JVMEvent. I think I've covered off what defines a JVMEvent which leaves CPU breakout. GC thread CPU usage is only provided for some but not all pause events which is unfortunate.
At the end of the day, how we model needs to reflect what we are modeling as well as how we intend to use what we are modeling. There are absolutely many different ways to model this problem. That said, I didn't see a ton of need to create a ton of abstraction as GC is what it is and modeling as such seemed to be the most useful way to model it. If you add Safepoint events into the mix then it feels like this model holds up. However, if there is a better model then we should, of course, revisit.