chore: Simplify flight recorder interactions #32499

sebastian-alfers · 2024-09-02T14:07:21Z

chore: Simplify flight recorder interactions. Fixes #32309

Drop the actor extension approach.
Drop the akka.java-flight-recorder.enabled, to always recorder Flight Recorder interactions.
Drop the java version check.
Drop all NoOp fallbacks and let jvm'sx -XX:+UnlockCommercialFeatures -XX:+FlightRecorder handle any further usage.

johanandren · 2024-09-02T14:14:53Z

akka-actor-typed/src/main/scala/akka/actor/typed/internal/ActorFlightRecorder.scala

-      "akka.actor.typed.internal.jfr.JFRActorFlightRecorder",
-      NoOpActorFlightRecorder)
-}
+  lazy val recorder: ActorFlightRecorder = new JFRActorFlightRecorder()


I think we can make the object the implementation instead of a separate singleton field, and then use that directly instead of a field in the classes that use it.

johanandren

More fields to skip, I expect all to be internal so should be fine to mima filter them if MiMa is unhappy about dropping constructor parameters etc.

johanandren · 2024-09-03T06:57:38Z

akka-actor-typed/src/main/scala/akka/actor/typed/delivery/internal/ConsumerControllerImpl.scala

@@ -274,7 +274,7 @@ private class ConsumerControllerImpl[A] private (
  import ProducerControllerImpl.Resend
  import settings.flowControlWindow

-  private val flightRecorder = ActorFlightRecorder.recorder
+  private val flightRecorder = ActorFlightRecorder


And no need for this field or the parameter above, use the ActorFlightRecorder directly

johanandren · 2024-09-03T06:57:52Z

akka-cluster-sharding/src/main/scala/akka/cluster/sharding/Shard.scala

@@ -448,7 +448,7 @@ private[akka] class Shard(

  private val rememberEntities: Boolean = rememberEntitiesProvider.isDefined

-  private val flightRecorder = ShardingFlightRecorder.recorder
+  private val flightRecorder = ShardingFlightRecorder


Use directly instead

johanandren · 2024-09-03T06:58:42Z

akka-remote/src/main/scala/akka/remote/artery/ArteryTransport.scala

@@ -284,7 +284,7 @@ private[remote] abstract class ArteryTransport(_system: ExtendedActorSystem, _pr

  override val log: MarkerLoggingAdapter = Logging.withMarker(system, classOf[ArteryTransport])

-  val flightRecorder: RemotingFlightRecorder = RemotingFlightRecorder.recorder
+  private val flightRecorder = RemotingFlightRecorder


drop field, and also the debug log statement below, it's there to know which recorder was used but now there is only one

johanandren · 2024-09-03T06:58:51Z

akka-remote/src/main/scala/akka/remote/artery/Association.scala

@@ -150,7 +150,7 @@ private[remote] class Association(
  require(remoteAddress.port.nonEmpty)

  private val log = Logging.withMarker(transport.system, classOf[Association])
-  private def flightRecorder = transport.flightRecorder
+  private def flightRecorder = RemotingFlightRecorder


use directly

remove this def altogether?

johanandren · 2024-09-03T06:59:52Z

Dang it, meant to comment, not to approve

sebastian-alfers · 2024-09-03T07:12:17Z

akka-actor/src/main/resources/reference.conf

-  # Some modules (remoting only right now) can emit custom events to the Java Flight Recorder if running
-  # on JDK 11 or later. If you for some reason do not want that, it can be disabled and switched to no-ops
-  # with this toggle.
-  java-flight-recorder {


This should probably be mentioned in the release-notes MD?

Yes, not a big problem but we should still mention

Ok, going to rebase on top of the merged #32497.

akka-remote/src/test/scala-jdk9-only/akka/remote/artery/jfr/RemotingFlightRecorderSpec.scala

.../main/resources/META-INF/native-image/com.typesafe.akka/akka-actor-typed/reflect-config.json

.../resources/META-INF/native-image/com.typesafe.akka/akka-cluster-sharding/reflect-config.json

...e/src/main/resources/META-INF/native-image/com.typesafe.akka/akka-remote/reflect-config.json

akka-cluster-sharding/src/test/scala/akka/cluster/sharding/NativeImageMetadataSpec.scala

akka-remote/src/test/scala/akka/remote/NativeImageMetadataSpec.scala

akka-actor-typed-tests/src/test/scala/akka/actor/typed/NativeImageMetadataSpec.scala

patriknw · 2024-09-03T12:12:51Z

...yped/src/main/mima-filters/2.10.0.backwards.excludes/32499-simplify-flight-recorder.excludes

@@ -0,0 +1,8 @@
+# #32499 chore: Simplify flight recorder interactions


The file name should be the latest release. Did this solve anything? Is it needed?

I changed the name 👍

Did this solve anything? Is it needed?

When I remove the content of the file, I get this error:

sbt:akka-actor> mimaReportBinaryIssues [error] akka-actor: Failed binary compatibility check against com.typesafe.akka:akka-actor_2.13:2.6.7! Found 2 potential problems (filtered 112) [error] * class akka.util.FlightRecorderLoader does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[MissingClassProblem]("akka.util.FlightRecorderLoader") [error] * object akka.util.FlightRecorderLoader does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[MissingClassProblem]("akka.util.FlightRecorderLoader$") [error] stack trace is suppressed; run last mimaReportBinaryIssues for the full output [error] (mimaReportBinaryIssues) Failed binary compatibility check against com.typesafe.akka:akka-actor_2.13:2.6.7! Found 2 potential problems (filtered 112) [error] Total time: 0 s, completed Sep 4, 2024, 9:44:43 AM sbt:akka-actor>

patriknw · 2024-09-03T12:15:03Z

...ctor/src/main/mima-filters/2.10.0.backwards.excludes/32499-simplify-flight-recorder.excludes

@@ -0,0 +1,3 @@
+# #32499 chore: Simplify flight recorder interactions


patriknw · 2024-09-03T12:28:41Z

akka-remote/src/main/scala/akka/remote/artery/RemotingFlightRecorder.scala

+    new TcpOutboundConnected(remoteAddress, streamName).commit()
+
+  def tcpOutboundSent(size: Int): Unit =
+    new TcpOutboundSent(size).commit()


Do we have to verify that it works as we expect? That this isn't adding any overhead when JFR isn't used? We had a description somewhere of how the jvm would omit this execution, but can't find it. I assume there wouldn't even be allocations of the events that could increase pressure on GC?

Yeah, I have the same recollection. It should be escape analysed to zero allocation when enabled, but then also completely eliminated when JFR is disabled. I also don't have any reference though, so we should double check by running a benchmark with JFR disabled and look compare allocations (can't use JFR obviously :D)

maybe jmh with -prof gc would be sufficient

Yeah, my thinking, before and after this change to compare allocation rate, maybe the CodecBenchmark I think that involves some of the remoting stream stages?

I'll do some verification bench runs today

patriknw · 2024-09-04T08:12:28Z

...ped/src/main/mima-filters/2.9.5.0.backwards.excludes/32499-simplify-flight-recorder.excludes

@@ -0,0 +1,8 @@
+# #32499 chore: Simplify flight recorder interactions


2.9.5, not 2.9.5.0

johanandren · 2024-09-04T11:08:22Z

Bench results looks good, I think the differences between main and this branch in my results is small enough to be just noise, and the normalised allocation rate looks the same (I expect it should be allocating noticeably more per op if our assumptions were true)

Raw output from runs and details:
jfr-object-bench-results.txt

patriknw · 2024-09-04T15:29:20Z

Is the CodecBenchmark using RemotingFlightRecorder? I don't see that Encoder or Decoder stages are using it.

johanandren · 2024-09-06T08:39:47Z

The InboundCompressionsImpl used by the decoder does, but now that I look closer it does not do a lot of JFR event emit (only on advertisement). I'll do my homework and run a better bench somehow.

johanandren · 2024-09-09T07:22:29Z

New bench run, you can see bench details here https://github.com/akka/akka/compare/wip-jfr-simplify-bench?expand=1#diff-5716ff6ab54d9a1887ab449af41b3f2b185e83cb6a22bc8d5c3580b4cdf4bb00R1 , it basically compares alloc rate for incrementing a counter to 10 with counting to 10 calling RemotingFlightRecorder for every increment.

Result seems to confirm that those calls are thrown out by JIT:

[info] JfrOverheadBenchmark.withJfr                        thrpt    2  25358066,747          ops/ms
[info] JfrOverheadBenchmark.withJfr:gc.alloc.rate          thrpt    2         0,001          MB/sec
[info] JfrOverheadBenchmark.withJfr:gc.alloc.rate.norm     thrpt    2        ≈ 10⁻⁸            B/op
[info] JfrOverheadBenchmark.withJfr:gc.count               thrpt    2           ≈ 0          counts
[info] JfrOverheadBenchmark.withoutJfr                     thrpt    2  25071049,832          ops/ms
[info] JfrOverheadBenchmark.withoutJfr:gc.alloc.rate       thrpt    2         0,001          MB/sec
[info] JfrOverheadBenchmark.withoutJfr:gc.alloc.rate.norm  thrpt    2        ≈ 10⁻⁸            B/op
[info] JfrOverheadBenchmark.withoutJfr:gc.count            thrpt    2           ≈ 0          counts

patriknw · 2024-09-09T15:13:50Z

thanks for the new bench

patriknw

LGTM

johanandren reviewed Sep 2, 2024

View reviewed changes

johanandren approved these changes Sep 3, 2024

View reviewed changes

sebastian-alfers commented Sep 3, 2024

View reviewed changes

akka-remote/src/test/scala-jdk9-only/akka/remote/artery/jfr/RemotingFlightRecorderSpec.scala Outdated Show resolved Hide resolved

sebastian-alfers force-pushed the jfr-simplify branch from 544e8b0 to 8f498a1 Compare September 3, 2024 09:13

johanandren reviewed Sep 3, 2024

View reviewed changes

patriknw reviewed Sep 3, 2024

View reviewed changes

sebastian-alfers force-pushed the jfr-simplify branch from b29d0e5 to 67238d7 Compare September 4, 2024 07:56

patriknw reviewed Sep 4, 2024

View reviewed changes

sebastian-alfers force-pushed the jfr-simplify branch from 18ecb1e to 6380119 Compare September 6, 2024 07:25

sebastian-alfers force-pushed the jfr-simplify branch from 6380119 to b327f21 Compare September 9, 2024 14:41

sebastian-alfers added 5 commits September 9, 2024 16:47

chore: Simplify flight recorder interactions

db6faf0

release notes comment

16e661f

align native image test

bdd9a77

fix version in path for mima files

997356d

fix version in mima file - again

ae360a1

sebastian-alfers force-pushed the jfr-simplify branch from b327f21 to ae360a1 Compare September 9, 2024 14:49

patriknw approved these changes Sep 9, 2024

View reviewed changes

johanandren approved these changes Sep 9, 2024

View reviewed changes

sebastian-alfers merged commit b6501eb into akka:main Sep 9, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Simplify flight recorder interactions #32499

chore: Simplify flight recorder interactions #32499

sebastian-alfers commented Sep 2, 2024

johanandren Sep 2, 2024

johanandren left a comment

johanandren Sep 3, 2024

johanandren Sep 3, 2024

johanandren Sep 3, 2024

johanandren Sep 3, 2024

patriknw Sep 4, 2024

johanandren commented Sep 3, 2024

sebastian-alfers Sep 3, 2024

johanandren Sep 3, 2024 •

edited

Loading

sebastian-alfers Sep 3, 2024

patriknw Sep 3, 2024

sebastian-alfers Sep 4, 2024

patriknw Sep 3, 2024

patriknw Sep 3, 2024

johanandren Sep 3, 2024

patriknw Sep 3, 2024

johanandren Sep 3, 2024

johanandren Sep 4, 2024

patriknw Sep 4, 2024

sebastian-alfers Sep 4, 2024

johanandren commented Sep 4, 2024

patriknw commented Sep 4, 2024

johanandren commented Sep 6, 2024 •

edited

Loading

johanandren commented Sep 9, 2024

patriknw commented Sep 9, 2024

patriknw left a comment

		@@ -0,0 +1,8 @@
		# #32499 chore: Simplify flight recorder interactions

		@@ -0,0 +1,3 @@
		# #32499 chore: Simplify flight recorder interactions

chore: Simplify flight recorder interactions #32499

chore: Simplify flight recorder interactions #32499

Conversation

sebastian-alfers commented Sep 2, 2024

Choose a reason for hiding this comment

johanandren 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

johanandren commented Sep 3, 2024

Choose a reason for hiding this comment

johanandren Sep 3, 2024 • edited Loading

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

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

johanandren commented Sep 4, 2024

patriknw commented Sep 4, 2024

johanandren commented Sep 6, 2024 • edited Loading

johanandren commented Sep 9, 2024

patriknw commented Sep 9, 2024

patriknw left a comment

Choose a reason for hiding this comment

johanandren Sep 3, 2024 •

edited

Loading

johanandren commented Sep 6, 2024 •

edited

Loading