Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Hamlin Li]

Hi Daniel,

Got you point.

Thank you
-Hamlin

On 2016/6/22 13:24, Daniel Fuchs wrote:

Hi Hamlin,

On 22/06/16 02:40, Hamlin Li wrote:

Just another minor comment, I'm not sure if following line is necessary
in Logger.java, as it's already been checked in LogManager.java line 
577:


439 if (cfg == other) return cfg;


Yes, you're right - but it makes sense for the method to have it :-)

The reason it's already checked in LogManager is to avoid the
creation and invocation of a PrivilegedAction for nothing.
If we did not need to go through that - we would just call
mergeConfigWithSystemPeer and let the importConfig config
method do the check.

best regards,

-- daniel

Re: RFR 8159245: Loggers created by system classes are not initialized correctly when configured programmatically from application code.

[Hamlin Li]

Hi Daniel,

Thank you for detailed explanation, new version makes sense to me.
Just another minor comment, I'm not sure if following line is necessary 
in Logger.java, as it's already been checked in LogManager.java line 577:


439 if (cfg == other) return cfg;

Thank you
-Hamlin

On 2016/6/21 19:54, Daniel Fuchs wrote:

Hi Hamlin,

I was mistaken in my first assessment.

The case where the system handler's list is not empty
should only happen if by misfortune two different threads
happen to attempt to merge the same two configurations
concurrently. Though of no consequence for level, filter,
etc... (single values) that would be an issue for the
handlers list if not handled correctly.

With that in mind I have slightly revised the fix - and
added a more verbose comment explaining the reason for the
isEmpty() check.

http://cr.openjdk.java.net/~dfuchs/webrev_8159245/webrev.01/index.html

best regards,

-- daniel


On 21/06/16 07:57, Daniel Fuchs wrote:

Hi Hamlin,

There's no difference in behaviour - mergeConfigWithSystemPeer is called
unconditionally but the handlers from the application logger will be
copied in the system logger config only if the system's logger
list is empty - just like before.

This is something that will probably need to be revisited - maybe
the system handlers should be closed first and the application
handlers added unconditionally.
The issue here is that if the system logger had handlers
configured from the logging.properties file then the user handlers
are going to be garbage collected before being closed - which is
not ideal - even if it should rarely happen.

Let me think on it.

RFR of JDK-8159785: Add test that tests ClassLoader.getResource/getResources in Multi-Release Jar

[Hamlin Li]

Would you please review the following patch?

bug: https://bugs.openjdk.java.net/browse/JDK-8159785
webrev: http://cr.openjdk.java.net/~mli/8159785/webrev.00/

JDK-8151542 fixed bugs 
and added test to verify Class.getResource works as expected, but there 
is not test to verify that ClassLoader.getResource/getResources works 
well too.


Thank you
-Hamlin

Re: JDK 9 RFR of JDK-8157211: Mark tools/launcher/FXLauncherTest.java as intermittently failing

[Hamlin Li]

Would you please help to review the code change?

Thank you
-Hamlin

On 2016/5/18 12:52, Hamlin Li wrote:

