On Tue, 8 Dec 2020 16:31:37 GMT, Erik Gahlin <egah...@openjdk.org> wrote:
>> 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? Configuring the throttler implementation in native is a bit involved. With the existing order, with enabled first, events are enabled before subsequent conditions are set up. For the throttler, it means as soon as the enabled setting is flipped, the throttler gets traffic, but its not yet configured to accept it. It has a default, which is off, meaning it accepts all events until the subsequent call to configure the throttler which can take some time, because the setup is non-trivial. It was found that rates are not respected because of the throttler not having been setup yet when its starts to get traffic. > 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 "/". Fixed. > 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. Fixed. > 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 Fixed. > 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. Fixed. > 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. Fixed. > 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" New control elements and attributes introduced. ------------- PR: https://git.openjdk.java.net/jdk/pull/1624
