-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add JFR event #19
base: master
Are you sure you want to change the base?
Add JFR event #19
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package io.type.pollution.agent; | ||
|
||
import jdk.jfr.Category; | ||
import jdk.jfr.Description; | ||
import jdk.jfr.Enabled; | ||
import jdk.jfr.Event; | ||
import jdk.jfr.Label; | ||
import jdk.jfr.Name; | ||
import jdk.jfr.StackTrace; | ||
|
||
@Name(TypeCheckThrashEvent.NAME) | ||
@Label("Type check cache thrashing") | ||
@Category("Type check") | ||
@Description("An interface type check is invalidating a previously cached type check result. If this happens often, it can lead to thrashing and contention of the cache field.") | ||
@StackTrace | ||
@Enabled(false) | ||
public class TypeCheckThrashEvent extends Event { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The stack trace help - but it will print it till the tracing class, which is not very nice. |
||
static final String NAME = "io.type.pollution.agent.TypeCheckThrashEvent"; | ||
|
||
@Label("Concrete type") | ||
@Description("Concrete class of the object that was type checked (the thrashed cache field is stored in this class)") | ||
public String concreteClass; | ||
|
||
@Label("Interface type") | ||
@Description("Interface that this object was checked against (this is the type that is stored in the thrashed cache field)") | ||
public String interfaceClass; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type-pollution-agent/agent/src/main/java/io/type/pollution/agent/TraceInstanceOf.java
Line 81 in 5324120
IDK if you plan to have it running in a proper benchmark but this can be a VERY high frequency event, and I've designed it to run in the hot path, at the time e.g. traces are pre-computed on bytecode transformation
if you plan to do something similar, maybe you just want to emit the summary event instead - like the report - on demand or at intervals or using a similar trick than the new TLAB JFR events (by using a counter which "overflow" and emit events only if it overflows).
If is not meant to be used in a real benchmark (but sadly that's the easier way to make it happen too :( ) let's leave it as it is and in case, change it in something cheaper after, if it create problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed, this code is very hot. I disabled the event by default for that reason. The performance overhead is substantial in my testing, but only when there is thrashing (for my tests, that's the failure case).
I am not using this inside a benchmark, though I am repurposing jmh benchmarks for this test. So it's not performance critical to me. I hope this logic is sufficient to make it no overhead for the case where the jfr event is not enabled (isEnabled should be constant folded).
The problem with emitting the report is that it is a complicated nested data structure. This makes emitting but also parsing more complicated. I didn't feel it was worth it for the jfrunit use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I can see it why but remember that the "pollution" pattern depends by both the ordering of type checks - which can trash the cache using a single thread OR because of high level of concurrency - which means you need an option to do it in a proper real world benchmark too, although it will be used just to detect that there's some trashing going on and fail a release - for example. Something which cannot be done with simple unit tests, really.
What can be done here, to enable using this in high-throughput scenario is:
i.e.
TypeCheckThrashEvent
stored within the counter instance: it has to contain an atomic long with the number of accumulated thrashed eventsIn a jfr unit test, the default threshold can be 1: we don't care about performance and should "just work": in a benchmark we can have 1000 or 10000 and the actual event count must be scaled using the limit, as per TLAB allocation events, in JFR.
Let me know if it seems appealing for you: if not we can go through this one, but when you'll need it for benchmark, we should get back to this, eventually.