tools/launcher/FXLauncherTest.java
This test is known to fail intermittently (JDK-8130392 
<https://bugs.openjdk.java.net/browse/JDK-8130392>), it should be 
marked accordingly with@key intermittentjtreg tag in the test file.


bug: https://bugs.openjdk.java.net/browse/JDK-8157211
webrev: http://cr.openjdk.java.net/~mli/8157211/webrev.00/

Thank you
-Hamlin

--- a/test/tools/launcher/FXLauncherTest.javaTue May 17 19:20:05 
2016 -0700
+++ b/test/tools/launcher/FXLauncherTest.javaTue May 17 21:44:18 
2016 -0700

@@ -29,6 +29,7 @@
  * jfx app class, a main-class for the manifest, a bogus one and none.
  * All should execute except the incorrect fx app class entries.
  * @run main/othervm FXLauncherTest
+ * @key intermittent
  */
 import java.io.File;
 import java.io.IOException;

JDK 9 RFR of JDK-8151904: test/java/lang/StackWalker/VerifyStackTrace.java needs to handle update releases

[Hamlin Li]

The test currently hardcodes the version number "9". We should build a 
JDK with a different release number e.g. 9.1 and checks if this test 
handles it properly.
Test run successfully on 9-ea+118, dummybundles(9-ea+255, 9.1-ea+255, 
9.0.1-ea+255, 9.0.0.1-ea+255).


bug: https://bugs.openjdk.java.net/browse/JDK-8151904
webrev: http://cr.openjdk.java.net/~mli/8151904/webrev.00/

Thank you
-Hamlin

JDK 9 RFR of JDK-8157211: Mark tools/launcher/FXLauncherTest.java as intermittently failing

[Hamlin Li]

tools/launcher/FXLauncherTest.java
This test is known to fail intermittently (JDK-8130392 
), it should be marked 
accordingly with@key intermittentjtreg tag in the test file.


bug: https://bugs.openjdk.java.net/browse/JDK-8157211
webrev: http://cr.openjdk.java.net/~mli/8157211/webrev.00/

Thank you
-Hamlin

--- a/test/tools/launcher/FXLauncherTest.java Tue May 17 19:20:05 2016 -0700
+++ b/test/tools/launcher/FXLauncherTest.javaTue May 17 21:44:18 
2016 -0700

@@ -29,6 +29,7 @@
  * jfx app class, a main-class for the manifest, a bogus one and none.
  * All should execute except the incorrect fx app class entries.
  * @run main/othervm FXLauncherTest
+ * @key intermittent
  */
 import java.io.File;
 import java.io.IOException;

Re: JDK 9 RFR of JDK-8157006/8157011: Problem list java/io/pathNames/GeneralWin32.java and tools/pack200/TestNormal.java

[Hamlin Li]

Would you please review the change to problem list 2 frequently failing 
tests.


Thank you
-Hamlin

On 2016/5/16 15:39, Hamlin Li wrote:

java/io/pathNames/GeneralWin32.java
has been seen to fail with high frequency on windows. Until 
JDK-8156595 is addressed, the test should be problem listed.

bug: https://bugs.openjdk.java.net/browse/JDK-8157006

tools/pack200/TestNormal.java
has been seen to fail with some frequency on windows. Until 
JDK-8156807 is addressed, the test should be problem listed.

bug: https://bugs.openjdk.java.net/browse/JDK-8157011

webrev: http://cr.openjdk.java.net/~mli/8157006/webrev.00/

Thank you
-Hamlin

JDK 9 RFR of JDK-8157006/8157011: Problem list java/io/pathNames/GeneralWin32.java and tools/pack200/TestNormal.java

[Hamlin Li]

java/io/pathNames/GeneralWin32.java
has been seen to fail with high frequency on windows. Until JDK-8156595 
is addressed, the test should be problem listed.

bug: https://bugs.openjdk.java.net/browse/JDK-8157006

tools/pack200/TestNormal.java
has been seen to fail with some frequency on windows. Until JDK-8156807 
is addressed, the test should be problem listed.

bug: https://bugs.openjdk.java.net/browse/JDK-8157011

webrev: http://cr.openjdk.java.net/~mli/8157006/webrev.00/

Thank you
-Hamlin

JDK 9 RFR of JDK-8156189: Problem list tools/jdeps/modules/GenModuleInfo.java and ModuleTest.java until JDK-8153481 is resolved

[Hamlin Li]

JDK-8153481 is being fixed, so put 
tools/jdeps/modules/GenModuleInfo.java and ModuleTest.java in Problem list.


bug: https://bugs.openjdk.java.net/browse/JDK-8156189
webrev: http://cr.openjdk.java.net/~mli/8156189/webrev.00/

Thank you
-Hamlin

JDK 9 RFR of JDK-8151785: Doc typo in src/../java/util/stream/PipelineHelper.java

[Hamlin Li]

Please help to review the tiny doc typo fix for bug 
https://bugs.openjdk.java.net/browse/JDK-8151785
it should be "copyInto(wrapSink(sink), spliterator);", but it is 
"intoWrapped(wrapSink(sink), spliterator);", there is no method called 
intoWrapped.



diff -r 25e8c082d7ef 
src/java.base/share/classes/java/util/stream/PipelineHelper.java
--- a/src/java.base/share/classes/java/util/stream/PipelineHelper.java 
Sun Mar 13 20:26:29 2016 +0100
+++ b/src/java.base/share/classes/java/util/stream/PipelineHelper.java 
Sun Mar 13 20:46:22 2016 -0700

@@ -98,7 +98,7 @@
  * @implSpec
  * The implementation behaves as if:
  * {@code
- * intoWrapped(wrapSink(sink), spliterator);
+ * copyInto(wrapSink(sink), spliterator);
  * }
  *
  * @param sink the {@code Sink} to receive the results

Thank you
-Hamlin

RFR: 8148928: java/util/stream/test/**/SequentialOpTest.java timed out intermittently

[Hamlin Li]

Hi everyone,

Would you please help to review the fix for bug 
https://bugs.openjdk.java.net/browse/JDK-8148928, 
java/util/stream/test/**/SequentialOpTest.java timed out intermittently.

webrev: http://cr.openjdk.java.net/~mli/8148928/webrev.00/

Thank you
-Hamlin

Re: RFR: 8076458: java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java timeout

[Hamlin Li]

Hi Paul,

Sorry for delayed response, have been occupied by other higher priority 
task.
Thanks for your review, I agree with you that your second approach is 
better.

New webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.01/


Below are times cost for different ops:
  total:169.996
  testOps only:108.988
  testIntOps only :23.865
  testLongOps only :22.326
  testDoubleOps only :16.944
so, I build small data providers for each of them.

Thank you
-Hamlin

On 2016/1/26 21:18, Paul Sandoz wrote:

Hi Hamlin,

Conservatively I would prefer not to remove data sets if at all possible. It 
will affect all tests, and leaf tasks for parallel streams should have enough 
data to crunch on.

I suspect the problem of the flatMap test is not necessarily due to the source 
sizes being of 1000 elements but that there are tests that substitute an 
element whose value is m for n elements from 0..m, which can explode things and 
generate lots of garbage.

Have you tried executing those kinds tests when the data size is < 1000?

My bet is the FlatMapOpTest will run significantly faster and you will not need 
to split it out.

There are two ways we could consider doing this:

1) Check the size in the test method:

