RFR: 8335907: JFR: Make SettingControls more robust [v2]

Erik Gahlin egahlin at openjdk.org
Mon Jul 29 10:53:52 UTC 2024


> Could I have a review of changes to the SettingControl implementations to make them more robust:
> 
> - Consistently ignore invalid values for the methods `combine(Set)` and `setValue(String)`.
> - Consistently set the textual value field in the `SettingControl` after the actual value has been changed.
> - Consistently set the textual value field to the default value in the constructor.
> 
> A test was added to ensure that current and future setting controls are properly tested. This also improves test coverage. See [JDK-8309052](https://bugs.openjdk.org/browse/JDK-8309052)
> 
> **Implementation comments:**
> 
> - A `jdk.jfr.internal.util.Rate` class was added to make the code more readable.
> - The `ValueParser$TimespanUnit` class was made a top-level class for unit handling.
> - A `Utils::multiplyOverflow(long, long, long)` method was added to avoid negative values in case of an overflow.
> - `String::strip` was used instead of `String::trim` to make the implementation Unicode aware.
> - The cutoff setting in the `EventControl` class was changed to “infinity”, which in practice was already the default value.
> 
> Composition, which is typically preferable over inheritance, was previously used for the `EnabledSetting` and `StackTraceSetting` classes. However, the code became hard to read and error-prone, so there is now an abstract `BooleanSetting` class to enforce consistency.
> 
> **Testing:**
>  - test/jdk/jdk/jfr/*
>  - 100 * test/jdk/jdk/jfr/api/setting/TestSettingControl.java 
> 
> **Out of scope:**
> - Formatting issues
> - Wider use of the TimespanUnit enum
> - Clarify the SettingControl specification on how invalid values should be handled

Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision:

  Simplify implementation of BooleanSetting::combine

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/20336/files
  - new: https://git.openjdk.org/jdk/pull/20336/files/01488537..76203942

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=20336&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20336&range=00-01

  Stats: 13 lines in 1 file changed: 3 ins; 7 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/20336.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20336/head:pull/20336

PR: https://git.openjdk.org/jdk/pull/20336


More information about the hotspot-jfr-dev mailing list