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

Add JFR event #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion agent/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@
<shadedPattern>agent.net.bytebuddy</shadedPattern>
</relocation>
</relocations>
<createDependencyReducedPom>false</createDependencyReducedPom>
<createDependencyReducedPom>true</createDependencyReducedPom>
</configuration>
</execution>
</executions>
Expand Down
70 changes: 45 additions & 25 deletions agent/src/main/java/io/type/pollution/agent/Agent.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
import java.time.LocalDateTime;
import java.time.format.DateTimeFormatter;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.Executors;
import java.util.concurrent.TimeUnit;

Expand All @@ -40,6 +43,8 @@ public class Agent {
private static final Long REPORT_INTERVAL_SECS = Long.getLong("io.type.pollution.report.interval");
private static final boolean ENABLE_LAMBDA_INSTRUMENTATION = Boolean.getBoolean("io.type.pollution.lambda");

private static final Set<String> disabledClassLoaders = Collections.synchronizedSet(new HashSet<>());

public static void premain(String agentArgs, Instrumentation inst) {
if (ENABLE_FULL_STACK_TRACES) {
TraceInstanceOf.startMetronome(FULL_STACK_TRACES_SAMPLING_PERIOD_MS);
Expand Down Expand Up @@ -78,31 +83,43 @@ public static void premain(String agentArgs, Instrumentation inst) {
typeDescription,
classLoader,
module,
protectionDomain) -> builder
.visit(TypeConstantAdjustment.INSTANCE)
.visit(new AsmVisitorWrapper.AbstractBase() {

@Override
public int mergeWriter(int flags) {
return flags | ClassWriter.COMPUTE_FRAMES;
}

@Override
public int mergeReader(int flags) {
return flags;
}

@Override
public net.bytebuddy.jar.asm.ClassVisitor wrap(TypeDescription instrumentedType,
net.bytebuddy.jar.asm.ClassVisitor classVisitor,
Implementation.Context implementationContext,
TypePool typePool,
FieldList<FieldDescription.InDefinedShape> fields,
MethodList<?> methods,
int writerFlags, int readerFlags) {
return new ByteBuddyUtils.ByteBuddyTypePollutionClassVisitor(net.bytebuddy.jar.asm.Opcodes.ASM9, classVisitor);
}
})).installOn(inst);
protectionDomain) -> {
if (classLoader != null) {
// in gradle test runners, there are class loaders that don't include the agent classes
try {
classLoader.loadClass(TraceInstanceOf.class.getName());
} catch (ClassNotFoundException e) {
disabledClassLoaders.add(classLoader.toString());
return builder;
}
}

return builder
.visit(TypeConstantAdjustment.INSTANCE)
.visit(new AsmVisitorWrapper.AbstractBase() {

@Override
public int mergeWriter(int flags) {
return flags | ClassWriter.COMPUTE_FRAMES;
}

@Override
public int mergeReader(int flags) {
return flags;
}

@Override
public net.bytebuddy.jar.asm.ClassVisitor wrap(TypeDescription instrumentedType,
net.bytebuddy.jar.asm.ClassVisitor classVisitor,
Implementation.Context implementationContext,
TypePool typePool,
FieldList<FieldDescription.InDefinedShape> fields,
MethodList<?> methods,
int writerFlags, int readerFlags) {
return new ByteBuddyUtils.ByteBuddyTypePollutionClassVisitor(net.bytebuddy.jar.asm.Opcodes.ASM9, classVisitor);
}
});
}).installOn(inst);
}

private static void printFinalReport() {
Expand All @@ -125,6 +142,9 @@ private static CharSequence reportOf(Collection<TraceInstanceOf.TraceCounter.Sna
return "";
}
StringBuilder report = new StringBuilder();
for (String disabledClassLoader : disabledClassLoaders) {
report.append("Classes on this class loader could not be instrumented: ").append(disabledClassLoader).append('\n');
}
int rowId = 0;
for (TraceInstanceOf.TraceCounter.Snapshot counter : counters) {
report.append("--------------------------\n");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,13 @@ public void onTypeCheckHit(Class interfaceClazz, String trace) {
LAST_SEEN_INTERFACE_UPDATER.lazySet(this, interfaceClazz);
if (lastSeen != null) {
updateTraceCount(interfaceClazz, trace);

TypeCheckThrashEvent event = new TypeCheckThrashEvent();
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a Miss one too - or use some field to distinguish between them?

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.

Copy link
Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not using this inside a benchmark

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.

  1. use a atomic reference field updater which contains the current TypeCheckThrashEvent stored within the counter instance: it has to contain an atomic long with the number of accumulated thrashed events
  2. when the current one overflow - we can perform a compareAndSet(usedEvent, null) pro-bono: if it fails is not important
  3. any racy thread which overflow > 1 more past the threshold of the current event, can try create a new initialized event with compareAndSet(usedEvent, new Event(1)) - if it failes, can retry or give up, really - but with compareAndExchange, in theory can already check if there's a newer event ready to pick it

In 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.

if (event.isEnabled()) {
event.concreteClass = clazz.getName();
event.interfaceClass = interfaceClazz.getName();
event.commit();
}
}
}
}
Expand All @@ -118,7 +125,7 @@ public StackTraceArrayList(int capacity) {
private static final AtomicLongFieldUpdater<TraceCounter> SAMPLING_TICK_UPDATER =
AtomicLongFieldUpdater.newUpdater(TraceCounter.class, "lastSamplingTick");

private final Class clazz;
final Class clazz;
private volatile long lastSamplingTick = System.nanoTime();
private final ConcurrentHashMap<Trace, TraceData> traces = new ConcurrentHashMap<>();
private static final ThreadLocal<Trace> TRACE = ThreadLocal.withInitial(Trace::new);
Expand Down
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Instead, you can still reuse the existing trace regardless, because is free and cheap (is precomputed) and is "human actionable" the same, if pasted in the Idea output (as explained in the README).

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;
}