if (data.size() < 1000) {
 exerciseOps(data, s -> s.flatMap(mfLt));
 exerciseOps(data, s -> s.flatMap(integerRangeMapper));
 exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
e).boxed().limit(10)));
}

2) Include a new data provider for smaller data sets

@Test(dataProvider = "StreamTestData", dataProviderClass = 
StreamTestDataProvider.class)
public void testOps(String name, TestData.OfRef data) {
 Collection result = exerciseOps(data, s -> s.flatMap(mfId));
 assertEquals(data.size(), result.size());

 result = exerciseOps(data, s -> s.flatMap(mfNull));
 assertEquals(0, result.size());

 result = exerciseOps(data, s-> s.flatMap(e -> Stream.empty()));
 assertEquals(0, result.size());
}

@Test(dataProvider = "StreamTestData.small", dataProviderClass = 
StreamTestDataProvider.class)
public void testOpsX(String name, TestData.OfRef data) {
 exerciseOps(data, s -> s.flatMap(mfLt));
 exerciseOps(data, s -> s.flatMap(integerRangeMapper));
 exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
e).boxed().limit(10)));
}

I prefer the latter approach (applied to ref and primitive data sets). It’s 
more work, but i think the right direction.

Paul.


On 26 Jan 2016, at 08:08, Hamlin Li <huaming...@oracle.com> wrote:

Hi everyone,

Would you please help to review the fix for bug 
https://bugs.openjdk.java.net/browse/JDK-8076458, 
java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java 
timeout.
webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.00/

Thank you
-Hamlin

Re: RFR: 8076458: java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java timeout

[Hamlin Li]

On 2016/1/29 20:53, Paul Sandoz wrote:

On 29 Jan 2016, at 13:43, Hamlin Li <huaming...@oracle.com> wrote:

Hi Paul,

Sorry for delayed response, have been occupied by other higher priority task.
Thanks for your review, I agree with you that your second approach is better.
New webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.01/


The changes to the data providers look ok.

Would you mind splitting out the tests between StreamTestData and 
StreamTestData.small as outlined in 2) below. That way for the non-eplosive 
stuff we can still crunch on larger data without much of a slow down.

Hi Pual,

Yes, you're right, it does not slow down too much, it cost 15.553 
seconds after the first revision(webrev.01), and it cost 16.064 after 
the second revision(webrev.02).

Please check the webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.02/

Below are times cost for different ops:
  total:169.996
  testOps only:108.988
  testIntOps only :23.865
  testLongOps only :22.326
  testDoubleOps only :16.944
so, I build small data providers for each of them.


Ok, and i suspect/hope it drops by at least an order of magnitude with your 
changes applied.

Yes, it cost 15.553 seconds after the first revision(webrev.01).

Out of curiosity i wonder what the times would be if using parallel GC rather 
than G1.

With different GC options after second revision(webrev.02):
  -UseParallelGC:  elapsed time (seconds): 16.047
  +UseParallelGC: elapsed time (seconds): 13.263
  -UseG1GC: elapsed time (seconds): 16.612
  +UseG1GC:elapsed time (seconds): 16.998
  -UseParallelOldGC: elapsed time (seconds): 16.039
  +UseParallelOldGC: elapsed time (seconds): 14.297

