Comments (8)
"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.
from gctoolkit.
They are related and @kcpeppe is taking a look :-)
from gctoolkit.
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.
from gctoolkit.
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?
from gctoolkit.
@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 :)
from gctoolkit.
@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.
from gctoolkit.
I've approved #326 based on code quality (syntax) but I've set you both as reviewers pending this discussion
from gctoolkit.
I think we can merge. What @dsgrieve is describing would be a disruptive change to what is a public API. We'll need to think hard about the cost/benefits of such a change.
from gctoolkit.
Related Issues (20)
- gen-z support in Aggregator/Aggregation
- Java 17 Parallel Old/New Parsing has altered from JDK 11, impacting what is reported as promoted bytes and what is reported in young gen HOT 2
- Memory pool size data HOT 2
- IllegalStateException: Already undeployed HOT 5
- Don't need of additional stream. HOT 2
- VertxJVMEventChannel could not be instantiated HOT 4
- Gctoolkit doesnt parse this log HOT 5
- GCtoolkit doesnt find the full gc pattern HOT 2
- How do code with GitHub without installing git
- UnifiedG1GCParser doesn't recognize survivor & eden occupancy HOT 3
- Unified Log new Decorator not supported HOT 1
- [Question] Why GenerationalHeapParser ignored CMS Concurrent Event and Remark but only recognized InitialMark? HOT 9
- CMSTenuredPoolParser does not parse initialMark and CMSRemark correctly
- new release request HOT 6
- [I/O] GCLogFile.diary() open the file 2 times and don't release it HOT 1
- AbstractJavaVirtualMachine.getCommandLine() is currently stubbed out, can't retrieve CLI parameters for Pre-Unified log. HOT 2
- NPE in Aggregation.estimatedStartTime() when processing a Pre-Unified GC log with timestamps only. HOT 6
- GCToolKit support for simple logs
- Generational parser does not seem to handle GC Cause included in young event. HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from gctoolkit.