Per-thread flamegraph option in JFR heatmap converter #1414
Conversation
| } | ||
|
|
||
| private Integer getMethodIndex(MethodKey key) { | ||
| return methodCache.computeIfAbsent(key, this::makeMethod); |
There was a problem hiding this comment.
Good. Now comes the trick: instead of this::makeMethod pass a lambda/method reference cached in final Function<MethodKey, Integer> field, and you'll get 5% performance improvement out of thin air.
There was a problem hiding this comment.
Alternatively, you can replace computeIfAbsent with get+put pair without lambda and get the same effect.
There was a problem hiding this comment.
I had a quick look at the flamegraph and it looks a tiny bit faster, but why can't the JVM make this optimization on its own? I don't see the difference between a final Function<MethodKey, Integer> and the method reference. Is it just the additional reference to the class?
There was a problem hiding this comment.
but why can't the JVM make this optimization on its own
Function object created by this::someMethod depends on the instance. JVM does do per-instance caching. I can understand why: let's say there are 10 method references in the code - this would require JVM to add 10 extra synthetic fields, which may or may not be used, for every instance of an otherwise small object.
So, it's up to developer to decide whether to cache non-static lambdas or not.
fandreuz
force-pushed
the
per-thread-flamegraph
branch
from
c4569a0 to
cb506dd
Compare
fandreuz
force-pushed
the
per-thread-flamegraph
branch
from
cb506dd to
041cfb7
Compare
| stackTracesCache.put(id, stackTracesRemap.index(cachedStackTrace, size)); | ||
| } | ||
|
|
||
| private Integer getMethodIndex(MethodKey key) { |
There was a problem hiding this comment.
Why Integer? Return value is always used as int.
| return methodIdx; | ||
| } | ||
|
|
||
| private Integer makeMethod(MethodKey key) { |
There was a problem hiding this comment.
int. Or just inline this function in the above method.
| private final MethodKeyType keyType; | ||
|
|
||
| public MethodKey(MethodKeyType keyType, long methodId, int location, byte type, boolean firstInStack) { | ||
| if (type < 0) throw new IllegalArgumentException("Unexpected type: " + type); |
There was a problem hiding this comment.
Unnecessary limitation. Just replace (long) type with (type & 0xffL).
| } | ||
|
|
||
| public void beforeChunk() { | ||
| state.methodsCache.clear(); |
There was a problem hiding this comment.
Seems like this corrupted frames in multi-chunk recordings.
Cleaning method cache between chunks is essential.
There was a problem hiding this comment.
How did you find this out? I converted a 4gb JFR and the output looked ok. I'd assume it contains more than one chunk, will check
There was a problem hiding this comment.
Works like a charm now, thanks!
fandreuz
force-pushed
the
per-thread-flamegraph
branch
from
0b9e7a4 to
568425b
Compare
| jfr.stackTraces.forEach(new Dictionary.Visitor<StackTrace>() { | ||
| @Override | ||
| public void visit(long key, StackTrace trace) { | ||
| heatmap.beforeChunk(); |
There was a problem hiding this comment.
Dumb mistake, sorry. 9d970af
Signed-off-by: Andrei Pangin <1749416+apangin@users.noreply.github.com>
4 participants
Description
In this PR I introduce a new feature in the JFR heatmap converter to have an artificial frame at the base of the flamegraph containing the thread name where the stacktrace happened.
Related issues
#1316
Motivation and context
Motivation in the original issue.
How has this been tested?
Manual testing.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.