Thank you
-Hamlin


Paul.


Thank you
-Hamlin

On 2016/1/26 21:18, Paul Sandoz wrote:

Hi Hamlin,

Conservatively I would prefer not to remove data sets if at all possible. It 
will affect all tests, and leaf tasks for parallel streams should have enough 
data to crunch on.

I suspect the problem of the flatMap test is not necessarily due to the source 
sizes being of 1000 elements but that there are tests that substitute an 
element whose value is m for n elements from 0..m, which can explode things and 
generate lots of garbage.

Have you tried executing those kinds tests when the data size is < 1000?

My bet is the FlatMapOpTest will run significantly faster and you will not need 
to split it out.

There are two ways we could consider doing this:

1) Check the size in the test method:

if (data.size() < 1000) {
 exerciseOps(data, s -> s.flatMap(mfLt));
 exerciseOps(data, s -> s.flatMap(integerRangeMapper));
 exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
e).boxed().limit(10)));
}

2) Include a new data provider for smaller data sets

@Test(dataProvider = "StreamTestData", dataProviderClass = 
StreamTestDataProvider.class)
public void testOps(String name, TestData.OfRef data) {
 Collection result = exerciseOps(data, s -> s.flatMap(mfId));
 assertEquals(data.size(), result.size());

 result = exerciseOps(data, s -> s.flatMap(mfNull));
 assertEquals(0, result.size());

 result = exerciseOps(data, s-> s.flatMap(e -> Stream.empty()));
 assertEquals(0, result.size());
}

@Test(dataProvider = "StreamTestData.small", dataProviderClass = 
StreamTestDataProvider.class)
public void testOpsX(String name, TestData.OfRef data) {
 exerciseOps(data, s -> s.flatMap(mfLt));
 exerciseOps(data, s -> s.flatMap(integerRangeMapper));
 exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
e).boxed().limit(10)));
}

I prefer the latter approach (applied to ref and primitive data sets). It’s 
more work, but i think the right direction.

Paul.

RFR: 8076458: java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java timeout

[Hamlin Li]

Hi everyone,

Would you please help to review the fix for bug 
https://bugs.openjdk.java.net/browse/JDK-8076458, 
java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java 
timeout.

webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.00/

Thank you
-Hamlin

Re: JDK 9 RFR of JDK-8144321: Improve existent tests to check the exact line numbers in Stack Frame.

[Hamlin Li]

Hi Daniel,

Thanks for reviewing and suggestions, 1> add big warning to warn 
maintainer that the tests were dependent on line numbers.; 2> verify 
line numbers by parsing the source code. I think your second suggestion 
is great! :-), but to keep the logic simple and straight, I prefer your 
first suggestion which is to add big warning. When tests detect that 
line numbers are mismatched, there will be detailed error message 
explaining the reason.

webrev : http://cr.openjdk.java.net/~mli/8144321/webrev.01/

Thank you
-Hamlin

On 2015/12/5 18:35, Daniel Fuchs wrote:

HI Hamlin,

If I understand well any line, comment, etc, added to the test
in the future is likely to make the tests fail because the line numbers
will no longer match.

I have seen other tests (I think they where compiler tests) which
exhibited such a characteristic, but those had a big warning in
their summary to warn maintainer that the tests were dependent
on line numbers.

If you can't find a way to make these test independent of
line numbers, then at least there should be a big warning
in the test summary, together with instruction on how to figure out
which values to put in the annotation...

One possibility to avoid having the test depend on hardcode line numbers
could be to embed some logic in the test to make it process (read) its 
own source
file to discover the actual line numbers at run time and generate some 
kind of
data file/or static Map that StackFrameAnnotationUtil could in turn 
read to get

the expected line number.
For instance, instead of embedding the real number, the annotation could
embed an index, and the corresponding line could have a marker comment
at the end referring to the index... e.g.:

93 @StackFrameAnnotation(lineNumbers = {1, 2})
...
  
120 walker.walk(s -> { // should check this line number in 
@StackFrameAnnotation.  // @LINE#1

121 s.filter(f -> f.getClassName().startsWith("AcrossThreads$"))
122 .forEach(f -> StackFrameAnnotationUtils.verifyLineNumber(
123 f.getClassName(),
124 f.getMethodName(),
125 f.getLineNumber().getAsInt()));
126 return null;
127 });
  128 } else {
129 assertWalker(walker, n); // should check this line number in 
@StackFrameAnnotation.  // @LINE#2

  130 }

