On Fri, 30 May 2025 22:30:25 GMT, Erik Gahlin <egah...@openjdk.org> wrote:
> Could I have review of an enhancement that adds rate-limited sampling to Java > event, including five events in the JDK (SocketRead, SocketWrite, FileRead, > FileWrite, and JavaExceptionThrow). > > Testing: test/jdk/jdk/jfr > > Thanks > Erik src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 261: > 259: getfield(codeBuilder, eventClassDesc, > ImplicitFields.FIELD_DURATION); > 260: Bytecode.invokevirtual(codeBuilder, > TYPE_EVENT_CONFIGURATION, METHOD_THROTTLE); > 261: int result = codeBuilder.allocateLocal(TypeKind.LONG); Do we really need to store the result in a local? Can't we just dup it on the expression stack and store it directly into the field after another aload, or dup? Perhaps dup twice to then issue the mask operation? src/jdk.jfr/share/classes/jdk/jfr/internal/EventInstrumentation.java line 590: > 588: private void setDuration(CodeBuilder codeBuilder, > Consumer<CodeBuilder> expression) { > 589: codeBuilder.aload(0); > 590: expression.accept(codeBuilder); dont know what expression.accept() does, but does it really consume the this pointer? I see its pushed again (aload(0)) if its throttled below? src/jdk.jfr/share/classes/jdk/jfr/internal/event/EventConfiguration.java line 59: > 57: // static boolean shouldThrottleCommit(long timestamp) > 58: public boolean shouldThrottleCommit(long timestamp) { > 59: return throttler.sample(timestamp); Can we assert on isEnabled? src/jdk.jfr/share/classes/jdk/jfr/internal/event/EventConfiguration.java line 77: > 75: return rawDuration; > 76: } > 77: long endTime = startTime + rawDuration; I see now that you are using endTime as well, nice. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118272073 PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118272829 PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118277498 PR Review Comment: https://git.openjdk.org/jdk/pull/25559#discussion_r2118279057