RFR: 8257602: Introduce JFR Event Throttling and new jdk.ObjectAllocationSample event (enabled by default) [v4]

[Erik Gahlin]

On Wed, 9 Dec 2020 20:58:48 GMT, Markus Grönlund <mgronlun at 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