Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v3]

[Erik Gahlin]

On Wed, 11 May 2022 12:59:49 GMT, Magnus Ihse Bursie  wrote:

>> Adam Sotona has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   8244681: Add a warning for possibly lossy conversion in compound 
>> assignments
>>   recommended correction of the warning description
>
> make/modules/jdk.jfr/Java.gmk line 26:
> 
>> 24: #
>> 25: 
>> 26: DISABLED_WARNINGS_java += exports lossy-conversions
> 
> Note that with the fix of JDK-8286392 (and JDK-8286396) the 
> `lossy-conversions` warning should not be disabled for the JFR code. 
> 
> In general, you need to check which of the subtasks of JDK-8286374 that has 
> been fixed, and adjust the makefiles accordingly, before pushing this fix.
> 
> (In the future, it might be easier to push the fix which disables the 
> warnings first, and then file follow-up bugs on aa per-component basis, and 
> remind them to remove the disabling in the makefile. That way there won't be 
> a race between individual fixes and a "master" bug like this.)

I agree, but if it doesn't happen, I can follow up with a separate PR where I 
remove the disablement.

-

PR: https://git.openjdk.java.net/jdk/pull/8599

Re: RFR: 8244681: Add a warning for possibly lossy conversion in compound assignments [v3]

[Erik Gahlin]

On Wed, 11 May 2022 07:45:39 GMT, Adam Sotona  wrote:

>> Please review this patch adding new lint option, **lossy-conversions**, to 
>> javac to warn about type casts in compound assignments with possible lossy 
>> conversions.
>> 
>> The new lint warning is shown if the type of the right-hand operand of a 
>> compound assignment is not assignment compatible with the type of the 
>> variable.
>> 
>> The implementation of the warning is based on similar check performed to 
>> emit "possible lossy conversion" compilation error for simple assignments. 
>> 
>> Proposed patch also include complex matrix-style test with positive and 
>> negative test cases of lossy conversions in compound assignments.
>> 
>> Proposed patch also disables this new lint option in all affected JDK 
>> modules and libraries to allow smooth JDK build. Individual cases to address 
>> possibly lossy conversions warnings in JDK are already addressed in a 
>> separate umbrella issue and its sub-tasks.
>> 
>> Thanks for your review,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8244681: Add a warning for possibly lossy conversion in compound assignments
>   recommended correction of the warning description

Lossy conversion issues for jdk.jfr and jdk.management.jfr. have been fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/8599

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Erik Gahlin]

On Fri, 29 Apr 2022 18:27:27 GMT, Mikhailo Seledtsov  
wrote:

>> Alan Bateman has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e
>
> test/jdk/jdk/jfr/api/consumer/TestManyRecordings.java line 57:
> 
>> 55: int classLoaderCount = Integer.parseInt(args[0]);
>> 56: int classCount = Integer.parseInt(args[1]);
>> 57: for (int i = 0; i  
> Did you mean classLoaderCount here instead of classCount? Also, please make 
> sure there is a space between < and classLoaderCount.

The JFR "tests" look strange. I would expect a test called TestManyRecording to 
start recordings, not created a lot of classes, similar to TestManyClasses. How 
is this related to Loom?  Could this be a merge gone bad?

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Erik Gahlin]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

Marked as reviewed by egahlin (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v8]

[Erik Gahlin]

On Wed, 27 Apr 2022 14:24:20 GMT, Alan Bateman  wrote:

>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which 
>> JDK version to target.
>> 
>> We will refresh this PR periodically to pick up changes and fixes from the 
>> loom repo.
>> 
>> Most of the new mechanisms in the HotSpot VM are disabled by default and 
>> require running with `--enable-preview` to enable.
>> 
>> The patch has support for x64 and aarch64 on the usual operating systems 
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for 
>> zero and some of the other ports. Additional ports can be contributed via 
>> PRs against the fibers branch in the loom repo.
>> 
>> There are changes in many areas. To reduce notifications/mails, the labels 
>> have been trimmed down for now to hotspot, serviceability and core-libs. 
>> We'll add the complete set of labels when the PR is further along.
>> 
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from 
>> Doug Lea's CVS. These changes will probably be proposed and integrated in 
>> advance of this PR.
>> 
>> The changes include some non-exposed and low-level infrastructure to support 
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to 
>> make life a bit easier and avoid having to separate VM changes and juggle 
>> branches at this time.
>
> Alan Bateman has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refresh 7965cc6b168e567ac2596f2fbc3b00a7d99b7e1e

src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp line 168:

