Use jfr parser from jdk mission control #1229

laurit · 2023-04-19T19:54:18Z

No description provided.

breedx-splk

Looks good overall, had a few thoughts/questions sprinkled throughout. Nothing too interesting....

I have a few hangups on this that I think we should discuss.

I was a little surprised at the net increase in LOC. I kinda would have expected a reduction, but ok, it's approximately the same. Is the addition of the jmc dependency and all its I prefixed classnames worth it? It's certainly less readable....
I know performance was likely to be a motivation behind this change, specifically reducing allocations which ultimately result in GC. Are there test numbers to back up this change as an improvement? Can those be shared?
Does going this route prevent us from adopting the upstream/collab jfr-connection?
Are we ok with this divergence from the community collab code? Do we think this ultimately makes things more compatible or less compatible in the mid/long term?

breedx-splk · 2023-04-21T18:11:43Z

agent/license-normalizer-bundle.json

+    },
+    {
+      "bundleName": "UPL-BSD-3-Clause",
+      // org.openjdk.jmc:flightrecorder gets detected as UPL, it is really UPL+BSD dual license"


sadly, json doesn't support comments like this. An alternative would be to put it as a string in some other key that hopefully gets ignored.

Whatever parses this seems to be able to cope with such comments. Also tried adding comment as a field, but that failed because the parser tried to bind this to a class field that doesn't exist.

profiler/src/main/java/com/splunk/opentelemetry/profiler/EventReader.java

profiler/src/main/java/org/openjdk/jmc/flightrecorder/internal/parser/v1/ThreadUtil.java

profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrPathHandler.java

breedx-splk · 2023-04-21T20:48:45Z

profiler/src/main/java/com/splunk/opentelemetry/profiler/JfrPathHandler.java

+        buffer = chunkLoader.call();
+        IItemCollection items = EventCollectionUtil.build(context.buildEventArrays());
+        items.stream().flatMap(iItems -> iItems.stream()).forEach(eventProcessingChain::accept);
+        eventProcessingChain.flushBuffer();


This is a behavior change. It used to flush after each file, now it's flushing after each chunk. Should this go back outside the loop?

In previous parser chunks were hidden so we used our own chunk tracker that called flushBuffer on chunk change. The behavior didn't change, just the flushBuffer call moved.

...c/main/java/com/splunk/opentelemetry/profiler/allocation/sampler/AllocationEventSampler.java

breedx-splk · 2023-04-21T21:10:35Z

profiler/src/main/java/com/splunk/opentelemetry/profiler/util/StackSerializer.java

+    String methodName = method.getMethodName();
+    Integer lineNumber = frame.getFrameLineNumber();
+    if (lineNumber == null) {
+      lineNumber = -1;


GDI spec says 0 when unknown.

Thanks, will change. Note that previous parser returned -1 when line number was not available

Replaced all occurrences of -1, please review.

breedx-splk · 2023-04-21T21:12:31Z

profiler/src/test/java/com/splunk/opentelemetry/profiler/EventProcessingChainTest.java

@@ -88,108 +89,25 @@ void testLifecycle() {
    inOrder.verifyNoMoreInteractions();
  }

-  @Test
-  void testFlushOnChunkChange() {


I guess this is less interesting now that the chunk tracker exists elsewhere? Should we still verify that flush is called somewhere though?

I think this is covered with other tests, if flushBuffer is not called nothing will get exported with pprof.

laurit · 2023-04-24T07:02:31Z

I was a little surprised at the net increase in LOC. I kinda would have expected a reduction, but ok, it's approximately the same. Is the addition of the jmc dependency and all its I prefixed classnames worth it? It's certainly less readable....

I think if you exclude the added license files then there is no change in loc. It is true that the api of the previous parser is nicer, but considering that we need the profiling code to have minimal overhead everything that improves the situation is worth it, even if it comes at the cost of code readability.

I know performance was likely to be a motivation behind this change, specifically reducing allocations which ultimately result in GC. Are there test numbers to back up this change as an improvement? Can those be shared?

I'll put together some stuff and paste it to slack.

Does going this route prevent us from adopting the upstream/collab jfr-connection?

It doesn't. jfr-connection just returns a stream with jfr content, it doesn't affect the parsing. jfr-connection has a test that uses jmc parser. Currently I fail to see a reason why we would want to use jfr-connection.

mateuszrzeszutek

Looks promising.

I'm a little bit worried about all the internal JMC classes that we use; but the JFR based implementation also depends on internal behavior (chunks) so it's not exactly a new thing.

.../com/splunk/opentelemetry/profiler/allocation/exporter/PlainTextAllocationEventExporter.java

laurit requested review from a team as code owners April 19, 2023 19:54

laurit force-pushed the jfr-parser branch from cf6b8ef to ca357e6 Compare April 20, 2023 16:37

laurit requested a review from a team as a code owner April 20, 2023 16:37

breedx-splk reviewed Apr 21, 2023

View reviewed changes

laurit force-pushed the jfr-parser branch from 06f56e3 to 162e3d2 Compare April 24, 2023 14:06

mateuszrzeszutek reviewed Apr 25, 2023

View reviewed changes

.../com/splunk/opentelemetry/profiler/allocation/exporter/PlainTextAllocationEventExporter.java Outdated Show resolved Hide resolved

laurit added 6 commits April 25, 2023 14:50

Use jfr parser from jdk mission control

637e3d5

add licenses

d31daea

spotless

c76b050

address review comment

3539c93

address review comments

526b68b

update licenses

311bb0b

laurit force-pushed the jfr-parser branch from cc337bb to 311bb0b Compare April 25, 2023 11:56

fix null check

d650fee

mateuszrzeszutek approved these changes Apr 28, 2023

View reviewed changes

Unknow line number should use 0

af696de

breedx-splk approved these changes Apr 28, 2023

View reviewed changes

laurit added 2 commits April 28, 2023 19:51

suppress warning

555772e

add null check

fb5dbfb

laurit merged commit c2ab0c1 into signalfx:main Apr 28, 2023

laurit deleted the jfr-parser branch April 28, 2023 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use jfr parser from jdk mission control #1229

Use jfr parser from jdk mission control #1229

laurit commented Apr 19, 2023

breedx-splk left a comment

breedx-splk Apr 21, 2023

laurit Apr 22, 2023

breedx-splk Apr 21, 2023

laurit Apr 22, 2023

breedx-splk Apr 21, 2023

laurit Apr 25, 2023

laurit Apr 25, 2023 •

edited

Loading

breedx-splk Apr 21, 2023

laurit Apr 22, 2023

laurit commented Apr 24, 2023

mateuszrzeszutek left a comment

Use jfr parser from jdk mission control #1229

Use jfr parser from jdk mission control #1229

Conversation

laurit commented Apr 19, 2023

breedx-splk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurit Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurit commented Apr 24, 2023

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

laurit Apr 25, 2023 •

edited

Loading