On Wed, 9 Dec 2020 20:58:48 GMT, Markus Grönlund <mgron...@openjdk.org> wrote:
>> Greetings, >> >> please help review this enhancement to let JFR sample object allocations by >> default. >> >> A description is provided in the JIRA issue. >> >> Thanks >> Markus > > Markus Grönlund has updated the pull request incrementally with 12 additional > commits since the last revision: > > - initialization check > - thread locals and detach and reattach > - Tighter ThrottleUnit > - JFC control elements > - TLAB include > - ThrottleUnit enum > - remote tests > - jfc control attributes > - Sampling frequency adjustment for large objects > - Treat large objects as tlabs for sampling purposes > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/6918f0c8...4e986552 src/jdk.jfr/share/classes/jdk/jfr/internal/EventControl.java line 79: > 77: private final PlatformEventType type; > 78: private final String idName; > 79: Why move Enabled to later? src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java line 229: > 227: // Expected input format is "x/y" or "x\y" where x is a non-negative > long > 228: // and y is a time unit. Split the string at the delimiter. > 229: private static String parseThrottleString(String s, boolean value) { I think we should only support one type of slash "/". src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java line 249: > 247: } > 248: > 249: private static TimeUnit timeUnit(String unit) { This could be done with an enum with a constructor. src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThrottleSetting.java line 65: > 63: @Override > 64: public String combine(Set<String> values) { > 65: double max = OFF; Probably better to use a long (nanos) than floating number src/jdk.jfr/share/classes/jdk/jfr/internal/settings/ThrottleSetting.java line 88: > 86: @Override > 87: public void setValue(String s) { > 88: this.value = s; If parsing fails, I think things should be kept as is. At least that is what the SettingControl interface say.s I looked at other setting control and the implementation seems wrong there as well. src/jdk.jfr/share/conf/jfr/default.jfc line 618: > 616: > 617: <event name="jdk.ObjectAllocationSample"> > 618: <setting name="enabled">true</setting> I think enabled should have the "memory-profiling" control. src/jdk.jfr/share/conf/jfr/profile.jfc line 608: > 606: > 607: <event name="jdk.ObjectAllocationInNewTLAB"> > 608: <setting name="enabled" > control="memory-profiling-enabled-medium">false</setting> Need to sync this with <selection>. Perhaps a new choice are needed "Object Allocation" ------------- PR: https://git.openjdk.java.net/jdk/pull/1624