> 166:   assert(!tl->is_excluded(), "invariant");
> 167:   if (virtual_thread) {
> 168: // TODO: blob cache for virtual threads

Fix this now or after integration?

src/hotspot/share/jfr/metadata/metadata.xml line 121:

> 119: thread="true" stackTrace="true">
> 120:  description="Thread enlisted as a carrier" />
> 121:  description="Class of the continuation" />

"Continuation class" = >" Continuation Class"
numFrames = frames
numRefs = references
"Number of interpreted frames" => "Interpreted Frames"
"Number of references" => "References"
"Stack size in bytes" => "Stack Size" contentType"bytes"

src/hotspot/share/jfr/metadata/metadata.xml line 130:

> 128: thread="true" stackTrace="true">
> 129:  description="Thread enlisted as a carrier" />
> 130:  description="Class of the continuation" />

contClass => continuationClass
"Continuation class" => "Continuation Class"
"Class of the continuation" Remove (not needed)
"Number of interpreted frames" => "Interpreted Frames"
numFrames => frames
"Number of references" => "References"
numRefs => references
"Stack size in bytes" => "Stack Size" contentType="bytes"

src/hotspot/share/jfr/metadata/metadata.xml line 138:

> 136:category="Java Virtual Machine" label="Continuation Freeze Young" 
> thread="true" stackTrace="false" startTime="false">
> 137: 
> 138: 

"Allocated new" => "Allocated New"

src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp line 94:

> 92: static const size_t global_buffer_size = 512 * K;
> 93: 
> 94: static const size_t thread_local_buffer_prealloc_count = 32;

Why is more memory needed? Moore's law or something specific to virtual threads?

src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointWriter.cpp line 81:

> 79:   be_writer.write(size);
> 80:   be_writer.write(time);
> 81:   be_writer.write(JfrTicks::now().value() - time);

Why is this changed?

src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadEndEvent.java line 35:

> 33: 
> 34: @Category({"Java Runtime"})
> 35: @Label("Virtual thread end")

"Virtual thread end" => "Virtual Thread End"
Remove description.

src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadPinnedEvent.java line 35:

> 33: 
> 34: @Category({"Java Runtime"})
> 35: @Label("Virtual thread pinned")

"Virtual thread pinned" => "Virtual Thread Pinned"
Remove description

src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadStartEvent.java line 35:

> 33: 
> 34: @Category({"Java Runtime"})
> 35: @Label("Virtual thread start")

"virtual thread start" => "Virtual Thread Start"
Remove description

src/jdk.jfr/share/classes/jdk/jfr/events/VirtualThreadSubmitFailedEvent.java 
line 35:

> 33: 
> 34: @Category({"Java Runtime"})
> 35: @Label("Virtual thread submit task failed")

The label is a bit a long, would "Virtual Thread Submit Failed" work?

-

PR: https://git.openjdk.java.net/jdk/pull/8166

Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19 [v3]

[Erik Gahlin]

On Wed, 30 Mar 2022 02:56:41 GMT, Vicente Romero  wrote:

>> Please review this PR which is updating the ASM included in the JDK to ASM 
>> 9.2. This version should be included in JDK19. There are basically two 
>> commits here, one that was automatically generated and that mostly changes 
>> file headers etc and another one, smaller, that make changes to the code to 
>> adapt it to our needs, JDK version etc. In this second commit one can see 
>> that two classes that were removed by the automatically generated change 
>> have been copied back:
>> 
>> jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter
>> jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter
>> 
>> This is because package: `jdk.jfr.internal.instrument` needs them.
>> 
>> TIA
>
> Vicente Romero has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fixing files missing new line at the end

JFR parts look OK.

-

Marked as reviewed by egahlin (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8000

Re: RFR: JDK-8280157: wrong texts Falied in a couple of tests

[Erik Gahlin]

On Wed, 19 Jan 2022 09:19:00 GMT, Matthias Baesken  wrote:

> Very small change fixing wrong strings "Falied" in a couple of tests.

Marked as reviewed by egahlin (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/7142

Re: RFR: 8280035: Use Class.isInstance instead of Class.isAssignableFrom where applicable

[Erik Gahlin]

On Thu, 13 Jan 2022 08:39:04 GMT, Andrey Turbanov  wrote:

>> Method `Class.isAssignableFrom` is often used in form of:
>> 
>> if (clazz.isAssignableFrom(obj.getClass())) {
>> Such condition could be simplified to more shorter and performarnt code
>> 
>> if (clazz.isInstance(obj)) {
>> 
>> Replacement is equivalent if it's known that `obj != null`.
>> In JDK codebase there are many such places.
>> 
>> I tried to measure performance via JMH
>> 
>> Class integerClass = Integer.class;
>> Class numberClass = Number.class;
>> 
>> Object integerObject = 45666;
>> Object doubleObject = 8777d;
>> 
>> @Benchmark
>> public boolean integerInteger_isInstance() {
>> return integerClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean integerInteger_isAssignableFrom() {
>> return integerClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isInstance() {
>> return integerClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean integerDouble_isAssignableFrom() {
>> return integerClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isInstance() {
>> return numberClass.isInstance(doubleObject);
>> }
>> 
>> @Benchmark
>> public boolean numberDouble_isAssignableFrom() {
>> return numberClass.isAssignableFrom(doubleObject.getClass());
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isInstance() {
>> return numberClass.isInstance(integerObject);
>> }
>> 
>> @Benchmark
>> public boolean numberInteger_isAssignableFrom() {
>> return numberClass.isAssignableFrom(integerObject.getClass());
>> }
>> 
>> @Benchmark
>> public void numberIntegerDouble_isInstance(Blackhole bh) {
>> bh.consume(numberClass.isInstance(integerObject));
>> bh.consume(numberClass.isInstance(doubleObject));
>> }
>> 
>> @Benchmark
>> public void integerIntegerDouble_isAssignableFrom(Blackhole bh) {
>> bh.consume(integerClass.isAssignableFrom(integerObject.getClass()));
>> bh.consume(integerClass.isAssignableFrom(doubleObject.getClass()));
>> }
>> 
>> `isInstance` is a bit faster than `isAssignableFrom`
>> 
>> Benchmark  Mode  Cnt  Score   Error  Units
>> integerDouble_isAssignableFrom avgt5  1,173 ± 0,026  ns/op
>> integerDouble_isInstance   avgt5  0,939 ± 0,038  ns/op
>> integerIntegerDouble_isAssignableFrom  avgt5  2,106 ± 0,068  ns/op
>> numberIntegerDouble_isInstance avgt5  1,516 ± 0,046  ns/op
>> integerInteger_isAssignableFromavgt5  1,175 ± 0,029  ns/op
>> integerInteger_isInstance  avgt5  0,886 ± 0,017  ns/op
>> numberDouble_isAssignableFrom  avgt5  1,172 ± 0,007  ns/op
>> numberDouble_isInstanceavgt5  0,891 ± 0,030  ns/op
>> numberInteger_isAssignableFrom avgt5  1,169 ± 0,014  ns/op
>> numberInteger_isInstance   avgt5  0,887 ± 0,016  ns/op
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/TypeLibrary.java line 405:
> 
>> 403: Object res = m.invoke(a, new Object[0]);
>> 404: if (res instanceof Annotation[]) {
>> 405: for (Annotation rep : (Annotation[]) 
>> m.invoke(a, new Object[0])) {
> 
> BTW it looks suspicious to have `m.invoke(a, new Object[0])` called again 
> here. Shouldn't it just reuse `res` variable instead?

It looks unnecessary. Please feel free to fix.

-

PR: https://git.openjdk.java.net/jdk/pull/7061

Re: RFR: 8279918: Fix various doc typos [v2]

[Erik Gahlin]

On Thu, 13 Jan 2022 14:01:04 GMT, Pavel Rappo  wrote:

>> - Most of the typos are of a trivial kind: missing whitespace.
>> - If any of the typos should be fixed in the upstream projects instead, 
>> please say so; I will drop those typos from the patch.
>> - As I understand it, ` ` in ImageInputStream and DataInput is an irrelevant 
>> formatting artefact and thus can be removed.
>> - `` is an apostrophe, which does not require to be encoded.
>
> Pavel Rappo has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Additional typos

Marked as reviewed by egahlin (Reviewer).

The JFR related changes looks OK.

-

PR: https://git.openjdk.java.net/jdk/pull/7063

Re: RFR: 8277992: Add fast jdk_svc subtests to jdk:tier3

[Erik Gahlin]

On Thu, 2 Dec 2021 08:21:30 GMT, Aleksey Shipilev  wrote:

> All right then. Thanks for followups!
> 
> @egahlin, @mgronlun: this affects JFR tests a bit, subsumes `jfr_tier_3` into 
> `tier3`, basically. Are you good with this?

Seems reasonable.

I'm reluctant to become (R)eviewer on this, since this is basically the first 
time I have looked at the TEST.groups file. There must be somebody who knows 
this better than me and that can become a second reviewer.

-

PR: https://git.openjdk.java.net/jdk/pull/6619

Re: RFR: 8266936: Add a finalization JFR event [v10]

[Erik Gahlin]

On Fri, 27 Aug 2021 15:23:35 GMT, Markus Grönlund  wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to 
>> replace and mitigate Object.finalize() uses in the JDK libraries; please see 
>> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK 
>> code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which 
>> classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - localize
>  - cleanup
>  - FinalizerStatistics

Marked as reviewed by egahlin (Reviewer).

src/java.base/share/classes/java/lang/ref/Finalizer.java line 71:

> 69: }
> 70: 
> 71: 

extraneous whitespace?

test/jdk/jdk/jfr/event/runtime/TestFinalizerStatisticsEvent.java line 98:

> 96:   case TEST_CLASS_NAME: {
> 97:   
> Asserts.assertTrue(event.getString("codeSource").startsWith("file://"));
> 98:   foundTestClassName = true;

Could we (sanity) check "objects" and "totalFinalzersRun" fields as well?

-

PR: https://git.openjdk.java.net/jdk/pull/4731

Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]

[Erik Gahlin]

On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung  wrote:

>> This reimplements core reflection with method handles.
>> 
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation 
>> uses `MethodHandle`.  For `Field` accessor, the new implementation uses 
>> `VarHandle`.For the first few invocations of one of these reflective 
>> methods on a specific reflective object we invoke the corresponding method 
>> handle directly. After that we spin a dynamic bytecode stub defined in a 
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its 
>> class data as a dynamically computed constant. Loading the method handle 
>> from a constant allows JIT to inline the method-handle invocation in order 
>> to achieve good performance.
>> 
>> The VM's native reflection methods are needed during early startup, before 
>> the method-handle mechanism is initialized. That happens soon after 
>> System::initPhase1 and before System::initPhase2, after which we switch to 
>> using method handles exclusively.
>> 
>> The core reflection and method handle implementation are updated to handle 
>> chained caller-sensitive method calls [1] properly. A caller-sensitive 
>> method can define with a caller-sensitive adapter method that will take an 
>> additional caller class parameter and the adapter method will be annotated 
>> with `@CallerSensitiveAdapter` for better auditing.   See the detailed 
>> description from [2].
>> 
>> Ran tier1-tier8 tests.   
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2] 
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix left-over assignment

Change in test/jdk/jdk/jfr/api/consumer/TestHiddenMethod.java looks good.

-

Marked as reviewed by egahlin (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/5027

Integrated: 8272515: JFR: Names should only be valid Java identifiers

[Erik Gahlin]

On Wed, 8 Sep 2021 13:00:17 GMT, Erik Gahlin  wrote:

> Hi,
> 
> Could I have a review of change that prevents invalid Java identifiers or 
> type names in JFR events. For rationale and compatibility issues see the CSR 
> request. The only change to java.base is making 
> jdk.modules.internal.Checks::isJavaIdentifier(String) public, so it can be 
> reused by the jdk.jfr module.
> 
> Testing: /jdk/jdk/jfr + tier1 + tier2
> 
> Thanks
> Erik

This pull request has now been integrated.

Changeset: 48aff231
Author:Erik Gahlin 
URL:   
https://git.openjdk.java.net/jdk/commit/48aff23165db668eb9c06477d16a8e72b6dc6b56
Stats: 308 lines in 11 files changed: 259 ins; 23 del; 26 mod

8272515: JFR: Names should only be valid Java identifiers

Reviewed-by: mgronlun

-

PR: https://git.openjdk.java.net/jdk/pull/5415

RFR: 8272515: JFR: Names should only be valid Java identifiers

[Erik Gahlin]

Hi,

Could I have a review of change that prevents invalid Java identifiers or type 
names in JFR events. For rationale and compatibility issues see the CSR 
request. The only change to java.base is making 
jdk.modules.internal.Checks::isJavaIdentifier(String) public, so it can be 
reused by the jdk.jfr module.

Testing: /jdk/jdk/jfr + tier1 + tier2

Thanks
Erik

-

Commit messages:
 - Improve guard against invalid type
 - Remove unused imports
 - Initial

Changes: https://git.openjdk.java.net/jdk/pull/5415/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5415=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8272515
  Stats: 308 lines in 11 files changed: 259 ins; 23 del; 26 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5415.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5415/head:pull/5415

PR: https://git.openjdk.java.net/jdk/pull/5415

Re: RFR: 8266936: Add a finalization JFR event [v2]

[Erik Gahlin]

On Tue, 18 May 2021 22:41:06 GMT, Brent Christian  wrote:

>> Please review this enhancement to add a new JFR event, generated whenever a 
>> finalizer is run.
>> (The makeup is similar to the Deserialization event, 
>> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).)
>> 
>> The event's only datum (beyond those common to all jfr events) is the class 
>> of the object that was finalized.
>> 
>> The Category for the event:
>> `"Java Virtual Machine" / "GC" / "Finalization"`
>> is what made sense to me, even though the event is generated from library 
>> code.
>> 
>> Along with the new regtest, I added a run mode to the basic finalizer test 
>> to enable jfr.
>> Automated testing looks good so far.
>> 
>> Thanks,
>> -Brent
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Test flag should be volatile

> I was also thinking if this event should be implemented in the VM in order to 
> avoid some performance overhead such as object allocation. Erik, what is the 
> benefit of implementing in in the VM?

No startup cost, no allocation and there are callbacks when a class gets 
unloaded, so it's probably easier to clear any table where the statistics is 
held.

-

PR: https://git.openjdk.java.net/jdk/pull/4101

Re: RFR: 8203359: Container level resources events [v9]

[Erik Gahlin]

On Wed, 21 Apr 2021 22:38:32 GMT, Erik Gahlin  wrote:

>> Jaroslav Bachorik has refreshed the contents of this pull request, and 
>> previous commits have been removed. The incremental views will show 
>> differences compared to the previous content of the PR.
>
> I wonder if something similar to below could be added to 
> jdk.jfr.internal.Utils:
> 
> private static Metrics[] metrics;
> public static Metrics getMetrics() {
> if (metrics == null) {
> metrics = new Metrics[] { Metrics.systemMetrics() };
> }
> return metrics[0];
> }
> 
> public static boolean shouldSkipBytecode(String eventName, Class 
> superClass) {
> if (superClass != AbstractJDKEvent.class) {
> return false;
> }
> if (!eventName.startsWith("jdk.Container")) {
> return false;
> }
> return getMetrics() == null;
> }
> 
> Then we could add checks to 
> jdk.jfr.internal.JVMUpcalls::bytesForEagerInstrumentation(...)
> 
> eventName = ei.getEventName();
> if (Utils.shouldSkipBytecode(eventName, superClass))) {
> return oldBytes;
> }
> 
> and jdk.jfr.internal.JVMUpcalls:onRetransform(...)
> 
> if (jdk.internal.event.Event.class.isAssignableFrom(clazz) && 
> !Modifier.isAbstract(clazz.getModifiers())) {
> if (Utils.shouldSkipBytecode(clazz.getName(), clazz.getSuperclass())) 
> {
> return oldBytes;
> }
> 
> This way we would not pay for generating bytecode for events in a 
> non-container environment. 
> 
> Not sure if it works, but could perhaps make startup faster? We would still 
> pay for generating the event handlers during registration, but it's much 
> trickier to avoid since we need to store the event type somewhere.

> @egahlin Unfortunately, I had to make one late change in the periodic event 
> hook registration.
> If the events are registered conditionally only when running in a container 
> the event metadata are not correct and `TestDefaultConfigurations.java` test 
> will fail. When I register the hooks unconditionally, the metadata is 
> correctly generated and the test passes.
> I will hold off integration until I hear back from you whether this is 
> acceptable or I should try to find an alternative solution.

I think we should always register the metadata, even if you can't get the 
event. 

That's how we handle different GCs. Users must be able to explore events even 
if they can't use them. For example, you must be able to configure container 
events in JMC (with correct labels/descriptions) without actually connecting to 
a JVM running in a Docker container.

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8266936: Add a finalization JFR event [v2]

[Erik Gahlin]

On Tue, 18 May 2021 22:41:06 GMT, Brent Christian  wrote:

>> Please review this enhancement to add a new JFR event, generated whenever a 
>> finalizer is run.
>> (The makeup is similar to the Deserialization event, 
>> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).)
>> 
>> The event's only datum (beyond those common to all jfr events) is the class 
>> of the object that was finalized.
>> 
>> The Category for the event:
>> `"Java Virtual Machine" / "GC" / "Finalization"`
>> is what made sense to me, even though the event is generated from library 
>> code.
>> 
>> Along with the new regtest, I added a run mode to the basic finalizer test 
>> to enable jfr.
>> Automated testing looks good so far.
>> 
>> Thanks,
>> -Brent
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Test flag should be volatile

I wonder if there needs to be one event per finalized object?

Perhaps a counter per class would be as useful, i.e. 
jdk.FinalizationStatistics, and if it could be implemented in the VM, without 
other implications, that would be great. 

Such an event could be enabled by default and provide much greater value than 
an event that users would need to know about and configure themselves, which 
99,99% of all user will not do.

-

PR: https://git.openjdk.java.net/jdk/pull/4101

Re: RFR: 8266936: Add a finalization JFR event [v2]

[Erik Gahlin]

On Tue, 18 May 2021 22:41:06 GMT, Brent Christian  wrote:

>> Please review this enhancement to add a new JFR event, generated whenever a 
>> finalizer is run.
>> (The makeup is similar to the Deserialization event, 
>> [JDK-8261160](https://bugs.openjdk.java.net/browse/JDK-8261160).)
>> 
>> The event's only datum (beyond those common to all jfr events) is the class 
>> of the object that was finalized.
>> 
>> The Category for the event:
>> `"Java Virtual Machine" / "GC" / "Finalization"`
>> is what made sense to me, even though the event is generated from library 
>> code.
>> 
>> Along with the new regtest, I added a run mode to the basic finalizer test 
>> to enable jfr.
>> Automated testing looks good so far.
>> 
>> Thanks,
>> -Brent
>
> Brent Christian has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Test flag should be volatile

This looks useful, but I am worried about the performance impact:

- The added allocation for every object that is finalized. 
- The event being enabled in the default configuration. 

The default configuration must be safe to use even in pathological cases, i.e. 
an application with lots of objects being finalized. It's probably too much 
overhead (or number of events) for the profile configuration as well.

There is also the added startup cost of JFR. One event may not matter that 
much, but they all add up. We need to put in place a mechanism that doesn't 
rely on bytecode instrumentation at runtime. There is an enhancement for that, 
but it requires some engineering first.

-

PR: https://git.openjdk.java.net/jdk/pull/4101

Re: RFR: 8203359: Container level resources events [v11]

[Erik Gahlin]

On Mon, 3 May 2021 13:16:06 GMT, Jaroslav Bachorik  
wrote:

>> With this change it becomes possible to surface various cgroup level metrics 
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>> 
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>> 
>> For each of those subsystems a configuration data will be emitted as well. 
>> The initial proposal is to emit the configuration data events at least once 
>> per chunk and the metrics values at 30 seconds interval. 
>> By using these values the emitted events seem to contain useful information 
>> without increasing overhead (the metrics values are read from `/proc` 
>> filesystem so that should not be done too frequently).
>
> Jaroslav Bachorik has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains 10 commits:
> 
>  - Small fixes
>  - Remove trailing spaces
>  - Doh
>  - Report container type and register events conditionally
>  - Remove unused test files
>  - Initial test support for JFR container events
>  - Update the JFR control files
>  - Split off the CPU throttling metrics
>  - Formatting spaces
>  - 8203359: Container level resources events

Looks good, but if there are test issues they should be fixed.

-

Marked as reviewed by egahlin (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v10]

[Erik Gahlin]

On Thu, 22 Apr 2021 16:00:32 GMT, Jaroslav Bachorik  
wrote:

>> With this change it becomes possible to surface various cgroup level metrics 
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>> 
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>> 
>> For each of those subsystems a configuration data will be emitted as well. 
>> The initial proposal is to emit the configuration data events at least once 
>> per chunk and the metrics values at 30 seconds interval. 
>> By using these values the emitted events seem to contain useful information 
>> without increasing overhead (the metrics values are read from `/proc` 
>> filesystem so that should not be done too frequently).
>
> Jaroslav Bachorik has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 13 additional 
> commits since the last revision:
> 
>  - Prevent event container bytecode generation if no container present
>  - Fix event metadata
>  - Roll back conditional registration of container events
>  - Remove container events flag
>  - Remove trailing spaces
>  - Doh
>  - Report container type and register events conditionally
>  - Remove unused test files
>  - Initial test support for JFR container events
>  - Update the JFR control files
>  - ... and 3 more: 
> https://git.openjdk.java.net/jdk/compare/904d9495...04c3f092

src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java line 729:

> 727: 
> 728: public static boolean shouldSkipBytecode(String eventName, Class 
> superClass) {
> 729: if 
> (!superClass.getName().equals("jdk.jfr.events.AbstractJDKEvent")) {

Was there a problem checking against the class instance? If so, perhaps you 
could add a check that the class is in the boot class loader (null).

src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java line 737:

> 735: private static Metrics getMetrics() {
> 736: if (metrics == null) {
> 737: metrics = Metrics.systemMetrics();

Will this not lead to a lookup every time in an non-container environment?

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v9]

[Erik Gahlin]

On Wed, 21 Apr 2021 13:34:28 GMT, Jaroslav Bachorik  
wrote:

>> With this change it becomes possible to surface various cgroup level metrics 
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>> 
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>> 
>> For each of those subsystems a configuration data will be emitted as well. 
>> The initial proposal is to emit the configuration data events at least once 
>> per chunk and the metrics values at 30 seconds interval. 
>> By using these values the emitted events seem to contain useful information 
>> without increasing overhead (the metrics values are read from `/proc` 
>> filesystem so that should not be done too frequently).
>
> Jaroslav Bachorik has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix event metadata

I wonder if something similar to below could be added to jdk.jfr.internal.Utils:

private static Metrics[] metrics;
public static Metrics getMetrics() {
if (metrics == null) {
metrics = new Metrics[] { Metrics.systemMetrics() };
}
return metrics[0];
}

public static boolean shouldSkipBytecode(String eventName, Class 
superClass) {
if (superClass.getClassLoader() != null) {
return false;
}
if (!eventName.startsWith("jdk.Container")) {
return false;
}
return getMetrics() == null;
}

Then we could add checks to 
jdk.jfr.internal.JVMUpcalls::bytesForEagerInstrumentation(...)

eventName = ei.getEventName();
if (Utils.shouldSkipBytecode(eventName, superClass))) {
return oldBytes;
}

and jdk.jfr.internal.JVMUpcalls:onRetransform(...)

if (jdk.internal.event.Event.class.isAssignableFrom(clazz) && 
!Modifier.isAbstract(clazz.getModifiers())) {
if (Utils.shouldSkipBytecode(clazz.getName(), clazz.getSuperclass())) {
return oldBytes;
}

This way we would not pay for generating bytecode for events in a non-container 
environment. 

Not sure if it works, but could perhaps make startup faster? We would still pay 
for generating the event handlers during registration, but it's much trickier 
to avoid since we need to store the event type somewhere.

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v8]

[Erik Gahlin]

On Wed, 14 Apr 2021 14:32:32 GMT, Severin Gehwolf  wrote:

>> This is taken as reported by cgroups - I didn't want to change the semantics 
>> so it does not confuse people familiar with cgroups.
>
> I don't see this event field being set anywhere? Which Metrics API call is 
> this using?

How does it look in proc?

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v7]

[Erik Gahlin]

On Wed, 14 Apr 2021 08:28:01 GMT, Jaroslav Bachorik  
wrote:

>> src/jdk.jfr/share/conf/jfr/default.jfc line 1051:
>> 
>>> 1049:   false
>>> 1050: 
>>> 1051:   true
>> 
>> I don't think we should create "flag" for "Container Events". Instead we 
>> should treat them like CPU and other OS events, always on. Since JFR can be 
>> used outside a container, it seems wrong to have this as an option.
>
> Ok. So what would be a good rule-of-thumb for when one should introduce a 
> flag?

I think we want to limit the number flags/options There are already too many, 
preferably we would have none, but some of the ones we have today, like gc and 
exception, are necessary, because the impact of have them on by default would 
be catastrophic (long stop the world pauses and possibly million of events per 
second).

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v8]

[Erik Gahlin]

On Wed, 14 Apr 2021 12:14:33 GMT, Jaroslav Bachorik  
wrote:

>> src/jdk.jfr/share/classes/jdk/jfr/events/ContainerCPUThrottlingEvent.java 
>> line 44:
>> 
>>> 42: @Category({"Operating System", "Container", "Processor"})
>>> 43: @Description("Container CPU throttling related information.")
>>> 44: public class ContainerCPUThrottlingEvent extends AbstractJDKEvent {
>> 
>> I wonder if we should put all the container events in the same category 
>> {"Operating System", "Container"}, Or possibly add them under the already 
>> existing categories under "Operating System"?
>
> I guess we could fit those events under `Operating System/Memory` and 
> `Operating System/Processor` categories.
> What about I/O? Currently, there is only `Operating System/Network` category. 
> The options are:
> * Add `Operating System/IO` category and move `Network` to `Operating 
> System/IO/Network`
> * Add `Operation System/FileSystem` category and move the container IO event 
> there
> 
> What would you prefer?

I prefer adding File System (or having separate container category).

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Integrated: 8265036: JFR: Remove use of -XX:StartFlightRecording= and -XX:FlightRecorderOptions=

[Erik Gahlin]

On Sun, 18 Apr 2021 15:17:35 GMT, Erik Gahlin  wrote:

> Hi,
> 
> Could I have a review of fix that removes the use of "=" together with 
> -XX:StartFlightRecording and -XX:FlightRecorderOptions. It's been possible to 
> use "-XX:StartFlightRecording:" and "-XX:FlightRecorderOption:" since JFR was 
> introduced into OpenJDK (JDK 11), so this is not a change of the 
> specification, just an update to make the use consistent in tests, comments, 
> documentation etc.
> 
> I also removed the use of -XX:+FlightRecorder, which is not needed, and has 
> been deprecated since JDK 13. 
> 
> Testing: jdk/jdk/jfr, tier 1-4.
> 
> Thanks
> Erik

This pull request has now been integrated.

Changeset: 4dcaac1f
Author:Erik Gahlin 
URL:   https://git.openjdk.java.net/jdk/commit/4dcaac1f
Stats: 104 lines in 40 files changed: 0 ins; 0 del; 104 mod

8265036: JFR: Remove use of -XX:StartFlightRecording= and 
-XX:FlightRecorderOptions=

Reviewed-by: cjplummer

-

PR: https://git.openjdk.java.net/jdk/pull/3561

Re: RFR: 8265036: JFR: Remove use of -XX:StartFlightRecording= and -XX:FlightRecorderOptions= [v2]

[Erik Gahlin]

On Mon, 19 Apr 2021 20:16:59 GMT, Chris Plummer  wrote:

> The changes look good. Have you considered doing a test run where the use of 
> `=` and `XX:+FlightRecorder` are disallowed just to make sure you caught them 
> all?

It's a good idea, but it requires changes outside OpenJDK which I rather not do 
now. I'm sure somebody will reintroduce '=', so this is not so much about 
getting rid of them all, but to avoid them being propagated, when code is copy 
pasted for new tests etc.

-

PR: https://git.openjdk.java.net/jdk/pull/3561

Re: RFR: 8265036: JFR: Remove use of -XX:StartFlightRecording= and -XX:FlightRecorderOptions= [v2]

[Erik Gahlin]

> Hi,
> 
> Could I have a review of fix that removes the use of "=" together with 
> -XX:StartFlightRecording and -XX:FlightRecorderOptions. It's been possible to 
> use "-XX:StartFlightRecording:" and "-XX:FlightRecorderOption:" since JFR was 
> introduced into OpenJDK (JDK 11), so this is not a change of the 
> specification, just an update to make the use consistent in tests, comments, 
> documentation etc.
> 
> I also removed the use of -XX:+FlightRecorder, which is not needed, and has 
> been deprecated since JDK 13. 
> 
> Testing: jdk/jdk/jfr, tier 1-4.
> 
> Thanks
> Erik

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

  Replace '=' in jfrOptionsSet.cpp

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3561/files
  - new: https://git.openjdk.java.net/jdk/pull/3561/files/4ce08939..138bac16

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=3561=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3561=00-01

  Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3561.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3561/head:pull/3561

PR: https://git.openjdk.java.net/jdk/pull/3561

RFR: 8265036: JFR: Remove use of -XX:StartFlightRecording= and -XX:FlightRecorderOptions=

[Erik Gahlin]

Hi,

Could I have a review of fix that removes the use of "=" together with 
-XX:StartFlightRecording and -XX:FlightRecorderOptions. It's been possible to 
use "-XX:StartFlightRecording:" and "-XX:FlightRecorderOption:" since JFR was 
introduced into OpenJDK (JDK 11), so this is not a change of the specification, 
just an update to make the use consistent in tests, comments, documentation etc.

I also removed the use of -XX:+FlightRecorder, which is not needed, and has 
been deprecated since JDK 13. 

Testing: jdk/jdk/jfr, tier 1-4.

Thanks
Erik

-

Commit messages:
 - Initial

Changes: https://git.openjdk.java.net/jdk/pull/3561/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3561=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265036
  Stats: 97 lines in 39 files changed: 0 ins; 0 del; 97 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3561.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3561/head:pull/3561

PR: https://git.openjdk.java.net/jdk/pull/3561

Re: RFR: 8203359: Container level resources events [v8]

[Erik Gahlin]

On Wed, 14 Apr 2021 08:31:55 GMT, Jaroslav Bachorik  
wrote:

>> With this change it becomes possible to surface various cgroup level metrics 
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>> 
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>> 
>> For each of those subsystems a configuration data will be emitted as well. 
>> The initial proposal is to emit the configuration data events at least once 
>> per chunk and the metrics values at 30 seconds interval. 
>> By using these values the emitted events seem to contain useful information 
>> without increasing overhead (the metrics values are read from `/proc` 
>> filesystem so that should not be done too frequently).
>
> Jaroslav Bachorik has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 11 additional 
> commits since the last revision:
> 
>  - Roll back conditional registration of container events
>  - Remove container events flag
>  - Remove trailing spaces
>  - Doh
>  - Report container type and register events conditionally
>  - Remove unused test files
>  - Initial test support for JFR container events
>  - Update the JFR control files
>  - Split off the CPU throttling metrics
>  - Formatting spaces
>  - ... and 1 more: 
> https://git.openjdk.java.net/jdk/compare/1f93be70...67a61bd7

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerCPUThrottlingEvent.java line 
44:

> 42: @Category({"Operating System", "Container", "Processor"})
> 43: @Description("Container CPU throttling related information.")
> 44: public class ContainerCPUThrottlingEvent extends AbstractJDKEvent {

I wonder if we should put all the container events in the same category 
{"Operating System", "Container"}, Or possibly add them under the already 
existing categories under "Operating System"?

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerCPUThrottlingEvent.java line 
46:

> 44: public class ContainerCPUThrottlingEvent extends AbstractJDKEvent {
> 45:   @Label("CPU Elapsed Slices")
> 46:   @Description("Number of time-slice periods that have elapsed if a CPU 
> quota has been setup for the container.")

If the description is one sentence, period should not be included.

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerCPUUsageEvent.java line 46:

> 44: public class ContainerCPUUsageEvent extends AbstractJDKEvent {
> 45:   @Label("CPU Time")
> 46:   @Description("Aggregate time, in nanoseconds, consumed by all tasks in 
> the container.")

We usually skip the unit "nanoseconds" in descriptions when the field has a 
content type that describes the unit.

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerConfigurationEvent.java line 
45:

> 43: @Description("A set of container specific attributes.")
> 44: public final class ContainerConfigurationEvent extends AbstractJDKEvent {
> 45: @Label("Container type")

Capitalize "type" in the label

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerConfigurationEvent.java line 
78:

> 76: 
> 77: @Label("Memory and Swap Limit")
> 78: @Description("Maximum amount of physical memory and swap space, in 
> bytes, that can be allocated in the container.")

No need to mention bytes in the description when the field has DataAmount 
annotation.

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerIOUsageEvent.java line 47:

> 45: public class ContainerIOUsageEvent extends AbstractJDKEvent {
> 46: 
> 47:   @Label("BlkIO Request Count")

Change to "Block IO"

src/jdk.jfr/share/classes/jdk/jfr/events/ContainerMemoryUsageEvent.java line 46:

> 44: public final class ContainerMemoryUsageEvent extends AbstractJDKEvent {
> 45: @Label("Memory Pressure")
> 46: @Description("(attempts per second * 1000), if enabled, that the 
> operating system tries to satisfy a memory request for any " +

This unit seems a bit strange. Do we really need to multiply by 1000?

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v7]

[Erik Gahlin]

On Wed, 14 Apr 2021 08:28:37 GMT, Jaroslav Bachorik  
wrote:

>> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 
>> 163:
>> 
>>> 161: private static void initializeContainerEvents() {
>>> 162: containerMetrics = Container.metrics();
>>> 163: if (containerMetrics != null) {
>> 
>> I understand this will reduce startup time, but it's contrary to how we 
>> treat other events. 
>> 
>> We register events, even if they can't be used. We want users to see what 
>> events are available (and their metadata) and use JMC recording wizard or 
>> other means to configure a .jfc file without actually being connected to a 
>> containerized process. We want the same events to minimize (subtle) platform 
>> dependent bugs.
>> 
>> I think we should try to find other means to reduce the startup time. It's 
>> better to have consistent behaviour, but an initial implementation than 
>> isn't as performant, than inconsistent behavior and somewhat faster 
>> implementation.
>> 
>> At some point we will need to address the startup cost of registering Java 
>> events anyway. For example, we could generate metadata at build time in a 
>> binary format, similar to what we already do with native events. Could even 
>> be the same file. Then we can have hundreds of Java events without the cost 
>> of reflection and unnecessary class loading at startup. We could add a 
>> simple check so that bytecode for the container events (commit() etc) are 
>> not generated unless in a container environment. A couple of (cached) checks 
>> in JVMUpcalls may be sufficient to prevent instrumentation cost.
>
> Right. So, for the initial drop I will just leave the container events 
> registered unconditionally.

I think that is fine.

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events [v7]

[Erik Gahlin]

On Fri, 2 Apr 2021 12:33:03 GMT, Jaroslav Bachorik  
wrote:

>> With this change it becomes possible to surface various cgroup level metrics 
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>> 
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>> 
>> For each of those subsystems a configuration data will be emitted as well. 
>> The initial proposal is to emit the configuration data events at least once 
>> per chunk and the metrics values at 30 seconds interval. 
>> By using these values the emitted events seem to contain useful information 
>> without increasing overhead (the metrics values are read from `/proc` 
>> filesystem so that should not be done too frequently).
>
> Jaroslav Bachorik has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove trailing spaces

src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 163:

> 161: private static void initializeContainerEvents() {
> 162: containerMetrics = Container.metrics();
> 163: if (containerMetrics != null) {

I understand this will reduce startup time, but it's contrary to how we treat 
other events. 

We register events, even if they can't be used. We want users to see what 
events are available (and their metadata) and use JMC recording wizard or other 
means to configure a .jfc file without actually being connected to a 
containerized process. We want the same events to minimize (subtle) platform 
dependent bugs.

I think we should try to find other means to reduce the startup time. It's 
better to have consistent behaviour, but an initial implementation than isn't 
as performant, than inconsistent behavior and somewhat faster implementation.

At some point we will need to address the startup cost of registering Java 
events anyway. For example, we could generate metadata at build time in a 
binary format, similar to what we already do with native events. Could even be 
the same file. Then we can have hundreds of Java events without the cost of 
reflection and unnecessary class loading at startup. We could add a simple 
check so that bytecode for the container events (commit() etc) are not 
generated unless in a container environment. A couple of (cached) checks in 
JVMUpcalls may be sufficient to prevent instrumentation cost.

src/jdk.jfr/share/conf/jfr/default.jfc line 1051:

> 1049:   false
> 1050: 
> 1051:   true

I don't think we should create "flag" for "Container Events". Instead we should 
treat them like CPU and other OS events, always on. Since JFR can be used 
outside a container, it seems wrong to have this as an option.

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8203359: Container level resources events

[Erik Gahlin]

On Wed, 24 Mar 2021 18:39:06 GMT, Severin Gehwolf  wrote:

>> With this change it becomes possible to surface various cgroup level metrics 
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>> 
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is 
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>> 
>> For each of those subsystems a configuration data will be emitted as well. 
>> The initial proposal is to emit the configuration data events at least once 
>> per chunk and the metrics values at 30 seconds interval. 
>> By using these values the emitted events seem to contain useful information 
>> without increasing overhead (the metrics values are read from `/proc` 
>> filesystem so that should not be done too frequently).
>
> @jbachorik Would it make sense for `ContainerConfigurationEvent` to include 
> the underlying cgroup version info (v1 or legacy vs. v2 or unified)? 
> `Metrics.getProvider()` should give that info.

Does each getter call result in parsing /proc, or do things aggregated over 
several calls or hooks?

Do you have any data how expensive the invocations are? 

You could for example try to measure it by temporary making the events 
durational, and fetch the values between begin() and end(), and perhaps show a 
'jfr print --events Container* recording.jfr' printout. 

If possible, it would be interesting to get some idea about the startup cost as 
well

If not too much overhead, I think it would be nice to skip the "flag" in the 
.jfcs, and always record the events in a container environment.

I know there is a way to test JFR using Docker, maybe @mseledts could provide 
information? Some sanity tests would be good to have.

-

PR: https://git.openjdk.java.net/jdk/pull/3126

Re: RFR: 8261160: Add a deserialization JFR event [v5]

[Erik Gahlin]

On Fri, 12 Feb 2021 16:26:09 GMT, Chris Hegarty  wrote:

>> This issue adds a new event to improve diagnostic information of Java 
>> deserialization. The event captures the details of deserialization activity 
>> from ObjectInputStream. The event details are similar to that of the serial 
>> filter, but is agnostic of whether a filter is installed or not. The event 
>> also captures the filter status, if there is one.
>
> Chris Hegarty has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Split exception into type and message

Marked as reviewed by egahlin (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8261160: Add a deserialization JFR event

[Erik Gahlin]

On Tue, 9 Feb 2021 12:35:27 GMT, Chris Hegarty  wrote:

> This issue adds a new event to improve diagnostic information of Java 
> deserialization. The event captures the details of deserialization activity 
> from ObjectInputStream. The event details are similar to that of the serial 
> filter, but is agnostic of whether a filter is installed or not. The event 
> also captures the filter status, if there is one.

src/java.base/share/classes/java/io/ObjectInputStream.java line 1366:

> 1364: DeserializationEvent event = new DeserializationEvent();
> 1365: if (event.shouldCommit()) {
> 1366: event.filterStatus = status == null ? "n/a" : status.name();

We use null for missing value, so no need to have "n/a"

src/java.base/share/classes/java/io/ObjectInputStream.java line 1372:

> 1370: event.depth = depth;
> 1371: event.bytesRead = bytesRead;
> 1372: event.exception = Objects.toString(ex, "n/a");

You may want to change the name of the field to "exceptionMessage" to make it 
clear it's the message, not the class.

src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 45:

> 43: 
> 44: @Label ("Class")
> 45: public String clazz;

We typically use "type" when referring to a class.

src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 45:

> 43: 
> 44: @Label ("Class")
> 45: public String clazz;

Is it possible to make the field of type Class?

src/jdk.jfr/share/classes/jdk/jfr/events/DeserializationEvent.java line 51:

> 49: 
> 50: @Label ("Reference count")
> 51: public long totalObjectRefs;

"Reference count" sounds a bit like resource counter? Is that the case? If not, 
perhaps "Object References" is better. We tried to avoid abbreviations. How 
about naming the field "totalObjectReferences" or just "objectReferences"?

-

PR: https://git.openjdk.java.net/jdk/pull/2479

Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v5]

[Erik Gahlin]

On Wed, 23 Sep 2020 18:41:06 GMT, Philippe Marschall 
 wrote:

>> Hello, newbie here
>> 
>> I picked JDK-8138732 to work on because it has a "starter" label and I 
>> believe I understand what to do.
>> 
>> - I tried to update the copyright year to 2020 in every file.
>> - I decided to change `@since` from 9 to 16 since it is a new annotation 
>> name in a new package.
>> - I tried to keep code changes to a minimum, eg not change to imports if 
>> fully qualified class names are used instead of
>>   imports. In some cases I did minor reordering of imports to keep them 
>> sorted alphabetically.
>> - All tier1 tests pass.
>> - One jpackage/jlink tier2 test fails but I don't believe it is related.
>> - Some tier3 Swing tests fail but I don't think they are related.
>
> Philippe Marschall has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now
> contains one commit:
>   8138732: Rename HotSpotIntrinsicCandidate to IntrinsicCandidate

Marked as reviewed by egahlin (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/45

Re: RFR: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package [v2]

[Erik Gahlin]

On Sat, 12 Sep 2020 00:19:00 GMT, Vladimir Kozlov  wrote:

>> Philippe Marschall has refreshed the contents of this pull request, and 
>> previous commits have been removed. The
>> incremental views will show differences compared to the previous content of 
>> the PR. The pull request contains one new
>> commit since the last revision:
>>   8138732: Rename HotSpotIntrinsicCandidate to IntrinsicCandidate
>
> Marked as reviewed by kvn (Reviewer).

Have you run the JFR tests in test/jdk/jdk/jfr?

-

PR: https://git.openjdk.java.net/jdk/pull/45

Re: RFR 8242931: Few more tests that use nashorn have been missed

[Erik Gahlin]

Hi Sundar,

Will there be a replacement for Nashorn, i.e. V8 with added Java bindings?

I am asking because one of the JFR test uses Nashorn for parsing JSON and if 
another parser is coming in the near future, it could be used. instead of 
adding a parser for the test.

Thanks
Erik

> On 16 Apr 2020, at 14:59, sundararajan.athijegannat...@oracle.com wrote:
> 
> Nashorn engine removal fix (8241749: Remove the Nashorn JavaScript Engine) 
> missed updating few tests, config files.
> 
> Please review.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8242931
> 
> Webrev: http://cr.openjdk.java.net/~sundar/8242931/webrev.00/
> 
> PS I'm disabling tests by adding @ignore. I've filed bugs for the tests. 
> Please revisit port or remove these nashorn using tests
> 
> Thanks,
> 
> -Sundar
> 

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Erik Gahlin]

Yes, I can sponsor it.

Erik

> On 1 Apr 2020, at 16:43, Alan Bateman  wrote:
> 
> On 31/03/2020 17:22, Denghui Dong wrote:
>> Hi Erik,
>> 
>> IMO, one event type per pool is a better choice, because:
>> 1. easy filtering and configuration as you said, and I don't think there 
>> will be too many buffer pool types in the future
>> 2. some buffer pools may have extra fields, e.g. direct memory has max 
>> capacity limit.
>> 
>> webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.05/ 
>> 
>> diff with webrev.04: http://cr.openjdk.java.net/~ddong/8238665/v4-v5.diff 
>> 
>> 
>> 
> This update looks good to me too.
> 
> Erik, are you okay to sponsor this one?
> 

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Erik Gahlin]

Looks good. 

Erik

> On 31 Mar 2020, at 18:22, Denghui Dong  wrote:
> 
> Hi Erik,
> 
> IMO, one event type per pool is a better choice, because:
> 1. easy filtering and configuration as you said, and I don't think there will 
> be too many buffer pool types in the future
> 2. some buffer pools may have extra fields, e.g. direct memory has max 
> capacity limit.
> 
> webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.05/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.05/>
> diff with webrev.04: http://cr.openjdk.java.net/~ddong/8238665/v4-v5.diff 
> <http://cr.openjdk.java.net/~ddong/8238665/v4-v5.diff>
> 
> please review it, thanks!
> 
> Denghui Dong
> --
> From:Erik Gahlin 
> Send Time:2020年3月31日(星期二) 19:18
> To:董登辉(卓昂) 
> Cc:Alan Bateman ; hotspot-jfr-dev 
> ; core-libs-dev 
> 
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi Denghui,
> 
> I think there should be one event type per pool, or there should be a text 
> field specifying which pool it is. 
> 
> If we believe there will be many pools in the future, it may make sense for a 
> field, otherwise I think an event type could be used. The benefit of one 
> event type per pool is that there can be a description per pool and it also 
> allows easy filtering and configuration.
> 
> If there would be four pools, they could all derive from an abstract event 
> class and only add @Label and @Description per event subclass. If there would 
> be forty pools, it would be too much noise and a field would be better.
> 
> Erik
> 
> On 31 Mar 2020, at 11:06, Denghui Dong  <mailto:denghui@alibaba-inc.com>> wrote:
> 
> PING.
> 
> @Erik Could you review it? 
> 
> Denghui Dong
> 
> 
> 
> ------
> From:董登辉(卓昂)  <mailto:denghui@alibaba-inc.com>>
> Send Time:2020年3月19日(星期四) 16:35
> To:Alan Bateman mailto:alan.bate...@oracle.com>>; 
> Erik Gahlin mailto:erik.gah...@oracle.com>>
> Cc:hotspot-jfr-dev  <mailto:hotspot-jfr-...@openjdk.java.net>>; core-libs-dev 
> mailto:core-libs-dev@openjdk.java.net>>
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi,
> 
> On Mar 18, 2020, at 11:46 PM, Alan Bateman  <mailto:alan.bate...@oracle.com>> wrote:
> 
> 
> 
> On 13/03/2020 14:54, Denghui Dong wrote:
> Good suggestion, moved.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.03/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.03/>
> This looks much better.
> 
> What would you think about renaming the JFR event to 
> "DirectBufferStatistics"? The concern I have with the proposed naming is that 
> it will be really awkward to extend it to support mapped buffers.
> 
> It’s ok for me.
> 
> 
> The implementation changes look okay, hopefully Erik will skim over them. One 
> small suggestion for for ManagementFactoryHelper is that you can 
> stream().collect(Collectors.toList()) to create the value for bufferPools. 
> You could even change it to volatile so that getBufferMXBeans isn't a 
> synchronized method.
> 
> 
> Make sense, updated.
> 
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.04/>
> 
> @Erik, could you help review it?
> 
> 
> -Alan.
> 
> Denghui Dong
> 

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Erik Gahlin]

Hi Denghui,

I think there should be one event type per pool, or there should be a text 
field specifying which pool it is. 

If we believe there will be many pools in the future, it may make sense for a 
field, otherwise I think an event type could be used. The benefit of one event 
type per pool is that there can be a description per pool and it also allows 
easy filtering and configuration.

If there would be four pools, they could all derive from an abstract event 
class and only add @Label and @Description per event subclass. If there would 
be forty pools, it would be too much noise and a field would be better.

Erik

> On 31 Mar 2020, at 11:06, Denghui Dong  wrote:
> 
> PING.
> 
> @Erik Could you review it? 
> 
> Denghui Dong
> 
> 
> 
> --
> From:董登辉(卓昂) 
> Send Time:2020年3月19日(星期四) 16:35
> To:Alan Bateman ; Erik Gahlin 
> 
> Cc:hotspot-jfr-dev ; core-libs-dev 
> 
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi,
> 
> On Mar 18, 2020, at 11:46 PM, Alan Bateman  <mailto:alan.bate...@oracle.com>> wrote:
> 
> 
> 
> On 13/03/2020 14:54, Denghui Dong wrote:
> Good suggestion, moved.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.03/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.03/>
> This looks much better.
> 
> What would you think about renaming the JFR event to 
> "DirectBufferStatistics"? The concern I have with the proposed naming is that 
> it will be really awkward to extend it to support mapped buffers.
> 
> It’s ok for me.
> 
> 
> The implementation changes look okay, hopefully Erik will skim over them. One 
> small suggestion for for ManagementFactoryHelper is that you can 
> stream().collect(Collectors.toList()) to create the value for bufferPools. 
> You could even change it to volatile so that getBufferMXBeans isn't a 
> synchronized method.
> 
> 
> Make sense, updated.
> 
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.04/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.04/>
> 
> @Erik, could you help review it?
> 
> 
> -Alan.
> 
> Denghui Dong

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Erik Gahlin]

Yes, I think it is fine to export from java.base to jdk.jfr for this 
feature.


If you would add support for other buffer pools, it should not be over 
MXBeans, but using classes/interfaces that is available in java.base.


Erik

On 2020-03-05 16:19, Denghui Dong wrote:

Hi Alan and Erik,

Sorry, I'm not sure if I fully understand what you mean.
For this feature, I still think exporting jdk.internal.access to jdk.jfr is a 
good solutionsince there are
some other packages has already exported to jdk.jfr in this module.

Thanks,
Denghui Dong

--
From:Erik Gahlin 
Send Time:2020年3月5日(星期四) 19:55
To:董登辉(卓昂) 
Cc:hotspot-jfr-dev ;
core-libs-dev ; Alan Bateman

Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

We should avoid adding a dependency on java.management. The
jdk.jfr module today only depends java.base so JFR can be used in
constrained environments. All JMX related functionality is in
jdk.management.jfr.

Erik

On 5 Mar 2020, at 10:54, Denghui Dong mailto:denghui@alibaba-inc.com>> wrote:

Hi Alan,

If I use ManagementFactoryHelper to get memory pools, I still
need to modify src/java.management/share/classes/module-info.java
to export sun.management, right?

Thanks,
Denghui Dong
--
From:Alan Bateman mailto:alan.bate...@oracle.com>>
Send Time:2020年3月5日(星期四) 16:46
To:董登辉(卓昂) mailto:denghui@alibaba-inc.com>>; Erik Gahlin
mailto:erik.gah...@oracle.com>>;
hotspot-jfr-dev mailto:hotspot-jfr-...@openjdk.java.net>>; core-libs-dev
mailto:core-libs-dev@openjdk.java.net>>
Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics

On 05/03/2020 02:44, Denghui Dong wrote:
> Hi Erik,
> Updated.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/
>
ManagementFactoryHelper has a method to get the list of MXBeans for the

buffer pools. That could be easily changed to return an unmodifable
list. That would allow you to emit statistics for memory-mapped files
too (the current patch seems to be limited to emitting stats for direct

buffers).

-Alan.

Re: RFR: 8222000: JFR: Process start event

[Erik Gahlin]

Hi Roger,

Thanks for the feedback.

Runtime.exec doesn't take arg[0] as a separate parameter, so it may make 
sense to not split it up in the event. For example, if plan is to 
execute "hg clone url", but the user forgets to add "hg", then it will 
look as if "clone" is the command, which could be confusing.


I will change the field name to 'command' to make it consistent with the 
single arg form of Runtime.exec.


Regarding delimiters, I think there are two use cases. Making something 
easy for humans to read and making it unambiguous for machine parsing. 
Since comma, or comma space, might be part of an argument, it is still 
unsafe for a parser. It would be possible to add escape sequences, but 
then it would be even harder for humans to read, which I think is the 
main use case. If we in the future would add support for serializing a 
String[], we could add another field, i.e. commandArray, which would 
work for machines without special parse logic.


There is no ProcessEnd event today, but that is something that could be 
added in the future. The exit status could be useful.


Updated webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Thanks
Erik

On 2020-03-05 15:27, Roger Riggs wrote:

Hi Erik,

Would the event be more useful if the executable arg[0] was a separate 
field from the arguments?

Though I exect it depends on what is later evaluating the event fields.

If all of cmdarrray is being joined, then the event field name is more 
appropriately
called 'command';  similar to the single arg form of Runtime.exec and 
new ProcessBuilder(...).


A small point but the joining with ' ' space has the usual problem of 
trying to parse out the arguments later.

using ',' comma might be less ambiguous.  (and Arrays.toString).

Otherwise, looks fine to me.

Is there already a Process exited event? Its not quite as convenient 
to instrument but useful if it captures the exit status. (ProcessImpl 
- Windows and Linux versions).


Roger



On 3/5/20 9:10 AM, Erik Gahlin wrote:

Hi

Could I have review of a JFR event that is emitted when a process is 
started from Java.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8222000

Webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Testing:
tier1,tier2 + jdk/jdk/jfr

Thanks
Erik

RFR: 8222000: JFR: Process start event

[Erik Gahlin]

Hi

Could I have review of a JFR event that is emitted when a process is 
started from Java.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8222000

Webrev:
http://cr.openjdk.java.net/~egahlin/8222000/

Testing:
tier1,tier2 + jdk/jdk/jfr

Thanks
Erik

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Erik Gahlin]

We should avoid adding a dependency on java.management. The jdk.jfr module 
today only depends java.base so JFR can be used in constrained environments. 
All JMX related functionality is in jdk.management.jfr.

Erik

> On 5 Mar 2020, at 10:54, Denghui Dong  wrote:
> 
> Hi Alan,
> 
> If I use ManagementFactoryHelper to get memory pools, I still need to modify 
> src/java.management/share/classes/module-info.java
> to export sun.management, right?
> 
> Thanks,
> Denghui Dong
> --
> From:Alan Bateman 
> Send Time:2020年3月5日(星期四) 16:46
> To:董登辉(卓昂) ; Erik Gahlin 
> ; hotspot-jfr-dev ; 
> core-libs-dev 
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> On 05/03/2020 02:44, Denghui Dong wrote:
> > Hi Erik,
> > Updated.
> > Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/
> >
> ManagementFactoryHelper has a method to get the list of MXBeans for the 
> buffer pools. That could be easily changed to return an unmodifable 
> list. That would allow you to emit statistics for memory-mapped files 
> too (the current patch seems to be limited to emitting stats for direct 
> buffers).
> 
> -Alan.

Re: RFR: 8238665: Add JFR event for direct memory statistics

[Erik Gahlin]

Hi Denghui,

The copyright headers should have the classpath exception, similar to other 
files in the same directory.

If somebody in core-libs could review this, especially the modification of 
src/java.base/share/classes/module-info.java, I can the push this.

Thanks
Erik

> On 5 Mar 2020, at 03:44, Denghui Dong  wrote:
> 
> Hi Erik,
> Updated.
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.01/>
> 
> Thanks
> Denghui Dong
> --
> From:Erik Gahlin 
> Send Time:2020年3月5日(星期四) 06:33
> To:董登辉(卓昂) 
> Cc:hotspot-jfr-dev 
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi Denghui, 
> 
> Looking through you patch again.  
> 
> Could you add copyright headers, and change “5000 ms” to “5 s” in the .jfc 
> files. When you reply with the updated patch, could you please include 
> core-libs-dev@openjdk.java.net <mailto:core-libs-dev@openjdk.java.net> in the 
> reply since you are modifying exports in java.base.
> 
> Thanks
> Erik
> 
> 
> On 4 Mar 2020, at 20:35, Erik Gahlin  <mailto:erik.gah...@oracle.com>> wrote:
> 
> Hi Denghui, 
> 
> I have tested your fix and it seems to work. TestDefaultConfiguration takes 
> into account the event is periodic, so it doesn't complain as I expected. 
> 
> Looks good. I can sponsor this.
> 
> Thanks
> Erik
> 
> 
> On 3 Mar 2020, at 07:14, Denghui Dong  <mailto:denghui@alibaba-inc.com>> wrote:
> 
> Hi Erik,
> I agree with you that the default interval should be changed to 5s, the 
> webrev (http://cr.openjdk.java.net/~ddong/8238665/webrev.01/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.01/>) has been updated.
> No need to modify TestDefaultConfigurations.java, it is passed.
> 
> Testing
> (Linux release/slow debug build): jdk/jfr, all passed except 
> TestNativeLibrariesEvent and TestNetworkUtilizationEvent(unstable). And these 
> two test failures were not caused by this patch.
> And there were some test errors in slow debug build, I have checked the error 
> test list and confirm they are also error in slow debug build without this 
> patch, such as TestRecordedFrame.
> 
> There are two new files in this patch, should I add copyright header to them?
> 
> Thanks
> Denghui Dong
> --
> From:Erik Gahlin mailto:erik.gah...@oracle.com>>
> Send Time:2020年3月3日(星期二) 04:08
> To:董登辉(卓昂) mailto:denghui@alibaba-inc.com>>
> Cc:hotspot-jfr-dev  <mailto:hotspot-jfr-...@openjdk.java.net>>
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi Denghui,
> 
> Thanks for the explanation. 
> I think the default interval could be increased to once every 5 seconds.
> 
> Have you run the all the tests in test/jdk/jdk/jfr? I would expect 
> TestDefaultConfigurations to have failed unless modified.
> Thanks
> Erik
> On 2020-02-26 14:40, 董登辉(卓昂) wrote:
> PING: Could you review it?
> Bug: https://bugs.openjdk.java.net/browse/JDK-8238665 
> <https://bugs.openjdk.java.net/browse/JDK-8238665>
> Webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.01/>
> 
> --
> From:董登辉(卓昂)  
> <mailto:denghui@alibaba-inc.com>
> Send Time:2020年2月11日(星期二) 16:13
> To:Erik Gahlin  <mailto:erik.gah...@oracle.com>
> Cc:hotspot-jfr-dev  
> <mailto:hotspot-jfr-...@openjdk.java.net>
> Subject:Re: RFR: 8238665: Add JFR event for direct memory statistics
> 
> Hi Erik,
> 
> Thanks for the review. See answers embedded.
> 
> New webrev: http://cr.openjdk.java.net/~ddong/8238665/webrev.01/ 
> <http://cr.openjdk.java.net/~ddong/8238665/webrev.01/>
> 
> On Feb 11, 2020, at 1:52 AM, Erik Gahlin  <mailto:erik.gah...@oracle.com>> wrote:
> 
> Hi Denghiu,
> 
> Can you explain when this event can be useful, i.e. when troubleshooting x 
> etc.? This makes it easier to determine if the event should be on by default, 
> if it is worth the increased startup cost that the instrumentation creates 
> and what a reasonable period for the event should be.
> 
> Many Java middleware(e.g. Netty) and applications use direct memory for 
> achieving higher performance,  if java process use too much direct memory, 
> some exceptions will occur, such as oom-killer. Hence, it’s necessary to 
> monitor it,  and we use JMX to monitor it in our production environment 
> currently.
> 
> src/jdk.jfr/share/classes/jdk/jfr/events/DirectMem

Re: [RFR]: Per thread IO statistics in JFR

[Erik Gahlin]

On 2019-01-17 09:00, Alan Bateman wrote:

On 17/01/2019 07:23, Thomas Stüfe wrote:

:

Do you object against keeping these counters (which basically boils 
down to Thread::current->stat_structure->counter++)? Or do you even 
object against making upcalls into the jvm? Note that, if deemed 
necessary, we could omit updating the counters unless JFR or our 
extended thread dumps are activated (which are the consumers of the 
counters).


In any case, I would have assumed the costs for upcall + counter 
update to be insignificant compared to the IO calls. We should of 
course measure that.


If you generally object upcalls into the libjvm for 
statistical/monitoring reasons, this would make matters on a number 
of fronts more complicated. For instance, it was discussed extending 
NMT coverage to the JDK - which is already in part reality at 
Unsafe.AllocateMemory - and this would have to be done with upcalls too.


There are many issues here that will need write-up and discussion, 
maybe a JEP if discussions converge on a proposal to bring into the 
main line as this is a significant change with implications for many 
areas of the platform. It also potentially conflicts in direction with 
some of the other projects in progress (particularly with Loom trying 
to re-imagine threads, do you really want to collect I/O stats on a 
per thread basis in the future???).


As regards the points to instrument then I think we have to assume 
that much of the native code that is targeted by the current webrev 
will go away or change significantly in the future. We've been on that 
path for some time, e.g. the zip area or the prototype to replace the 
SocketImpl used for classic networking that eliminates a lot of the 
native code touched in that area by the webrev. Once Panama is further 
along then I assume we will want to make use of it in the core 
libraries and at least initially replace the JNI methods that just 
wrap syscalls today, and longer term more significant refactoring. My 
point is that instrumenting native methods may not be the right 
approach, instead maybe we should be look at instrumenting the I/O 
paths at the java level as that will likely play better with the VM. 
There is some support for collecting I/O stats in JFR today and maybe 
someone working in that area can explain that a bit more and what the 
issues are.




Today we have File Read, File Write, Socket Read, and Socket Write 
events. The hook points are added to the JDK using bytecode 
instrumentation. This happens when you start a JFR recording, so there 
is no overhead unless you use it.


Erik

It's impossible to tell from the mail with the webrev what has been 
explored and not explored. It feels like early stages in a much large 
project that will need a write up of prototypes before a direction can 
be proposed.


-Alan

Re: RFR(XXS): 8214161: java.lang.IllegalAccessError: class jdk.internal.event.X509CertificateEvent (in module java.base) cannot access class jdk.jfr.internal.handlers.EventHandler (in module jdk.jfr)

[Erik Gahlin]

Looks good.

Erik

> On 22 Nov 2018, at 16:01, Markus Gronlund  wrote:
> 
> Greetings,
> 
> Please review the following small fix.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8214161 
> Webrev: http://cr.openjdk.java.net/~mgronlun/8214161/webrev01/ 
> 
> diff -r 0a77b7e41322 -r cec1604a9b89 
> src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp
> --- a/src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp  
>  Thu Nov 22 11:15:53 2018 +0100
> +++ b/src/hotspot/share/jfr/instrumentation/jfrEventClassTransformer.cpp  
>  Thu Nov 22 15:18:50 2018 +0100
> @@ -447,6 +447,7 @@
>   if (registered_symbol == NULL) {
> registered_symbol = SymbolTable::lookup_only(registered_constant, sizeof 
> registered_constant - 1, unused_hash);
> if (registered_symbol == NULL) {
> +  untypedEventHandler = true;
>   return false;
> }
>   }
> 
> Description:
> 
> Events in module java.base can't refer to types in module "jdk.jfr" before 
> the jdk.jfr module is loaded and valid read edges established.
> Unfortunately, there exist an uncovered exit path in the code that leaves the 
> eventHandler field "jdk.jfr"-typed, when it should not be.
> 
> Javap output:
> 
> Existing:
> public final class jdk.internal.event.X509CertificateEvent extends 
> jdk.internal.event.Event 
> ... 
>   private static jdk.jfr.internal.handlers.EventHandler eventHandler; 
> descriptor: Ljdk/jfr/internal/handlers/EventHandler; 
> flags: (0x100a) ACC_PRIVATE, ACC_STATIC, ACC_SYNTHETIC 
> 
> Fix: 
> public final class jdk.internal.event.X509CertificateEvent extends 
> jdk.internal.event.Event 
> ... 
>   private static java.lang.Object eventHandler; 
> descriptor: Ljava/lang/Object; 
> flags: (0x100a) ACC_PRIVATE, ACC_STATIC, ACC_SYNTHETIC  
> 
> Tests: open/test/jdk/:jdk_jfr
> 
> Thanks
> Markus

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Erik Gahlin]

Hi Sean,

I think we could still call the event 
"jdk.SecurityPropertyModification", but add a @Description that explains 
that events are only emitted for the JDK due to security concerns. If we 
at a later stage want to include user events, we could add those and 
remove the @Description, possibly with a setting where you can specify 
scope, or perhaps a new event all together.


Perhaps we could find a better name for the field validationEventNumber? 
It sounds like we have an event number, which is not really the case. We 
have a counter for the validation id.


I noticed that you use hashCode as an id. I assume this is to simplify 
the implementation? That is probably OK, the risk of a collision is 
perhaps low, but could you make the field a long, so we in the future 
could add something more unique (Flight Recorder uses compressed 
integers, so a long uses the same amount of disk space as an int). Could 
you also rename the field and the annotation, perhaps certificationId 
could work, so we don't leak how the id was implemented.


I could not find the file: 
test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java


Thanks
Erik

With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk 
code.


Some modifications also made based on off-thread suggestions :

src/java.base/share/classes/java/security/Security.java

* Only record JDK security properties for now (i.e. those in 
java.security conf file)
  - we can consider a new event to record non-JDK security events if 
demand comes about

  - new event renamed to *Jdk*SecurityPropertyModificationEvent

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

* Use hashCode() equality test for storing certs in List.

Tests updated to capture these changes

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java

* Verify that self signed certs exercise the modified code paths

I've added new test functionality to test use of self signed cert.

webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/

Regards,
Sean.
On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off 
thread who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the 
@Relational JFR annotation.
* Keep calls for JFR event operations local to call site to optimize 
jvm compilation


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:

Erik,

After some trial edits, I'm not so sure if moving the event & logger 
commit code into the class where it's used works too well after all.


In the code you suggested, there's an assumption that calls such as 
EventHelper.certificateChain(..) are low cost. While that might be 
the case here, it's something we can't always assume. It's called 
once for the JFR operation and once for the Logger operation. That 
pattern multiplies itself further in other Events. i.e. set up and 
assign the variables for a JFR event without knowing whether they'll 
be needed again for the Logger call. We could work around it by 
setting up some local variables and re-using them in the Logger code 
but then, we're back to where we were. The current EventHelper 
design eliminates this problem and also helps to abstract the 
recording/logging functionality away from the functionality of the 
class itself.


It also becomes a problem where we record events in multiple 
different classes (e.g. SecurityPropertyEvent). While we could leave 
the code in EventHelper for this case, we then have a mixed design 
pattern.


Are you ok with leaving as is for now? A future wish might be one 
where JFR would handle Logger via its own framework/API in a future 
JDK release.


I've removed the variable names using underscore. Also optimized 
some variable assignments in X509Impl.commitEvent(..)


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/

regards,
Sean.

On 09/07/2018 18:01, Seán Coffey wrote:

Erik,

Thanks for reviewing. Comments inline..

On 09/07/18 17:21, Erik Gahlin wrote:

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not 
underscore).

Sure. I see two offending variable names which I'll fix up.


- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we 
would commit the event in a local private function and then use 
the EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the 
recording/logging logic away in a helper class als

Re: Review Request JDK-8181443: Replace usages of jdk.internal.misc.Unsafe with MethodHandles.Lookup.defineClass

[Erik Gahlin]

JFR changes looks good.

Erik

> On 3 Oct 2018, at 19:56, Mandy Chung  wrote:
> 
> This patch replaces java.lang.invoke and JFR use of
> jdk.internal.misc.Unsafe::defineClass with Lookup::defineClass.
> It also replaces dynamic proxy and reflection implementation to
> use the shared secret to invoke ClassLoader::defineClass.
> 
> Webrev:
> http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8181443/webrev.00
> 
> With this change, JDK no longer uses Unsafe::defineClass.
> Any opinion in removing Unsafe::defineClass support?
> 
> I ran jdk_core, jdk_jfr, tier1_runtime and tier1_compiler tests.
> 
> thanks
> Mandy
> 
> 
> 

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Erik Gahlin]

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not underscore).

- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we would 
commit the event in a local private function and then use the 
EventHelper class for common functionality. Instead of:


  private static void recordEvent(SSLSessionImpl session) {
TLSHandshakeEvent hs_event = new TLSHandshakeEvent();
if (hs_event.isEnabled() || EventHelper.loggingSecurity()) {
String certIDs = "";
try {
certIDs = Stream.of(session.getPeerCertificates())
.filter(c -> c instanceof X509Certificate)
.map(c -> (X509Certificate) c)
.map(c -> c.getSerialNumber().toString(16))
.collect(Collectors.joining(", "));
} catch (SSLPeerUnverifiedException e) {
certIDs = e.getMessage(); // not verified msg
}

EventHelper.commitTLSHandshakeEvent(hs_event, null,
session.getPeerHost(), session.getPeerPort(),
session.getCipherSuite(), session.getProtocol(), 
certIDs);

}
}
...
public static void commitTLSHandshakeEvent(TLSHandshakeEvent event,
   Instant start,
   String remoteHost,
   int remotePort,
   String cipherSuite,
   String protocolVersion,
   String certChain) {
if (event != null && event.shouldCommit()) {
event.remoteHost = remoteHost;
event.remotePort = remotePort;
event.cipherSuite = cipherSuite;
event.protocolVersion = protocolVersion;
event.certificateChain = certChain;
event.commit();
}
if (loggingSecurity()) {
String prepend = getDurationString(start);
SECURITY_LOGGER.log(LOG_LEVEL, prepend +
" TLSHandshake: {0}:{1,number,#}, {2}, {3}, {4}",
remoteHost, remotePort, protocolVersion, cipherSuite, 
certChain);

}
}

We would do:

 private static void logHandshake(SSLSessionImpl s) {
 TLSHandshakeEvent event = new TLSHandshakeEvent();
 if (event.shouldCommit()) {
 event.remoteHost = s.getPeerHost();
 event.remotePort = s.getPeerPort();
 event.cipherSuite = s.getCipherSuite();
 event.protocolVersion = s.getProtocol();
 event.certificateChain = 
EventHelper.certificateChain(s.getPeerCerticates());

 event.commit();
 }
 if (EventHelper.isLoggingSecurity()) {
 EventHelper.SECURITY_LOGGER.log(LOG_LEVEL, " TLSHandshake: 
{0}:{1,number,#}, {2}, {3}, {4}",
 s.getPeerHost(), s.getPeerPort(), s.getProtocol(), 
s.getCipherSuite(),

 EventHelper.certificateChain(s.getPeerCerticates()));
 }
 }
 ...
 public static String certificateChain(Certificate[] certificates) {
 try {
 return Stream.of(certificates)
 .filter(c -> c instanceof X509Certificate)
 .map(c -> (X509Certificate) c)
 .map(c -> c.getSerialNumber().toString(16))
 .collect(Collectors.joining(", "));
 } catch (SSLPeerUnverifiedException e) {
 return e.getMessage(); // not verified msg
 }
}

It will also meanless overhead, since only one check is needed for the 
log and the JIT will be able to remove the allocation.


Maybe a similar pattern could be used for the other events as well?

Thanks
Erik

As per request from Erik, I separated the tests out into individual 
ones to test the JFR and Logger functionality. I introduced a new 
separate test for the CertificateChainEvent event also. Originally 
this was wrapped into the TLSHandshakeEvent test.


Thanks to Erik for extra refactoring and modifications carried out to 
clean up the code up further.


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/

This enhancement has a dependency on  JDK-8203629

Regards,
Sean.

On 02/07/18 09:49, Erik Gahlin wrote:



On 29 Jun 2018, at 17:34, Seán Coffey  wrote:

I've introduced a new test helper class in the jdk/test/lib/jfr 
directory to help with the dual test nature of the new tests. It's 
helped alot with test code duplication.


My thinking was to put things like the certificates in a separate 
file, i.e TestCertificates, and then have a logging test and a JFR 
test reuse it.


One rationale for adding logging was to use it if JFR is not present. 
By putting the tests together, it becomes impossible to compile and 
test logging without having JFR.


Looked at use of @DataAmount(DataAmou

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Erik Gahlin]

> On 29 Jun 2018, at 17:34, Seán Coffey  wrote:
> 
> I've introduced a new test helper class in the jdk/test/lib/jfr directory to 
> help with the dual test nature of the new tests. It's helped alot with test 
> code duplication.
> 

My thinking was to put things like the certificates in a separate file, i.e 
TestCertificates, and then have a logging test and a JFR test reuse it.

One rationale for adding logging was to use it if JFR is not present. By 
putting the tests together, it becomes impossible to compile and test logging 
without having JFR.

> Looked at use of @DataAmount(DataAmount.BITS) also. Not sure if it's fits. 
> The output is displayed in units like "KiB" - not the normal when examining 
> key lengths used in X509Certificates. i.e a 2048 bit key gets displayed as "2 
> KiB" - I'd prefer to keep the 2048 display version.

We should not let the JMC GUI decide how units are specified. There will be 
other GUIs and this is the first event that uses bits, so I don’t think it is 
formatted that way because it was considered superior.

Erik

> 
> new webrev at: http://cr.openjdk.java.net/~coffeys/webrev.8148188.v4/webrev/
> 
> Regards,
> Sean.
> 
> On 28/06/18 17:59, Seán Coffey wrote:
>> Comments inline.
>> 
>> 
>> On 28/06/2018 17:20, Erik Gahlin wrote:
>>> It's sufficient if an event object escapes to another method (regardless if 
>>> JFR is enabled or not).
>>> 
>>> Some more feedback:
>>> 
>>> Rename event jdk.CertChain to jdk.CertificateChain
>>> Rename event jdk.X509Cert to jdk.X509Certificate
>>> Rename field certChain to certificateChain.
>>> Rename field serialNum to serialNumber
>> all above done.
>>> Rename field algId to AlgorithmicId or AlgorithmicID
>> maybe "algorithm" works here also ?
>>> Rename @Label("Ciphersuite") to @Label("Cipher Suite")
>>> Rename @Label("Cert Chain") to @Label("Certificate Chain")
>>> Rename @Label("Property Name") to "Name" or "Key" if that makes sense in 
>>> the context?
>>> Rename @Label("Property Value") to "Value".
>>> Put events in a subcategory, i.e  @Category({"Java Development Kit", 
>>> "Security"})
>> done.
>>> Make classes final.
>> done. I had thought that the JFR mechanism required non-final classes.
>>> What is the unit of the key in X509Certificate event? Bits? If that is the 
>>> case, use @DataAmount(DataAmount.BITS)
>> Yes - it's essentially the bit length of the keys used. Let me look into 
>> that annotation usage.
>>> @Label("Serial numbers forming chain of trust") should not be a sentence. 
>>> How about "Serial Numbers"?
>>> 
>>> I think the tests are hard to read when two things are tested at the same 
>>> time. It is also likely that if a test fail due to logging issues, it will 
>>> be assigned to JFR because of the test name, even thought the issues is not 
>>> JFR related.
>> I think we're always going to have some ownership issues when tests serve a 
>> dual purpose. I still think it makes sense to keep the test logic in one 
>> place. Any failures in these tests will most likely be owned by security 
>> team. (moving the tests to security directory is also an option)
>>> 
>>> If you want to reuse code between tests, I would put it in testlibrary.
>> Let me check if there's any common patterns that could be added to the 
>> testlibary.
>> 
>> Thanks,
>> Sean.
>>> 
>>> Erik
>>> 
>>>> Thanks for the update Erik. By default I'm proposing that the new JFR 
>>>> Events and Logger be disabled. As a result the event class shouldn't 
>>>> escape. If performance metrics highlight an issue, we should revisit.
>>>> 
>>>> regards,
>>>> Sean.
>>>> 
>>>> 
>>>> On 27/06/2018 20:57, Erik Gahlin wrote:
>>>>> On 2018-06-27 21:14, Seán Coffey wrote:
>>>>>> 
>>>>>> 
>>>>>> On 27/06/2018 19:57, Xuelei Fan wrote:
>>>>>>> Hi Sean,
>>>>>>> 
>>>>>>> I may reply in several replies.
>>>>>>> 
>>>>>>> PKIXMasterCertPathValidator.java
>>>>>>> 
>>>>>>> +  CertChainEvent cce = new CertChainEvent();
>>>>>>> +  if(cce.isEnabled() || EventHelper.loggingSecurity(

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Erik Gahlin]

It's sufficient if an event object escapes to another method (regardless 
if JFR is enabled or not).


Some more feedback:

Rename event jdk.CertChain to jdk.CertificateChain
Rename event jdk.X509Cert to jdk.X509Certificate
Rename field certChain to certificateChain.
Rename field serialNum to serialNumber
Rename field algId to AlgorithmicId or AlgorithmicID
Rename @Label("Ciphersuite") to @Label("Cipher Suite")
Rename @Label("Cert Chain") to @Label("Certificate Chain")
Rename @Label("Property Name") to "Name" or "Key" if that makes sense in 
the context?

Rename @Label("Property Value") to "Value".
Put events in a subcategory, i.e  @Category({"Java Development Kit", 
"Security"})

Make classes final.
What is the unit of the key in X509Certificate event? Bits? If that is 
the case, use @DataAmount(DataAmount.BITS)
@Label("Serial numbers forming chain of trust") should not be a 
sentence. How about "Serial Numbers"?


I think the tests are hard to read when two things are tested at the 
same time. It is also likely that if a test fail due to logging issues, 
it will be assigned to JFR because of the test name, even thought the 
issues is not JFR related.


If you want to reuse code between tests, I would put it in testlibrary.

Erik

Thanks for the update Erik. By default I'm proposing that the new JFR 
Events and Logger be disabled. As a result the event class shouldn't 
escape. If performance metrics highlight an issue, we should revisit.


regards,
Sean.


On 27/06/2018 20:57, Erik Gahlin wrote:

On 2018-06-27 21:14, Seán Coffey wrote:



On 27/06/2018 19:57, Xuelei Fan wrote:

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

+  CertChainEvent cce = new CertChainEvent();
+  if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+  String c = reversedCertList.stream()
+  .map(x -> x.getSerialNumber().toString(16))
+.collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+   }

No matter the event or the JFR mechanism is enabled or not, the 
above code will create a new instance.  Is the return value of 
cce.isEnabled() dynamically changed or static?
This is a requirement from the JFR framework. All their 
event.isEnabled calls are instance methods and follow a similar 
pattern. The JFR team assure me that the JIT can optimize away such 
calls.


The JIT will most likely not be able to optimize if the event class 
escapes.


From a JFR perspective, this would be the preferred layout:

EventA eventA= new EventA();
eventA.value = this.value;
eventA.commit();

and then do logging separately:

System.Logger.log(DEBUG, this.value)

With this layout, the JIT will remove the allocation and dead store.

If it is expensive to gather the data for the event, like the 
CertChainEvent above, the following pattern should be used.


EventB eventB= new EventB ();
if (eventB.shouldCommit()) {
   eventB.value = calculateValue();
   eventB .commit();
}

If JFR is not enabled, shouldCommit returns false and the JIT should 
be able to remove the allocation.  This will be best from a 
performance point of view, and also in my opinion from a maintenance 
and readability perspective. Others may disagree.


Erik



Is there a need to support both logging and JFR?  I'm new to record 
events.  I did not get the point to use both.
I was initially hoping to concentrate on just JFR events but I got 
strong feedback to also consider use of Logger (esp. in cases where 
the jdk.jfr module is not available)


regards,
Sean.



Thanks,
Xuelei

On 6/26/2018 3:18 PM, Seán Coffey wrote:

Erik,

I rebased the patch with TLS v1.3 integration today. I hadn't 
realized how much the handshaker code had changed. Hopefully, I'll 
get a review from security dev team on that front.


Regards the JFR semantics, I believe the edits should match 
majority of requests . I still have a preference for the test 
infra design for these new logger/JFR tests used in version 1 of 
webrev. I think it makes sense to keep the test functionality 
together - no sense in separating them out into different 
components IMO. Also, some of the edits to the JFR testing were 
made to test for the new dual log/record functionality. I might 
catch up with you tomorrow to see what the best arrangement would be.


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/

regards,
Sean.


On 25/06/2018 21:22, Seán Coffey wrote:

Many thanks for the review comments Erik. Replies inline.


On 24/06/2018 14:21, Erik Gahlin wrote:

Hi Sean,

Some of the changes in the webrev belongs to JDK-8203629 and 
should be removed for clarity.


Some initial comments:

default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The 
purpose of the control attribute is so to provide pa

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Erik Gahlin]

On 2018-06-27 21:14, Seán Coffey wrote:



On 27/06/2018 19:57, Xuelei Fan wrote:

Hi Sean,

I may reply in several replies.

PKIXMasterCertPathValidator.java

+  CertChainEvent cce = new CertChainEvent();
+  if(cce.isEnabled() || EventHelper.loggingSecurity()) {
+  String c = reversedCertList.stream()
+  .map(x -> x.getSerialNumber().toString(16))
+.collect(Collectors.joining(", "));
+ EventHelper.commitCertChainEvent(cce, c);
+   }

No matter the event or the JFR mechanism is enabled or not, the above 
code will create a new instance.  Is the return value of 
cce.isEnabled() dynamically changed or static?
This is a requirement from the JFR framework. All their 
event.isEnabled calls are instance methods and follow a similar 
pattern. The JFR team assure me that the JIT can optimize away such 
calls.


The JIT will most likely not be able to optimize if the event class 
escapes.


From a JFR perspective, this would be the preferred layout:

EventA eventA= new EventA();
eventA.value = this.value;
eventA.commit();

and then do logging separately:

System.Logger.log(DEBUG, this.value)

With this layout, the JIT will remove the allocation and dead store.

If it is expensive to gather the data for the event, like the 
CertChainEvent above, the following pattern should be used.


EventB eventB= new EventB ();
if (eventB.shouldCommit()) {
   eventB.value = calculateValue();
   eventB .commit();
}

If JFR is not enabled, shouldCommit returns false and the JIT should be 
able to remove the allocation.  This will be best from a performance 
point of view, and also in my opinion from a maintenance and readability 
perspective. Others may disagree.


Erik



Is there a need to support both logging and JFR?  I'm new to record 
events.  I did not get the point to use both.
I was initially hoping to concentrate on just JFR events but I got 
strong feedback to also consider use of Logger (esp. in cases where 
the jdk.jfr module is not available)


regards,
Sean.



Thanks,
Xuelei

On 6/26/2018 3:18 PM, Seán Coffey wrote:

Erik,

I rebased the patch with TLS v1.3 integration today. I hadn't 
realized how much the handshaker code had changed. Hopefully, I'll 
get a review from security dev team on that front.


Regards the JFR semantics, I believe the edits should match majority 
of requests . I still have a preference for the test infra design 
for these new logger/JFR tests used in version 1 of webrev. I think 
it makes sense to keep the test functionality together - no sense in 
separating them out into different components IMO. Also, some of the 
edits to the JFR testing were made to test for the new dual 
log/record functionality. I might catch up with you tomorrow to see 
what the best arrangement would be.


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v2/webrev/

regards,
Sean.


On 25/06/2018 21:22, Seán Coffey wrote:

Many thanks for the review comments Erik. Replies inline.


On 24/06/2018 14:21, Erik Gahlin wrote:

Hi Sean,

Some of the changes in the webrev belongs to JDK-8203629 and 
should be removed for clarity.


Some initial comments:

default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The 
purpose of the control attribute is so to provide parameterized 
configuration of events for JMC.  As it is now, the security 
events will be enabled when a user turns on the exception events.

Makes sense. I'll remove that parameter.


X509CertEvent:
You should use milliseconds since epoch to represent a date 
instead of a string value, i.e.


@Label("Valid From")
@Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
public long validFrom;

@Label("Valid Until")
@Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
public long validUntil;

The CertificateValidity class operates on Date Object values. I'll 
work with the Date.getTime() method then (and update the Logger 
formatting)

This following annotation adds little value

@Description("Details of Security Property")

I would either remove the annotation, or provide information that 
helps a user understand the event. For instance, what is X509, and 
what kind of certificates are we talking about?
Yes - that looks like the wrong Description. I'll review all of 
these fields.


@Category("Java Application")

I'm a bit worried that we will pollute the "Java Application" 
namespace if we add lots of JDK events in that category. We may 
want to add a new top level category "Java Development Kit", 
analogous to the "Java Virtual Machine" category, and put all 
security related events in subcategory, perhaps called "Security".

Yes - Open to suggestions. "Security" sounds like a good suggestion.


@Label("X509Cert")

The label should be human readable name, with spaces, title cased 
etc. I would recommend "X.509 C

Re: RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr

[Erik Gahlin]

On 2018-06-24 19:00, Alan Bateman wrote:

On 24/06/2018 12:42, Erik Gahlin wrote:

I have updated jdk.internal.event.Event class with comments.

http://cr.openjdk.java.net/~egahlin/8203629.2/

It is basically a copy the comments in jdk.jfr.Event, but without 
reference to classes in the JFR module.


The rationale for using a class instead of an interface has to do 
with the implementation of JFR. Markus brought up some reasons in his 
e-mail.


I thought about mentioning JFR in the javadoc, and that the purpose 
of the class is to allow use of JFR without a compile time dependency 
on the JFR module, but decided not to, since I think the class could 
have a value on its own for JVMs that do not support JFR, but that 
want to do to something similar for the JDK. They could, for 
instance, add an implementation in the empty method bodies.
I read the mail from Markus and looked at the updated webrev. It's 
good to have some javadoc, just a bit strange to have an abstract 
class without any abstract methods. 


The design stems from jdk.jfr.Event.

If methods in jdk.jfr.Event would be abstract, it would not be possible 
to declare them final and we want them to be final, as users are not 
supposed to override them. When the jdk.jfr.Event class is loaded, the 
final modifiers are removed, so we can generate bytecode for subclasses.


This is a trick to prevent incorrect use of the API.

Why not make the class non-abstract and rely on the protected constructor?

We could do that, it would prevent direct instantiation, but the use of 
the abstract modifier also fits nicely with how the modifier is used for 
event hierarchies. Imagine the following hierarchy and reuse of the sql 
field:


class SQLEvent extends jdk.jfr.Event {
  @Label("SQL");
  String sql;
}
class SQLUpdateEvent extends SQLEvent {
}
class SQLInsertEvent extends SQLEvent {
}
class SelectEvent extends SQLEvent {
}

When users connect with JMC, or call FlightRecorder#getEventTypes(), 
they will get a list of all event types that have been registered in the 
JVM. In the above example, they would get the SQLEvent, which is not 
really meant to be an event that users can configure. If they declare 
SQLEvent abstract, it will be ignored by the JFR system.


This reuse mechanism can be seen in jdk.jfr.events.AbstractJDKEvent

Why not use @Registered(false) instead of the abstract modifier?

We want that annotation to be inherited, so it is possible to use it for 
large set events in a hierarchy. The abstract modifier only operates on 
a particular class.


If the jdk.jfr.Event class is also declared abstract, it can reuse the 
same mechanism. It is convenient from an implementation point of view, 
no special case for jdk.jfr.Event, and it may also hint to a user that 
this the class is not something they should instantiate. It also 
prevents instantiation using reflection and setAccessible(true).


An interface could also prevent instantiation, but it may open for 
malicious implementations and it seems wrong to make into an interface 
when users are not really meant to implement it. It also doesn't work 
well with the JVM implementation.


There isn't enough in the discussion here to understand the original 
rational for why jdk.jfr.Event is abstract, esp. as the constructor is 
protected so it can't be directed instantiated outside of the jdk.jfr 
package. Also an abstract class can extend a non-abstract class so 
abstract jdk.jfr.Event could extend a non-abstract 
jdk.internal.event.Event if you want to clean this up a bit. I don't 
want to get in the way of the current effort but at some point I think 
another set of eyes on the APIs here might help.



Thanks Alan.

We can revisit the design later.

Erik

BTW: My previous comment about the modifiers was mostly about 
jdk.jfr.Event.


-Alan

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Erik Gahlin]

Hi Sean,

Some of the changes in the webrev belongs to JDK-8203629 and should be 
removed for clarity.


Some initial comments:

default.jfc, profile.jfr:
The events should not have control="enable-exceptions". The purpose of 
the control attribute is so to provide parameterized configuration of 
events for JMC.  As it is now, the security events will be enabled when 
a user turns on the exception events.


X509CertEvent:
You should use milliseconds since epoch to represent a date instead of a 
string value, i.e.


@Label("Valid From")
@Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
public long validFrom;

@Label("Valid Until")
@Timestamp(Timestamp.MILLISECONDS_SINCE_EPOCH)
public long validUntil;

This following annotation adds little value

@Description("Details of Security Property")

I would either remove the annotation, or provide information that helps 
a user understand the event. For instance, what is X509, and what kind 
of certificates are we talking about?


@Category("Java Application")

I'm a bit worried that we will pollute the "Java Application" namespace 
if we add lots of JDK events in that category. We may want to add a new 
top level category "Java Development Kit", analogous to the "Java 
Virtual Machine" category, and put all security related events in 
subcategory, perhaps called "Security".


@Label("X509Cert")

The label should be human readable name, with spaces, title cased etc. I 
would recommend "X.509 Certificate". In general, avoid abbreviations 
like "certs" and instead use labels such as "Certificate Chain". The 
label should be short and not a full sentence.


For details see,
https://docs.oracle.com/javase/10/docs/api/jdk/jfr/Label.html

I think it would be good to separate testing of JFR and logging into 
different files / tests. I would prefer that the test name is the same 
as the event prefixed with "Test", i.e TestX509CertificateEvent, as this 
is the pattern used by other JFR tests.


I reworked one of the tests to how I like to see it:

/*
 * @test
 * @key jfr
 * @library /test/lib
 * @run main/othervm jdk.jfr.event.security.TestX509CertificateEvent
 */
public class TestX509CertificateEvent {

private static final String CERTIFICATE_1 = "...";
private static final String CERTIFICATE_2 = "...";

public static void main(String... args) throws CertificateException {

 Recording r = new Recording();
 r.enable(EventNames.X509Certificate).withoutStackTrace();
 r.start();

 CertificateFactory cf = CertificateFactory.getInstance("X.509");
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_2.getBytes()));


 // Make sure only one event per certificate
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_1.getBytes()));
 cf.generateCertificate(new 
ByteArrayInputStream(CERTIFICATE_2.getBytes()));


 r.stop();

 List events = Events.fromRecording(r);
 Asserts.assertEquals(events.size(), 2, "Expected two X.509 
Certificate events");


 assertEvent(events, "1000", "SHA256withRSA",
"CN=SSLCertificate, O=SomeCompany",
"CN=Intermediate CA Cert, O=SomeCompany",
 "RSA", 2048);
 assertEvent(events, "1000", "SHA256withRSA",
"CN=SSLCertificate, O=SomeCompany",
"CN=Intermediate CA Cert, O=SomeCompany",
 "RSA", 2048);
}

private static void assertEvent(List events, String 
certID, String algId, String subject,

String issuer, String keyType, int length) throws Exception {

for (RecordedEvent e : events) {
if (e.getString("serialNumber").equals(certID)) {
Events.assertField(e, "algId").equal(algId);
...
return;
}
}
System.out.println(events);
throw new Exception("Could not find event with Cert ID: " + 
certID);

}
}

The reworked example uses the Events.assertField method, which will give 
context if the assertion fails. Keeping the test simple, means it can be 
analyzed quickly if it fails in testing. There is no new test framework 
to learn, or methods to search for, and it is usually not hard to 
determine if the failure is product, test or infrastructure related, and 
what component (team) should be assigned the issue. The use of 
EventNames.X509Certificate means all occurrences of the event can be 
tracked done in an IDE using find by reference.


Thanks
Erik

Following on from the recent JDK-8203629 code review, I'd like to 
propose enhancements on how we can record events in security libs. The 
introduction of the JFR libraries can give us much better ways of 
examining JDK actions. For the initial phase, I'm looking to enhance 
some key security library events in JDK 11 so that they can be either 

Re: RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr

[Erik Gahlin]

On 2018-06-21 09:52, Alan Bateman wrote:

On 20/06/2018 18:59, Erik Gahlin wrote:

:

Also all the methods are empty which makes me wonder if they should 
be abstract (as the class is abstract) or whether it should be an 
interface.

Abstract is needed to prevent user from instantiating the class.

The methods need to have a body, otherwise event classes in java.base 
would need to implement the methods, which would be cumbersome. We 
like to use a class as it simplifies the implementation if we know 
all event classes have a common ancestor with java.lang.Object as a 
parent.


so we can be guaranteed that the class
I'm not sure that I understand the issues here but just to say that 
jdk.internal.event is not exported so code outside of the JDK cannot 
directly instantiate instances of classes in that package. Also 
interfaces can have default methods which may go to your concern about 
needing to implement each of the 6 methods. Once Event is cleaned up 
with some javadoc then it will be easier to argue this one way or the 
other.


-Alan

I have updated jdk.internal.event.Event class with comments.

http://cr.openjdk.java.net/~egahlin/8203629.2/

It is basically a copy the comments in jdk.jfr.Event, but without 
reference to classes in the JFR module.


The rationale for using a class instead of an interface has to do with 
the implementation of JFR. Markus brought up some reasons in his e-mail.


I thought about mentioning JFR in the javadoc, and that the purpose of 
the class is to allow use of JFR without a compile time dependency on 
the JFR module, but decided not to, since I think the class could have a 
value on its own for JVMs that do not support JFR, but that want to do 
to something similar for the JDK. They could, for instance, add an 
implementation in the empty method bodies.


I also removed an import statement in JDKEvents.java that referenced a 
test class that should not have been there.


Erik

Re: RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr

[Erik Gahlin]

Sounds good Sean.

We will push this RFE when your enhancement is ready and reviewed. We 
like to avoid pushing this to JDK 11 unless it is being used in "the 
current train".


Erik

Thanks for implementing this enhancement Erik. I think it'll prove to 
be popular given the functionality available in JFR recordings. I'd 
agree with Alan's comment that the Event class could be documented 
better to indicate its purpose.


I'm already using this enhancement to implement some new events in the 
JDK security libraries. I hope to get a review started real soon on 
that front.


Regards,
Sean.

On 19/06/18 03:06, Erik Gahlin wrote:

Hi,

Could I have a review of an enhancement that will make it possible to 
add JFR events to java.base, and possibly other modules in the JDK, 
without a compile time dependency on jdk.jfr.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8203629

Webrev:
http://cr.openjdk.java.net/~egahlin/8203629.0

Testing:
Tests in test/jdk/jdk/jfr

The functionality is a prerequisite for an upcoming enhancement that 
will add JFR events to the security libraries.


In short, jdk.jfr.Event (located in jdk.jfr) extends 
jdk.internal.event.Event (located in java.base). Metadata for events, 
such as labels and descriptions, are added using a mirror event class 
located in jdk.jfr module. If the fields of the mirror class and the 
event class don't match an InternalError is thrown.


To illustrate how the mechanism can be used, see the following 
example (not meant to be checked in).


http://cr.openjdk.java.net/~egahlin/8203629.example

Since the implementation of jdk.internal.event.Event is empty, the 
JIT will typically eliminate all traces of JFR functionality unless 
Flight Recorder is enabled.


Thanks
Erik

Re: RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr

[Erik Gahlin]

On 2018-06-20 19:09, Alan Bateman wrote:



On 19/06/2018 03:06, Erik Gahlin wrote:

Hi,

Could I have a review of an enhancement that will make it possible to 
add JFR events to java.base, and possibly other modules in the JDK, 
without a compile time dependency on jdk.jfr.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8203629

Webrev:
http://cr.openjdk.java.net/~egahlin/8203629.0

Testing:
Tests in test/jdk/jdk/jfr

The functionality is a prerequisite for an upcoming enhancement that 
will add JFR events to the security libraries.


In short, jdk.jfr.Event (located in jdk.jfr) extends 
jdk.internal.event.Event (located in java.base). Metadata for events, 
such as labels and descriptions, are added using a mirror event class 
located in jdk.jfr module. If the fields of the mirror class and the 
event class don't match an InternalError is thrown.


To illustrate how the mechanism can be used, see the following 
example (not meant to be checked in).


http://cr.openjdk.java.net/~egahlin/8203629.example

Since the implementation of jdk.internal.event.Event is empty, the 
JIT will typically eliminate all traces of JFR functionality unless 
Flight Recorder is enabled.


Adding a way to get events from java.base is good but I wonder if 
jdk.internal.event.Event could be cleaned up before you push this. It 
would be nice to have a class description and some minimal method 
descriptions too. 

I will add some comments.

Also all the methods are empty which makes me wonder if they should be 
abstract (as the class is abstract) or whether it should be an interface.

Abstract is needed to prevent user from instantiating the class.

The methods need to have a body, otherwise event classes in java.base 
would need to implement the methods, which would be cumbersome. We like 
to use a class as it simplifies the implementation if we know all event 
classes have a common ancestor with java.lang.Object as a parent.


so we can be guaranteed that the class
Some of the method modifiers are in unusual order and it would be good 
to get those cleaned up too.


I will make the class "public abstract". The other modifiers looked OK 
to me, but please let me know if there are other modifiers you want me 
to change.


Erik

RFR: 8203629: Produce events in the JDK without a dependency on jdk.jfr

[Erik Gahlin]

Hi,

Could I have a review of an enhancement that will make it possible to 
add JFR events to java.base, and possibly other modules in the JDK, 
without a compile time dependency on jdk.jfr.


Bug:
https://bugs.openjdk.java.net/browse/JDK-8203629

Webrev:
http://cr.openjdk.java.net/~egahlin/8203629.0

Testing:
Tests in test/jdk/jdk/jfr

The functionality is a prerequisite for an upcoming enhancement that 
will add JFR events to the security libraries.


In short, jdk.jfr.Event (located in jdk.jfr) extends 
jdk.internal.event.Event (located in java.base). Metadata for events, 
such as labels and descriptions, are added using a mirror event class 
located in jdk.jfr module. If the fields of the mirror class and the 
event class don't match an InternalError is thrown.


To illustrate how the mechanism can be used, see the following example 
(not meant to be checked in).


http://cr.openjdk.java.net/~egahlin/8203629.example

Since the implementation of jdk.internal.event.Event is empty, the JIT 
will typically eliminate all traces of JFR functionality unless Flight 
Recorder is enabled.


Thanks
Erik

Re: RFR: 8041626: [Event Request] Shutdown reason

[Erik Gahlin]

Hi Robin,

I can sponsor this.

I wonder if we should change the name of the event to "Shutdown" 
instead? It will give us flexibility to add other shutdown related 
fields in the future.


Could you change the label and field to "Status Code" and statusCode.  
Event fields should have headline-style capitalization and statusCode 
allows us to add other status related information in the future.


Thanks
Erik


Hi all,

Please review the following change that adds an event-based tracing 
event that is generated when the VM shuts down. The intent is to 
capture shutdowns that occur after the VM has been properly 
initialized (as initialization problems would most likely mean that 
the tracing framework hasn’t been properly started either).


Issue: https://bugs.openjdk.java.net/browse/JDK-8041626
Webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00/ 


Testing: hs-tier1,hs-tier2,jdk-tier1,jdk-tier2

Best regards,
Robin

Re: RFR(S): 8043520: Serviceability tests using @library failing with java.lang.NoClassDefFoundError

[Erik Gahlin]

Looks good!

Erik

Yekaterina Kantserova skrev 2014-05-20 15:48:

Thanks Staffan!

New webrev can be found here: 
http://cr.openjdk.java.net/~ykantser/8043520/webrev.01/


// Katja



On 05/20/2014 03:07 PM, Staffan Larsen wrote:

test/sun/management/jmxremote/bootstrap/PasswordFilePermissionTest.java
   “Dummy” is being built twice.

Otherwise good!

/Staffan


On 20 maj 2014, at 14:24, Yekaterina Kantserova 
yekaterina.kantser...@oracle.com wrote:



Staffan, Alan,

could you please review the following fix.

Bug: https://bugs.openjdk.java.net/browse/JDK-8043520
Webrev: http://cr.openjdk.java.net/~ykantser/8043520/webrev.00/

I've missed somehow several tests in 
http://cr.openjdk.java.net/~ykantser/8034960/webrev.01/ which 
existed in http://cr.openjdk.java.net/~ykantser/8034960/webrev.00/.


Thanks,
Katja

hg: jdk8/tl/jdk: 6402201: ProcessAttachTest.sh needs better synchronization

[erik . gahlin]

Changeset: 91ec3bc92793
Author:egahlin
Date:  2013-11-21 13:46 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/91ec3bc92793

6402201: ProcessAttachTest.sh needs better synchronization
Reviewed-by: alanb

! test/ProblemList.txt
! test/com/sun/jdi/ProcessAttachDebuggee.java
! test/com/sun/jdi/ProcessAttachTest.sh

hg: jdk8/tl/jdk: 7141544: TEST_BUG: com/sun/jdi/BreakpointWithFullGC.sh fails

[erik . gahlin]

Changeset: 894a4bae9e33
Author:egahlin
Date:  2013-11-20 12:32 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/894a4bae9e33

7141544: TEST_BUG: com/sun/jdi/BreakpointWithFullGC.sh fails
Reviewed-by: sla

! test/com/sun/jdi/BreakpointWithFullGC.sh

hg: jdk8/tl/jdk: 8028505: Put sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh on ProblemList.txt

[erik . gahlin]

Changeset: d6195774dd1f
Author:egahlin
Date:  2013-11-19 11:47 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d6195774dd1f

8028505: Put sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh on 
ProblemList.txt
Reviewed-by: alanb

! test/ProblemList.txt

hg: jdk8/tl/jdk: 6959636: testcase failing on windows javax/management/loading/LibraryLoader/LibraryLoaderTest.java

[erik . gahlin]

Changeset: ddaa9a8acaed
Author:egahlin
Date:  2013-11-13 15:21 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ddaa9a8acaed

6959636: testcase failing on windows 
javax/management/loading/LibraryLoader/LibraryLoaderTest.java
Reviewed-by: sla, jbachorik

! test/ProblemList.txt
! test/javax/management/loading/LibraryLoader/LibraryLoaderTest.java

hg: jdk8/tl/jdk: 6954510: TEST_BUG: Testcase failure com/sun/jdi/BreakpointWithFullGC.sh

[erik . gahlin]

Changeset: 256b3395346b
Author:egahlin
Date:  2013-11-13 18:41 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/256b3395346b

6954510: TEST_BUG: Testcase failure com/sun/jdi/BreakpointWithFullGC.sh
Reviewed-by: sla, sspitsyn

! test/com/sun/jdi/BreakpointWithFullGC.sh

hg: jdk8/tl/jdk: 8027209: javax/management/monitor/ThreadPoolAccTest.java fails intermittently

[erik . gahlin]

Changeset: 41dcb0c2e194
Author:egahlin
Date:  2013-11-12 14:52 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/41dcb0c2e194

8027209: javax/management/monitor/ThreadPoolAccTest.java fails intermittently
Reviewed-by: sla, jbachorik

! test/javax/management/monitor/ThreadPoolAccTest.java

hg: jdk8/tl/jdk: 6543856: MonitorVmStartTerminate.sh fails intermittently

[erik . gahlin]

Changeset: 4cff9f59644f
Author:egahlin
Date:  2013-11-12 17:40 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4cff9f59644f

6543856: MonitorVmStartTerminate.sh fails intermittently
Reviewed-by: sla, dholmes

! test/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.java
! test/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.sh
! test/sun/jvmstat/testlibrary/JavaProcess.java

hg: jdk8/tl/jdk: 6849945: VM Periodic Task Thread CPU time = -1ns in HotspotThreadMBean.getInternalThreadCpuTimes()

[erik . gahlin]

Changeset: d9f827e4d20c
Author:egahlin
Date:  2013-11-12 18:12 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d9f827e4d20c

6849945: VM Periodic Task Thread CPU time = -1ns in 
HotspotThreadMBean.getInternalThreadCpuTimes()
Reviewed-by: sla

! test/sun/management/HotspotThreadMBean/GetInternalThreads.java

hg: jdk8/tl/jdk: 7105883: JDWP: agent crash if there exists a ThreadGroup with null name

[erik . gahlin]

Changeset: c077a2810782
Author:egahlin
Date:  2013-10-23 10:50 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c077a2810782

7105883: JDWP: agent crash if there exists a ThreadGroup with null name
Reviewed-by: sla, jbachorik

! src/share/back/ThreadGroupReferenceImpl.c
+ test/com/sun/jdi/NullThreadGroupNameTest.java