The test could then first process its source file to figure out that @LINE#1 is 
at line 120 and
@LINE#2 is at line 129, and then build a map { 1 -> 120, 2 -> 129 } which
StackFrameAnnotationUtil would use to get the real line number from the index
it finds in the annotation...
Directly hardcoding line numbers inside annotations in the test looks 
a bit fragile. Maybe someone else on the list will be able to suggest 
even better ideas :-) best regards, -- daniel On 12/5/15 6:00 AM, 
Hamlin Li wrote:
Hi all, Would you please help to review the test development of 
https://bugs.openjdk.java.net/browse/JDK-8144321. webrev : 
http://cr.openjdk.java.net/~mli/8144321/webrev.00/ Thank you -Hamlin 

JDK 9 RFR of JDK-8144321: Improve existent tests to check the exact line numbers in Stack Frame.

[Hamlin Li]

Hi all,

Would you please help to review the test development of 
https://bugs.openjdk.java.net/browse/JDK-8144321.

webrev : http://cr.openjdk.java.net/~mli/8144321/webrev.00/

Thank you
-Hamlin

Re: RFR: 8144214 Some log messages will be discarded when VM is bootstrapping.

[Hamlin Li]

On 2015/12/1 18:32, Daniel Fuchs wrote:

Hi Hamlin,

I see that you're going to push a test for this with
JDK-8144215;
I will also ask for your help to push the test code after the test pass 
the open review.


Looks good to me.

Do you need a sponsor for this fix?

Hi Daniel,

Yes, I need a sponsor for this fix, it will be great if you could help 
to push the code. :-)

Please check the push message below :

8144214: Some log messages will be discarded when VM is bootstrapping
Summary: use logp rather than log.
Reviewed-by: dfuchs
Contributed-by: Hamlin Li <huaming...@oracle.com>

Thank you
-Hamlin



best regards,

-- daniel

On 30/11/15 12:28, Hamlin Li wrote:

Hi all,

Would you please help to review for
http://cr.openjdk.java.net/~mli/8144214/webrev.00/, which fixes bug
https://bugs.openjdk.java.net/browse/JDK-8144214.

Thank you
-Hamlin

Re: JDK 9 RFR of JDK-8144215: Test development task for : JEP-JDK-8046565: SQE Test Plan for Platform Logging API and Service

[Hamlin Li]

Hi Daniel,

Thanks for the review, I follow you suggestion to create a new RFE 
https://bugs.openjdk.java.net/browse/JDK-8144460 to track the pushing 
for this new test.

webrev : http://cr.openjdk.java.net/~mli/8144460/webrev.01/
old one is moved to http://cr.openjdk.java.net/~mli/8144460/webrev.00/

Thank you
-Hamlin

On 2015/12/1 18:40, Daniel Fuchs wrote:

Hi Hamlin,

You should probably create a new open RFE for pushing this new
test.
I'm not sure we can use internal task ids in commit/push comments.

From looking at the test, it would be preferable to create
the loggers after setting up the stub that pretend that the
VM is not yet booted. In other words - in BootstrapLoggerAPIsTest
lines 53-56 should preferably be moved after line 74.

best regards,

-- daniel

On 01/12/15 04:37, Hamlin Li wrote:

Hi all,

Would you please help to review the test development of JDK-8144215
<https://bugs.openjdk.java.net/browse/JDK-8144215>: Test development
task for : JEP-JDK-8046565: SQE Test Plan for Platform Logging API and
Service.
webrev : http://cr.openjdk.java.net/~mli/8144215/webrev.00/

Thank you
-Hamlin

JDK 9 RFR of JDK-8144215: Test development task for : JEP-JDK-8046565: SQE Test Plan for Platform Logging API and Service

[Hamlin Li]

Hi all,

Would you please help to review the test development of JDK-8144215 
: Test development 
task for : JEP-JDK-8046565: SQE Test Plan for Platform Logging API and 
Service.

webrev : http://cr.openjdk.java.net/~mli/8144215/webrev.00/

Thank you
-Hamlin

RFR: 8144214 Some log messages will be discarded when VM is bootstrapping.

[Hamlin Li]

Hi all,

Would you please help to review for 
http://cr.openjdk.java.net/~mli/8144214/webrev.00/, which fixes bug 
https://bugs.openjdk.java.net/browse/JDK-8144214.


Thank you
-Hamlin