RFR: 8358750: JFR: EventInstrumentation MASK_THROTTLE* constants should be computed in longs
Could I have a review of a fix to the throttling mechanism added in JDK 25? Long values should be shifted instead of integer values when creating the masks. During debugging, I found several issues, and I also strengthened the test. - The start time of the event needs to be stored if the event is being throttled. The reason is that the timestamp is reused later to determine if the event should be throttled or not. This happens with events that lack an end time. - The duration value that is written to the buffer must remove the mask if it is throttled. This is handled by`getDuration(blockCodeBuilder)`. - I added a clarifying comment that when duration is compared to 0, it also prevents a new duration from being calculated if the event has been throttled (for an instant event). The test now checks that the duration is not negative, that the start time is not before the test starts, and that the duration is not unreasonably large. The reason I multiply the recording duration by two is to give the test some slack in case we run into clock issues where the time taken for the event doesn't match the recording. Purpose of the check is to make sure we don't end up with the mask being added to the duration. Testing: test/jdk/jdk/jfr I validated manually that the throttle rate works using a small test program I have (which can't be checked in due to stability issues). ------------- Commit messages: - Initial Changes: https://git.openjdk.org/jdk/pull/26028/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26028&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8358750 Stats: 78 lines in 2 files changed: 48 ins; 22 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/26028.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26028/head:pull/26028 PR: https://git.openjdk.org/jdk/pull/26028
On Fri, 27 Jun 2025 20:49:37 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of a fix to the throttling mechanism added in JDK 25? Long values should be shifted instead of integer values when creating the masks.
During debugging, I found several issues, and I also strengthened the test.
- The start time of the event needs to be stored if the event is being throttled. The reason is that the timestamp is reused later to determine if the event should be throttled or not. This happens with events that lack an end time.
- The duration value that is written to the buffer must remove the mask if it is throttled. This is handled by`getDuration(blockCodeBuilder)`.
- I added a clarifying comment that when duration is compared to 0, it also prevents a new duration from being calculated if the event has been throttled (for an instant event).
The test now checks that the duration is not negative, that the start time is not before the test starts, and that the duration is not unreasonably large. The reason I multiply the recording duration by two is to give the test some slack in case we run into clock issues where the time taken for the event doesn't match the recording. Purpose of the check is to make sure we don't end up with the mask being added to the duration.
Testing: test/jdk/jdk/jfr + tier1
I validated manually that the throttle rate works using a small test program I have (which can't be checked in due to stability issues).
test/jdk/jdk/jfr/api/metadata/annotations/TestThrottle.java line 55:
53: public class TestThrottle { 54: 55: public static class UnthrottledEvent extends Event {
Suggestion: public static class UnthrottledEvent extends Event { ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26028#discussion_r2175776993
Could I have a review of a fix to the throttling mechanism added in JDK 25? Long values should be shifted instead of integer values when creating the masks.
During debugging, I found several issues, and I also strengthened the test.
- The start time of the event needs to be stored if the event is being throttled. The reason is that the timestamp is reused later to determine if the event should be throttled or not. This happens with events that lack an end time.
- The duration value that is written to the buffer must remove the mask if it is throttled. This is handled by`getDuration(blockCodeBuilder)`.
- I added a clarifying comment that when duration is compared to 0, it also prevents a new duration from being calculated if the event has been throttled (for an instant event).
The test now checks that the duration is not negative, that the start time is not before the test starts, and that the duration is not unreasonably large. The reason I multiply the recording duration by two is to give the test some slack in case we run into clock issues where the time taken for the event doesn't match the recording. Purpose of the check is to make sure we don't end up with the mask being added to the duration.
Testing: test/jdk/jdk/jfr + tier1
I validated manually that the throttle rate works using a small test program I have (which can't be checked in due to stability issues).
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Remove whitespace ------------- Changes: - all: https://git.openjdk.org/jdk/pull/26028/files - new: https://git.openjdk.org/jdk/pull/26028/files/214c9397..9d397d90 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=26028&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=26028&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/26028.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26028/head:pull/26028 PR: https://git.openjdk.org/jdk/pull/26028
On Tue, 1 Jul 2025 08:12:54 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of a fix to the throttling mechanism added in JDK 25? Long values should be shifted instead of integer values when creating the masks.
During debugging, I found several issues, and I also strengthened the test.
- The start time of the event needs to be stored if the event is being throttled. The reason is that the timestamp is reused later to determine if the event should be throttled or not. This happens with events that lack an end time.
- The duration value that is written to the buffer must remove the mask if it is throttled. This is handled by`getDuration(blockCodeBuilder)`.
- I added a clarifying comment that when duration is compared to 0, it also prevents a new duration from being calculated if the event has been throttled (for an instant event).
The test now checks that the duration is not negative, that the start time is not before the test starts, and that the duration is not unreasonably large. The reason I multiply the recording duration by two is to give the test some slack in case we run into clock issues where the time taken for the event doesn't match the recording. Purpose of the check is to make sure we don't end up with the mask being added to the duration.
Testing: test/jdk/jdk/jfr + tier1
I validated manually that the throttle rate works using a small test program I have (which can't be checked in due to stability issues).
Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:
Remove whitespace
Marked as reviewed by mgronlun (Reviewer). ------------- PR Review: https://git.openjdk.org/jdk/pull/26028#pullrequestreview-2982995446
On Fri, 27 Jun 2025 20:49:37 GMT, Erik Gahlin <egahlin@openjdk.org> wrote:
Could I have a review of a fix to the throttling mechanism added in JDK 25? Long values should be shifted instead of integer values when creating the masks.
During debugging, I found several issues, and I also strengthened the test.
- The start time of the event needs to be stored if the event is being throttled. The reason is that the timestamp is reused later to determine if the event should be throttled or not. This happens with events that lack an end time.
- The duration value that is written to the buffer must remove the mask if it is throttled. This is handled by`getDuration(blockCodeBuilder)`.
- I added a clarifying comment that when duration is compared to 0, it also prevents a new duration from being calculated if the event has been throttled (for an instant event).
The test now checks that the duration is not negative, that the start time is not before the test starts, and that the duration is not unreasonably large. The reason I multiply the recording duration by two is to give the test some slack in case we run into clock issues where the time taken for the event doesn't match the recording. Purpose of the check is to make sure we don't end up with the mask being added to the duration.
Testing: test/jdk/jdk/jfr + tier1
I validated manually that the throttle rate works using a small test program I have (which can't be checked in due to stability issues).
This pull request has now been integrated. Changeset: 77e69e02 Author: Erik Gahlin <egahlin@openjdk.org> URL: https://git.openjdk.org/jdk/commit/77e69e02ebd280636859dd698423db6ac3bc7f5c Stats: 78 lines in 2 files changed: 48 ins; 22 del; 8 mod 8358750: JFR: EventInstrumentation MASK_THROTTLE* constants should be computed in longs Reviewed-by: mgronlun ------------- PR: https://git.openjdk.org/jdk/pull/26028
participants (3)
-
Andrey Turbanov
-
Erik Gahlin
-
Markus Grönlund