Re: RFR: 8247532: Records deserialization is slow

[Peter Levart]

Hi Chris,


On 6/17/20 12:31 AM, Peter Levart wrote:
I did however notice that there is just a single test, 
DefaultValuesTest, that exercises different stream shapes for the 
same class in the stream. Would be good to expand coverage in this 
area, but of course some lower-level test-specific building blocks 
will be needed help build the specially crafted byte streams - I can 
help with this.


Overall I think that this change is good.

-Chris.

If there is a way for a test to compile several versions of the same 
(record) class and then produce byte streams with it, then we could 
simulate various class-evolution cases (field missing, surplus field, 
change of field type, etc...) without crafting the bytestream by hand. 
WDYT? 



I have a better idea. The test could contain several different classes 
with different names that would be used to generated variations of 
serialized stream. Such streams could then be "patched" so they would 
contain the common target class name and then used to (attempt to) 
deserialize the target class. I'll try to devise such test...



Regards, Peter

Re: RFR: 8247532: Records deserialization is slow

[Peter Levart]

Hi Remi,


The keys used are based on the ordered names and types of "fields" in 
the ObjectStreamClass that has been deserialized as part of the object 
stream for the record "type" being deserialized. So for each record type 
(Class) there can be several distinct 
ObjectStreamClasses deserialized in the same VM that were produced by 
serializing different versions of record types in different VMs or 
ClassLoader(s) and that may vary in the names and/or types and/or order 
of fields. And since the MethodHandle chain of transformations that is 
being used for a particular ObjectStreamClass is dependent on the 
ordered names and types of ObjectStreamField(s) of deserialized 
ObjectStreamClass, the key is based on that too.


You may ask why not simply add a MethodHandle field to the 
ObjectStreamClass instance that is deserialized and therefore unique? 
Well, such ObjectStreamClass only lasts for one "session" of 
ObjectStream deserialization, so such caching wouldn't be efficient. But 
common parts of that ObjectStreamClass that are only dependent on the 
current local-VM type are cached in a special instance of 
ObjectStreamClass and that's where I put a cache of deserializaiton 
constructors in the form of a Map.



Regards, Peter


On 6/17/20 1:06 AM, Remi Forax wrote:

Hi Peter,
is their a reason to not use a ClassValue using the record class 
as key instead of the more complex keys you are proposing ?

Rémi

- Mail original -

De: "Chris Hegarty" 
À: "Peter Levart" 
Cc: "core-libs-dev" 
Envoyé: Mardi 16 Juin 2020 17:15:03
Objet: Re: RFR: 8247532: Records deserialization is slow
Hi Peter,


On 14 Jun 2020, at 17:28, Peter Levart  wrote:

...

[2]
http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.01/


I think that this is very good. It’s clever to build a chain of method handles
that can be invoked with the stream field values. This is a nice separation
between the shape of the data and the actual stream data.

The caching is on a per-stream-field shape basis, which should be consistent in
the common case, but of course is not always guaranteed to be the case, hence
the need for the cache. I think that this should be fine, since the
ObjectStreamClass ( that holds the cache ) is already itself cached as a weak
reference. But I did wonder if the size of this new cache should be limited.
Probably not worth the complexity unless it is an obvious issue.

All the serailizable records tests pass successfully with your patch. Good. I
did however notice that there is just a single test, DefaultValuesTest, that
exercises different stream shapes for the same class in the stream. Would be
good to expand coverage in this area, but of course some lower-level
test-specific building blocks will be needed help build the specially crafted
byte streams - I can help with this.

Overall I think that this change is good.

-Chris.

Re: (15) RFR: JDK-8247444: Trust final fields in records

[John Rose]

On Jun 15, 2020, at 2:58 PM, Mandy Chung  wrote:
> 
> Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247444/webrev.00/ 
> 
> CSR: https://bugs.openjdk.java.net/browse/JDK-8247517 
> 

Good work.  Reviewed.  — John

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[David Holmes]

On 17/06/2020 2:45 am, Igor Ignatyev wrote:

Hi David,

thanks for your review. re: LingeredAppTest, I agree that the test doesn't look very 
useful, yet I'd remind that the goal of this (and other test in /test/lib-test) is to 
(sanity) test testlibrary in order to easily spot bugs in testlibrary in a clear manner 
so one would not have to investigate failures of actual "product" tests and go 
thru their sometimes convoluted logic just to find out that the problem was in 
LingeredApp. w/ that being said, this test can do a better job in testing LingeredApp, so 
I'll file a JBS ticket against hotspo/svc to evaluate/improve/discuss the fate of the 
test.


Given the test had not previously been run (it wouldn't even compile as 
far as I could see) it's validity and utility has to be called into 
question. But a follow up bug is fine for that as long it it currently 
works reliably.


Thanks,
David


Thanks,
-- Igor


On Jun 16, 2020, at 12:14 AM, David Holmes  wrote:

Hi Igor,

On 16/06/2020 10:39 am, Igor Ignatyev wrote:

@David, Erik, Magnus,
please find the answers to your comments at the bottom of this email.
@all,
David's and Erik's comments made me realize that some parts of the original 
patch were stashed away and didn't make it to webrev.00. I'm truly sorry for 
the confusion and inconvenience. I also agree w/ David that it's hard to 
follow/review, and my gaffe just made it worse, therefore I'd like to start it 
over again from a clean sate
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02

833 lines changed: 228 ins; 591 del; 14 mod;

could you please review the patch which puts all tests for common testlibrary classes 
(which is ones located in /test/lib) into one location under /test/lib-test? please note 
this intentionally doesn't move all "testlibrary" tests, e.g. tests for CTW, 
hotspot-specific testlibrary, are left in /test/hotspot/jtreg/testlibrary_tests/ctw .


Ok.


to simplify review, the patch has been split into several webrevs, with each of 
them accomplishes a more-or-less distinct thing, but is not necessary 
self-contained:


Many thanks for doing this!


0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites to 
test/lib-test:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/


Looks good.


1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and "merge" of 
hotspot's and jdk's OutputAnalyzerTest:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/


Looks good.


2. simplification of TestNativeProcessBuilder tests: converts Test class used 
by TestNativeProcessBuilder into a static nested class:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder


Looks good.


3. makefiles changes to build native parts of lib-test tests. in past, there 
were no tests w/ native parts in this test suite, TestNativeProcessBuilder is 
the 1st such test; this part also removes now unneeded lines from hotspot test 
suite makefile:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest


Hmm okay. Not sure this needed its own category of build logic but ...

Aside: Makefiles should not have the classpath exception version of the license 
header. But they all do for some reason. I'll check if we need to do a separate 
cleanup of that.


4. clean ups in hotspot test suite: adjusted location of 
SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test 
library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should not 
be moved to /test/lib-test; removed TestMutuallyExclusivePlatformPredicates 
from TEST.groups:
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/


Looks good. Took me a while to understand the connection to the test library 
here.


5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/


Someone from serviceability should evaluate this test. It may not be needed.


6. changes to make test/lib-test a fully supported and working test suite, and 
add in into tier1,  includes adding ProblemList, TEST.groups file, and 
'randomness' k/w:
http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/


Seems okay.

Thanks,
David
-


webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing:
  - make test TEST=tier1 locally on macosx
  - test/lib-test on  {windows,linux,macosx}-x64
  - tier1 job (in progress)
Thanks,
-- Igor

On Jun 14, 2020, at 11:23 PM, David Holmes mailto:david.hol...@oracle.com>> wrote:
<...>
This doesn't seem to move everything under 
test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.

this is intentionally, ctw test-library is hotspot specific, hence its tests 
are to reside in hotspot test suite (until we decide to move the library to 
/test/lib), the same is true for SimpleClassFileLoadHookTest.


You did not modify 

Re: RFR: 8247532: Records deserialization is slow

[Paul Sandoz]

> On Jun 16, 2020, at 3:36 PM, Peter Levart  wrote:
> 
> Yes, or even better: MethodHandles.empty(MethodType.methodType(pClass, 
> byte[].class, Object[].class)) which was suggested by Johanes Kuhn as here:
> 
> 
> http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.02/
> 

Ah yes much better.
Paul.

Re: (15) RFR: JDK-8247444: Trust final fields in records

[Mandy Chung]

Hi David,

Thanks for the review.

On 6/15/20 7:29 PM, David Holmes wrote:

Hi Mandy, Christoph,

The code changes here all look okay to me. The idea of introducing the 
notion of "trusted final fields" seems quite reasonable.


I made one editorial comment on the CSR request.



Thanks.
I'm unclear if the new TEST.properties file needs a copyright notice 
and header. We have 706 such files in the repo and 554 (mostly 
hotspot) do have the copyright notice and header.




I will find out the recommendation and update you.

Mandy


Thanks,
David

On 16/06/2020 7:58 am, Mandy Chung wrote:

This patch is joint contribution from Christoph Dreis [1] and me.

Webrev: 
http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247444/webrev.00/

CSR: https://bugs.openjdk.java.net/browse/JDK-8247517

This proposes to make final fields in records notmodifiable via 
reflection.  Field::set throws IAE if a Field is not modifiable. 
Thecurrent specification specifies the following Fields not modifiable:

- static final fields in any class
- final fields in a hidden class

The spec of Field::set is changed to specify that records are not 
modifiable via reflection.
  Noe that no change in Field::setAccessible(true), i.e. it will 
succeed to allow existing frameworks to have read access to final 
fields in records.  Just no write access.


VarHandle does not support write access if it's a static final field 
or an instance final field.


This patch also proposes `sun.misc.Unsafe::objectFieldOffset` and 
`sun.misc.Unsafe::staticField{Offset/Base}` not to support records.


No change is made in JNI.  JNI could be considered to disallow 
modification of final fields in records and hidden classes (static 
and instance fields).  But JNI has superpower and the current spec 
already allows to modify the value of a final static field even after 
it's constant folded (via JNI SetField and 
SetStaticField), then all bets are off.  This should be 
re-visited when we consider "final is truly final" for all classes.


Make final fields in records not modifiable via reflection enables 
JIT optimization as these final fields are effectively truly final.


This change impacts 3rd-party frameworks including 3rd-party 
serialization framework that rely on core reflection `setAccessible` 
or `sun.misc.Unsafe::allocateInstance` and `objectFieldOffset` etc to 
construct records but not using the canonical constructor.
These frameworks would need to be updated to construct records via 
its canonical constructor as done by the Java serialization.


I see this change gives a good opportunity to engage the maintainers 
of the serialization frameworks and work together to support new 
features including records, inline classes and the new serialization 
mechanism and which I think it is worth the investment.


This is a low risk enhancement.  I'd like to request approval for a 
late enhancement in JDK 15.  It extends the pre-existing code path 
with refactoring the hidden classes to prepare for new kinds of 
classes with trusted final fields.  The change is straight-forward.


Can this wait to integrate in JDK 16?

   It's important to get this enhancement in when record is a preview 
feature that we can get feedback and give time to 3rd-party 
serialization frameworks to look into adding the support for records. 
If we delayed this change in 16 and records exit preview, it would be 
bad for frameworks if they verify that they support records in 15 but 
fail in 16.  OTOH the risk of this patch is low.


Performance Impact

I addressed the performance concern I raised earlier myself. For 
reflection, VM creates the reflective Field objects and fills in 
MemberName when resolving a member.  VM will tag if this 
Field/MemberName is trusted final field.  I think this is a cleaner 
approach rather than in each place to check for final static and 
final fields in hidden or record class to determine if it has write 
access or not.


`sun.misc.Unsafe` does not use Field::isTrustedFinalField because (1) 
Unsafe has been allowing access of static final fields of any classes 
but isTrustedFinalField is not limited to instance fields (2) Unsafe 
disallows access to all fields in a hidden class (not limited to 
trusted final fields).  So it follows the precedence and simply 
checks if the declaring class is a record. `Class::isRecord` calls 
`Class::getSuperclass` to check if it's a subtype of `Record`.  As 
`Class::getSuperclass` is intrinsified, the call on isRecord on a 
normal class is fast. Christoph has contributed the microbenchmarks 
that confirm that no performance regression.


Thanks
Mandy
[1] 
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-June/040096.html 

Re: RFR: 8247532: Records deserialization is slow

[Remi Forax]

Hi Peter,
is their a reason to not use a ClassValue using the record class 
as key instead of the more complex keys you are proposing ?

Rémi

- Mail original -
> De: "Chris Hegarty" 
> À: "Peter Levart" 
> Cc: "core-libs-dev" 
> Envoyé: Mardi 16 Juin 2020 17:15:03
> Objet: Re: RFR: 8247532: Records deserialization is slow

> Hi Peter,
> 
>> On 14 Jun 2020, at 17:28, Peter Levart  wrote:
>> 
>> ...
>> 
>> [2]
>> http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.01/
>> 
> I think that this is very good. It’s clever to build a chain of method handles
> that can be invoked with the stream field values. This is a nice separation
> between the shape of the data and the actual stream data.
> 
> The caching is on a per-stream-field shape basis, which should be consistent 
> in
> the common case, but of course is not always guaranteed to be the case, hence
> the need for the cache. I think that this should be fine, since the
> ObjectStreamClass ( that holds the cache ) is already itself cached as a weak
> reference. But I did wonder if the size of this new cache should be limited.
> Probably not worth the complexity unless it is an obvious issue.
> 
> All the serailizable records tests pass successfully with your patch. Good. I
> did however notice that there is just a single test, DefaultValuesTest, that
> exercises different stream shapes for the same class in the stream. Would be
> good to expand coverage in this area, but of course some lower-level
> test-specific building blocks will be needed help build the specially crafted
> byte streams - I can help with this.
> 
> Overall I think that this change is good.
> 
> -Chris.

Re: RFR: JDK-8264244: BasicShortcutHintTest shortcut can not be found

[Andy Herrick]

looks good with these links

/Andy

On 6/16/2020 6:13 PM, Alexander Matveev wrote:

Hi Alexey,

Looks good. I think you got links and bug ID incorrect. It should be 
JDK-8246244 and you have 8264244. Links also does not work. Working 
links are:

https://bugs.openjdk.java.net/browse/JDK-8246244
http://cr.openjdk.java.net/~asemenyuk/8246244/webrev.00/

Thanks,
Alexander

On 6/16/2020 2:56 PM, Alexey Semenyuk wrote:

Please review fix [2] for jpackage bug [1].

The fix is to put value of `Exec` property of .desktop files 
generated by jpackage in double quotes if the value of the propery 
contains whitespace characters.


- Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8264244

[2] http://cr.openjdk.java.net/~asemenyuk/8264244/webrev.00

Re: RFR: 8247532: Records deserialization is slow

[Peter Levart]

Hi Paul,

On 6/16/20 5:50 PM, Paul Sandoz wrote:

Hi Peter,

  199 private Map deserializationCtrs;

Change to be either ConcurrentMap or ConcurrentHashMap, thereby making it 
clearer when using.



Sure. Will do that.





2679 private static MethodHandle defaultValueExtractorFor(Class 
pType) {

Can the implementation use MethodHandles,zero? e.g.

   return MethodHandles.dropArguments(MethodHandles.zero(pType), 0, 
byte[].class, Object[].class);
  
Paul.



Yes, or even better: MethodHandles.empty(MethodType.methodType(pClass, 
byte[].class, Object[].class)) which was suggested by Johanes Kuhn as here:



http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.02/


Regards, Peter





On Jun 16, 2020, at 8:15 AM, Chris Hegarty  wrote:

Hi Peter,


On 14 Jun 2020, at 17:28, Peter Levart  wrote:

...

[2] 
http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.01/


I think that this is very good. It’s clever to build a chain of method handles 
that can be invoked with the stream field values. This is a nice separation 
between the shape of the data and the actual stream data.

The caching is on a per-stream-field shape basis, which should be consistent in 
the common case, but of course is not always guaranteed to be the case, hence 
the need for the cache. I think that this should be fine, since the 
ObjectStreamClass ( that holds the cache ) is already itself cached as a weak 
reference. But I did wonder if the size of this new cache should be limited. 
Probably not worth the complexity unless it is an obvious issue.

All the serailizable records tests pass successfully with your patch. Good. I 
did however notice that there is just a single test, DefaultValuesTest, that 
exercises different stream shapes for the same class in the stream. Would be 
good to expand coverage in this area, but of course some lower-level 
test-specific building blocks will be needed help build the specially crafted 
byte streams - I can help with this.

Overall I think that this change is good.

-Chris.

Re: RFR: 8247532: Records deserialization is slow

[Peter Levart]

Hi Chris,

On 6/16/20 5:15 PM, Chris Hegarty wrote:

Hi Peter,


On 14 Jun 2020, at 17:28, Peter Levart  wrote:

...

[2] 
http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.01/


I think that this is very good. It’s clever to build a chain of method handles 
that can be invoked with the stream field values. This is a nice separation 
between the shape of the data and the actual stream data.

The caching is on a per-stream-field shape basis, which should be consistent in 
the common case, but of course is not always guaranteed to be the case, hence 
the need for the cache. I think that this should be fine, since the 
ObjectStreamClass ( that holds the cache ) is already itself cached as a weak 
reference. But I did wonder if the size of this new cache should be limited. 
Probably not worth the complexity unless it is an obvious issue.

All the serailizable records tests pass successfully with your patch. Good. I 
did however notice that there is just a single test, DefaultValuesTest, that 
exercises different stream shapes for the same class in the stream. Would be 
good to expand coverage in this area, but of course some lower-level 
test-specific building blocks will be needed help build the specially crafted 
byte streams - I can help with this.

Overall I think that this change is good.

-Chris.

If there is a way for a test to compile several versions of the same 
(record) class and then produce byte streams with it, then we could 
simulate various class-evolution cases (field missing, surplus field, 
change of field type, etc...) without crafting the bytestream by hand. WDYT?



Regards, Peter

RFR: JDK-8264244: BasicShortcutHintTest shortcut can not be found

[Alexey Semenyuk]

Please review fix [2] for jpackage bug [1].

The fix is to put value of `Exec` property of .desktop files generated 
by jpackage in double quotes if the value of the propery contains 
whitespace characters.


- Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8264244

[2] http://cr.openjdk.java.net/~asemenyuk/8264244/webrev.00

Re: RFR: JDK-8264244: BasicShortcutHintTest shortcut can not be found

[Alexander Matveev]

Hi Alexey,

Looks good. I think you got links and bug ID incorrect. It should be 
JDK-8246244 and you have 8264244. Links also does not work. Working 
links are:

https://bugs.openjdk.java.net/browse/JDK-8246244
http://cr.openjdk.java.net/~asemenyuk/8246244/webrev.00/

Thanks,
Alexander

On 6/16/2020 2:56 PM, Alexey Semenyuk wrote:

Please review fix [2] for jpackage bug [1].

The fix is to put value of `Exec` property of .desktop files generated 
by jpackage in double quotes if the value of the propery contains 
whitespace characters.


- Alexey

[1] https://bugs.openjdk.java.net/browse/JDK-8264244

[2] http://cr.openjdk.java.net/~asemenyuk/8264244/webrev.00

RFR: JDK-8213214: Set -Djava.io.tmpdir= when running tests

[Erik Joelsson]

(re-sending this as it doesn't look like it was delivered)

There are a lot of jtreg tests that use temporary files. These temporary 
files add up over time and fill up the global temp directories on our 
test systems. To tackle this, we should try to redirect these temporary 
files into a directory controlled by the test framework. Jtreg does not 
do this, but we can set -Djava.io.tmpdir from RunTest.gmk. This will not 
prevent all temp files from being created outside of the work dir, but 
at least most.



I have found one test where this becomes an issue, 
java/nio/file/Path/Misc.java on Windows when running in elevated mode 
with the workspace on a subst drive. This looks like an actual issue in 
the product, so I have filed a separate bug for it [1]. Since we 
currently use subst in our distributed test system to get around Windows 
path length issues, we are hitting this problem. While the bug is being 
evaluated, I have implemented a workaround in the test so that it is 
able to handle the known situation. I would like to have someone from 
core-libs look at the workaround.


Webrev: http://cr.openjdk.java.net/~erikj/8213214/webrev.01/

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

/Erik

[1] https://bugs.openjdk.java.net/browse/JDK-8213216

RFR: JDK-8213214: Set -Djava.io.tmpdir= when running tests

[Erik Joelsson]

There are a lot of jtreg tests that use temporary files. These temporary 
files add up over time and fill up the global temp directories on our 
test systems. To tackle this, we should try to redirect these temporary 
files into a directory controlled by the test framework. Jtreg does not 
do this, but we can set -Djava.io.tmpdir from RunTest.gmk. This will not 
prevent all temp files from being created outside of the work dir, but 
at least most.


I have found one test where this becomes an issue, 
java/nio/file/Path/Misc.java on Windows when running in elevated mode 
with the workspace on a subst drive. This looks like an actual issue in 
the product, so I have filed a separate bug for it [1]. Since we 
currently use subst in our distributed test system to get around Windows 
path length issues, we are hitting this problem. While the bug is being 
evaluated, I have implemented a workaround in the test so that it is 
able to handle the known situation. I would like to have someone from 
core-libs look at the workaround.


Webrev: http://cr.openjdk.java.net/~erikj/8213214/webrev.01/

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

/Erik

[1] https://bugs.openjdk.java.net/browse/JDK-8213216

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Igor Ignatyev]

Magnus, Erik,

thanks for your reviews, pushed w/ a newline being added at L#654 of 
make/Main.gmk.

Cheers,
-- Igor

> On Jun 16, 2020, at 7:03 AM, Magnus Ihse Bursie 
>  wrote:
> 
> On 2020-06-16 15:06, Erik Joelsson wrote:
>> 
>> On 2020-06-15 17:39, Igor Ignatyev wrote:
>>> @David, Erik, Magnus,
>>> 
>>> please find the answers to your comments at the bottom of this email.
>>> 
>>> @all,
>>> 
>>> David's and Erik's comments made me realize that some parts of the original 
>>> patch were stashed away and didn't make it to webrev.00. I'm truly sorry 
>>> for the confusion and inconvenience. I also agree w/ David that it's hard 
>>> to follow/review, and my gaffe just made it worse, therefore I'd like to 
>>> start it over again from a clean sate
>>> 
>>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02 
>>> Build changes 
>>> look good to me now, except for a missing newline in Main.gmk. (No need for 
>>> new review.)
>> 
> 
> Ditto.
> 
> /Magnus
>> 
>> /Erik
>> 
 833 lines changed: 228 ins; 591 del; 14 mod; 
>>> 
>>> could you please review the patch which puts all tests for common 
>>> testlibrary classes (which is ones located in /test/lib) into one location 
>>> under /test/lib-test? please note this intentionally doesn't move all 
>>> "testlibrary" tests, e.g. tests for CTW, hotspot-specific testlibrary, are 
>>> left in /test/hotspot/jtreg/testlibrary_tests/ctw .
>>> 
>>> to simplify review, the patch has been split into several webrevs, with 
>>> each of them accomplishes a more-or-less distinct thing, but is not 
>>> necessary self-contained:
>>> 
>>> 0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites 
>>> to test/lib-test:
>>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/ 
>>> 
>>> 
>>> 1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and 
>>> "merge" of hotspot's and jdk's OutputAnalyzerTest:
>>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/ 
>>> 
>>> 
>>> 2. simplification of TestNativeProcessBuilder tests: converts Test class 
>>> used by TestNativeProcessBuilder into a static nested class:
>>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder
>>>  
>>> 
>>> 
>>> 3. makefiles changes to build native parts of lib-test tests. in past, 
>>> there were no tests w/ native parts in this test suite, 
>>> TestNativeProcessBuilder is the 1st such test; this part also removes now 
>>> unneeded lines from hotspot test suite makefile:
>>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest 
>>> 
>>> 
>>> 4. clean ups in hotspot test suite: adjusted location of 
>>> SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test 
>>> library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should 
>>> not be moved to /test/lib-test; removed 
>>> TestMutuallyExclusivePlatformPredicates from TEST.groups:
>>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/ 
>>> 
>>> 
>>> 5. LingeredAppTest fix (which apparently has never been run before):
>>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/ 
>>> 
>>> 
>>> 6. changes to make test/lib-test a fully supported and working test suite, 
>>> and add in into tier1,  includes adding ProblemList, TEST.groups file, and 
>>> 'randomness' k/w:
>>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/ 
>>> 
>>> 
>>> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02 
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977 
>>> 
>>> testing: 
>>>  - make test TEST=tier1 locally on macosx
>>>  - test/lib-test on  {windows,linux,macosx}-x64
>>>  - tier1 job (in progress)
>>> 
>>> Thanks,
>>> -- Igor
>>> 
>>> 
 On Jun 14, 2020, at 11:23 PM, David Holmes >>> > wrote:
 <...>
>>> 
 This doesn't seem to move everything under 
 test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
>>> this is intentionally, ctw test-library is hotspot specific, hence its 
>>> tests are to reside in hotspot test suite (until we decide to move the 
>>> library to /test/lib), the same is true for SimpleClassFileLoadHookTest.
 
 You did not modify hotspot/jtreg/TEST.groups but it refers to:
 

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Igor Ignatyev]

Hi David,

thanks for your review. re: LingeredAppTest, I agree that the test doesn't look 
very useful, yet I'd remind that the goal of this (and other test in 
/test/lib-test) is to (sanity) test testlibrary in order to easily spot bugs in 
testlibrary in a clear manner so one would not have to investigate failures of 
actual "product" tests and go thru their sometimes convoluted logic just to 
find out that the problem was in LingeredApp. w/ that being said, this test can 
do a better job in testing LingeredApp, so I'll file a JBS ticket against 
hotspo/svc to evaluate/improve/discuss the fate of the test.

Thanks,
-- Igor

> On Jun 16, 2020, at 12:14 AM, David Holmes  wrote:
> 
> Hi Igor,
> 
> On 16/06/2020 10:39 am, Igor Ignatyev wrote:
>> @David, Erik, Magnus,
>> please find the answers to your comments at the bottom of this email.
>> @all,
>> David's and Erik's comments made me realize that some parts of the original 
>> patch were stashed away and didn't make it to webrev.00. I'm truly sorry for 
>> the confusion and inconvenience. I also agree w/ David that it's hard to 
>> follow/review, and my gaffe just made it worse, therefore I'd like to start 
>> it over again from a clean sate
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
>>> 833 lines changed: 228 ins; 591 del; 14 mod; 
>> could you please review the patch which puts all tests for common 
>> testlibrary classes (which is ones located in /test/lib) into one location 
>> under /test/lib-test? please note this intentionally doesn't move all 
>> "testlibrary" tests, e.g. tests for CTW, hotspot-specific testlibrary, are 
>> left in /test/hotspot/jtreg/testlibrary_tests/ctw .
> 
> Ok.
> 
>> to simplify review, the patch has been split into several webrevs, with each 
>> of them accomplishes a more-or-less distinct thing, but is not necessary 
>> self-contained:
> 
> Many thanks for doing this!
> 
>> 0. trivial move of tests from test/jdk and test/hotspot/jtreg test suites to 
>> test/lib-test:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/
> 
> Looks good.
> 
>> 1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and "merge" 
>> of hotspot's and jdk's OutputAnalyzerTest:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/
> 
> Looks good.
> 
>> 2. simplification of TestNativeProcessBuilder tests: converts Test class 
>> used by TestNativeProcessBuilder into a static nested class:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder
> 
> Looks good.
> 
>> 3. makefiles changes to build native parts of lib-test tests. in past, there 
>> were no tests w/ native parts in this test suite, TestNativeProcessBuilder 
>> is the 1st such test; this part also removes now unneeded lines from hotspot 
>> test suite makefile:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest
> 
> Hmm okay. Not sure this needed its own category of build logic but ...
> 
> Aside: Makefiles should not have the classpath exception version of the 
> license header. But they all do for some reason. I'll check if we need to do 
> a separate cleanup of that.
> 
>> 4. clean ups in hotspot test suite: adjusted location of 
>> SimpleClassFileLoadHookTest test, which is a test for hotspot-specific test 
>> library (located in /test/hotspot/jtreg/testlibrary/jvmti) and hence should 
>> not be moved to /test/lib-test; removed 
>> TestMutuallyExclusivePlatformPredicates from TEST.groups:
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/
> 
> Looks good. Took me a while to understand the connection to the test library 
> here.
> 
>> 5. LingeredAppTest fix (which apparently has never been run before):
>> http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/
> 
> Someone from serviceability should evaluate this test. It may not be needed.
> 
>> 6. changes to make test/lib-test a fully supported and working test suite, 
>> and add in into tier1,  includes adding ProblemList, TEST.groups file, and 
>> 'randomness' k/w:
>> http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/
> 
> Seems okay.
> 
> Thanks,
> David
> -
> 
>> webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
>> testing:
>>  - make test TEST=tier1 locally on macosx
>>  - test/lib-test on  {windows,linux,macosx}-x64
>>  - tier1 job (in progress)
>> Thanks,
>> -- Igor
>>> On Jun 14, 2020, at 11:23 PM, David Holmes >> > wrote:
>>> <...>
>>> This doesn't seem to move everything under 
>>> test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
>> this is intentionally, ctw test-library is hotspot specific, hence its tests 
>> are to reside in hotspot test suite (until we decide to move the library to 
>> /test/lib), the same is true for SimpleClassFileLoadHookTest.
>>> 
>>> You did not modify hotspot/jtreg/TEST.groups but it refers to:
>>> 

Re: (15) RFR: JDK-8247444: Trust final fields in records

[Mandy Chung]

On 6/16/20 2:49 AM, Remi Forax wrote:

Hi Mndy,
Looks good,
I don't like too much the fact that there is a new boolean field in j.l.r.Field 
instead of using the modifiers like in the VM counterpart,
but it will require masking and unmasking the modifiers which is a far bigger 
change, too big to worth it in my opinion.


Adding a new field instead of using the modifiers is a deliberate choice 
from my side.  It's higher risk to overload the modifiers to include 
VM-specific flags.   I am all for a better clean up (valhalla lworld 
does use the modifiers to cover a flag indicating if it's flattened 
which I am not satisfied either).



So +1


Thanks Remi.

Mandy


Rémi

- Mail original -

De: "mandy chung" 
À: "hotspot-dev" , "core-libs-dev" 
, "amber-dev"

Envoyé: Lundi 15 Juin 2020 23:58:19
Objet: (15) RFR: JDK-8247444: Trust final fields in records
This patch is joint contribution from Christoph Dreis [1] and me.

Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247444/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8247517

This proposes to make final fields in records notmodifiable via
reflection.  Field::set throws IAE if a Field is not modifiable.
Thecurrent specification specifies the following Fields not modifiable:
- static final fields in any class
- final fields in a hidden class

The spec of Field::set is changed to specify that records are not
modifiable via reflection.
  Noe that no change in Field::setAccessible(true), i.e. it will succeed
to allow existing frameworks to have read access to final fields in
records.  Just no write access.

VarHandle does not support write access if it's a static final field or
an instance final field.

This patch also proposes `sun.misc.Unsafe::objectFieldOffset` and
`sun.misc.Unsafe::staticField{Offset/Base}` not to support records.

No change is made in JNI.  JNI could be considered to disallow
modification of final fields in records and hidden classes (static and
instance fields).  But JNI has superpower and the current spec already
allows to modify the value of a final static field even after it's
constant folded (via JNI SetField and SetStaticField), then
all bets are off.  This should be re-visited when we consider "final is
truly final" for all classes.

Make final fields in records not modifiable via reflection enables JIT
optimization as these final fields are effectively truly final.

This change impacts 3rd-party frameworks including 3rd-party
serialization framework that rely on core reflection `setAccessible` or
`sun.misc.Unsafe::allocateInstance` and `objectFieldOffset` etc to
construct records but not using the canonical constructor.
These frameworks would need to be updated to construct records via its
canonical constructor as done by the Java serialization.

I see this change gives a good opportunity to engage the maintainers of
the serialization frameworks and work together to support new features
including records, inline classes and the new serialization mechanism
and which I think it is worth the investment.

This is a low risk enhancement.  I'd like to request approval for a late
enhancement in JDK 15.  It extends the pre-existing code path with
refactoring the hidden classes to prepare for new kinds of classes with
trusted final fields.  The change is straight-forward.

Can this wait to integrate in JDK 16?

   It's important to get this enhancement in when record is a preview
feature that we can get feedback and give time to 3rd-party
serialization frameworks to look into adding the support for records.
If we delayed this change in 16 and records exit preview, it would be
bad for frameworks if they verify that they support records in 15 but
fail in 16.  OTOH the risk of this patch is low.

Performance Impact

I addressed the performance concern I raised earlier myself.  For
reflection, VM creates the reflective Field objects and fills in
MemberName when resolving a member.  VM will tag if this
Field/MemberName is trusted final field.  I think this is a cleaner
approach rather than in each place to check for final static and final
fields in hidden or record class to determine if it has write access or
not.

`sun.misc.Unsafe` does not use Field::isTrustedFinalField because (1)
Unsafe has been allowing access of static final fields of any classes
but isTrustedFinalField is not limited to instance fields (2) Unsafe
disallows access to all fields in a hidden class (not limited to trusted
final fields).  So it follows the precedence and simply checks if the
declaring class is a record. `Class::isRecord` calls
`Class::getSuperclass` to check if it's a subtype of `Record`.  As
`Class::getSuperclass` is intrinsified, the call on isRecord on a normal
class is fast. Christoph has contributed the microbenchmarks that
confirm that no performance regression.

Thanks
Mandy
[1]
https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-June/040096.html

Re: RFR[16]: 8247681: Improve bootstrapping of unary concatenations

[Paul Sandoz]

Looks good.

It’s tempting to do something like this to de-dup the code with less potential 
for mistakes:

String s = null;
// Select the singular non-null value, if any
If (s0 != null && s1 == null) s = s0;
else if (s0 == null && s1 != null) s = s1;
If (s != null) {
if (s.isEmpty()) {
…
} else if (…) { … }
}

Paul.

> On Jun 16, 2020, at 6:00 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> this patch specializes bootstrapping of unary concatenation expressions.
> 
> Such expressions can be reduced to the canonical String.valueOf
> "stringifier" for any primitive argument, but needs a special
> stringifier for reference arguments since we need to produce a new
> String to be compliant with JLS.
> 
> Bug:https://bugs.openjdk.java.net/browse/JDK-8247681
> Webrev: http://cr.openjdk.java.net/~redestad/8247681/open.00/
> 
> This ensures we get a similar speed-up as JDK-8247605 for
> "" + fooString (and fooString + "").
> 
> Also speeds up bootstrapping for all such simple unary concatenation
> expressions.
> 
> Testing: tier1+2
> 
> Before:
> 
> Benchmark(intValue)  Mode  Cnt   Score Error  
> Units
> StringConcat.concatEmptyConstInt   4711  avgt5  15.539 ± 0.831  
> ns/op
> StringConcat.concatEmptyConstString4711  avgt5  17.046 ± 1.047  
> ns/op
> StringConcat.concatEmptyLeft   4711  avgt5   7.506 ± 0.588  
> ns/op
> StringConcat.concatEmptyRight  4711  avgt5   7.890 ± 0.314  
> ns/op
> 
> After:
> Benchmark(intValue)  Mode  Cnt   Score Error  
> Units
> StringConcat.concatEmptyConstInt   4711  avgt5  15.410 ± 0.944  
> ns/op
> StringConcat.concatEmptyConstString4711  avgt5   7.397 ± 0.384  
> ns/op
> StringConcat.concatEmptyLeft   4711  avgt5   7.465 ± 0.328  
> ns/op
> StringConcat.concatEmptyRight  4711  avgt5   7.857 ± 0.355  
> ns/op
> 
> Thanks!
> 
> /Claes

Re: RFR: 8247532: Records deserialization is slow

[Paul Sandoz]

Hi Peter,

 199 private Map deserializationCtrs;

Change to be either ConcurrentMap or ConcurrentHashMap, thereby making it 
clearer when using.


2679 private static MethodHandle defaultValueExtractorFor(Class 
pType) {

Can the implementation use MethodHandles,zero? e.g.

  return MethodHandles.dropArguments(MethodHandles.zero(pType), 0, 
byte[].class, Object[].class);
 
Paul.

> On Jun 16, 2020, at 8:15 AM, Chris Hegarty  wrote:
> 
> Hi Peter,
> 
>> On 14 Jun 2020, at 17:28, Peter Levart  wrote:
>> 
>> ...
>> 
>> [2] 
>> http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.01/
>> 
> I think that this is very good. It’s clever to build a chain of method 
> handles that can be invoked with the stream field values. This is a nice 
> separation between the shape of the data and the actual stream data.
> 
> The caching is on a per-stream-field shape basis, which should be consistent 
> in the common case, but of course is not always guaranteed to be the case, 
> hence the need for the cache. I think that this should be fine, since the 
> ObjectStreamClass ( that holds the cache ) is already itself cached as a weak 
> reference. But I did wonder if the size of this new cache should be limited. 
> Probably not worth the complexity unless it is an obvious issue.
> 
> All the serailizable records tests pass successfully with your patch. Good. I 
> did however notice that there is just a single test, DefaultValuesTest, that 
> exercises different stream shapes for the same class in the stream. Would be 
> good to expand coverage in this area, but of course some lower-level 
> test-specific building blocks will be needed help build the specially crafted 
> byte streams - I can help with this.
> 
> Overall I think that this change is good.
> 
> -Chris.
> 

Re: RFR: 8247532: Records deserialization is slow

[Chris Hegarty]

Hi Peter,

> On 14 Jun 2020, at 17:28, Peter Levart  wrote:
> 
> ...
> 
> [2] 
> http://cr.openjdk.java.net/~plevart/jdk-dev/RecordsDeserialization/webrev.01/
> 
I think that this is very good. It’s clever to build a chain of method handles 
that can be invoked with the stream field values. This is a nice separation 
between the shape of the data and the actual stream data.

The caching is on a per-stream-field shape basis, which should be consistent in 
the common case, but of course is not always guaranteed to be the case, hence 
the need for the cache. I think that this should be fine, since the 
ObjectStreamClass ( that holds the cache ) is already itself cached as a weak 
reference. But I did wonder if the size of this new cache should be limited. 
Probably not worth the complexity unless it is an obvious issue.

All the serailizable records tests pass successfully with your patch. Good. I 
did however notice that there is just a single test, DefaultValuesTest, that 
exercises different stream shapes for the same class in the stream. Would be 
good to expand coverage in this area, but of course some lower-level 
test-specific building blocks will be needed help build the specially crafted 
byte streams - I can help with this.

Overall I think that this change is good.

-Chris.

Re: [PATCH] 8246633: Improve the performance of ObjectInputStream.resolveClass(ObjectStreamClass)

[Roger Riggs]

Hi Peter,

I think you've hardwired an assumption about the contents of the stack 
into VM.latestUserDefineLoader...


On 6/12/20 7:19 PM, Peter Kessler (Open Source) wrote:

Roger,

I did think about confining the changes to ObjectInputStream.  Maybe I
did not think hard enough about it.

Keeping a cache of the result of a first stack walk might work for the
recursive calls.  One might still do a useless stack walk for each
top-level readObject call.  No concurrent class loading can invalidate
the cache for any particular stack walk.  But it seems fraught to
reuse a cached value in the face of concurrent class loading, or in
the presence of overloaded ObjectInputStream methods that might create
new ClassLoaders on this stack.  I'm sure I don't want to debug that.

My experiments have shown that the average stack walk is about 20
frames.  I do not have the data on how many of those frames are
recursive calls within ObjectInputStream methods and how many are in
other code.

I'd be interested in a histogram of the depths and in which applications.

Dragging in the StackWalker API would further complicate things, in my
opinion.  It might appear to keep the code out of platform native
code, but we know that walking thread stacks involves native code.

I settled on a system-wide one-bit cache: There have been no
user-defined ClassLoaders constructed, so I can not find one on any
particular stack walk.  It may not be the best possible solution, but
it addresses the issue of useless stack walks in the common case where
there are no user-defined ClassLoaders.

I agree that both ClassLoader and ObjectInputStream are complex.  In
fact, the proposed patch makes no changes to ObjectInputStream.  There
is one line added to each of the system-defined ClassLoaders to
identify them as not user-defined ClassLoaders, one line in the base
ClassLoader constructor, and the bulk of the change is around the
guard in jdk.internal.misc.VM.latestUserDefinedLoader() to avoid the
stack walk if possible.


The spec and implementation of OIS.resolveClass do not make any assumptions
about whether serialization is invoked from a class on the 
PlatformLoader, AppClassLoader,

or another loader.

In the case of a 'system' thread or class calling OIS.readObject, the 
change you are proposing

will force class loading in resolveClass to the AppClassLoader.

In VM.latestUserDefinedLoader:390 caught my eye because it forces the 
AppClassLoader.
It assumes that scanning the stack would find and return the 
AppClassLoader, not so in all cases.


Regards, Roger




 ... peter

-Original Message-
From: Roger Riggs 
Organization: Java Products Group, Oracle
Date: Friday, June 12, 2020 at 11:50 AM
To: "Peter Kessler (Open Source)" , 
"core-libs-dev@openjdk.java.net" 
Subject: Re: [PATCH] 8246633: Improve the performance of 
ObjectInputStream.resolveClass(ObjectStreamClass)

 Hi Peter,

 The hazard to avoid is the cross package coupling that makes both
 ClassLoader and
 ObjectInputStream more complex; both are more than sufficiently complex
 already.

 Optimizing the implementations make sense if it benefits enough uses
 cases and
 does not make the code harder to maintain and evolve.

 Have you explored any alternatives that can be confined to OIS?
 For example, taking advantage the many calls to readObject within
 a single OIS that are nested. Possibly the first (depth = 0) find user
 class loader
 can be re-used or at least to short circuit the search on subsequent
 nested calls?
 Or caching the classloader at each depth and only need to probe for it
 if it has
 not already been recorded. The StackWalker API may be useful to limit
 the depth
 of searches.

 Thanks, Roger



 On 6/11/20 2:52 PM, Peter Kessler OS wrote:
 > Hi Roger,
 >
 > Thanks for your comments.
 >
 > I am not knowingly changing the required behavior. All I am proposing is
 > guarding the thread stack walk if it will be a waste of effort. If an
 > application or framework has its own ClassLoaders, then the guard will 
fail
 > and the thread stack will be walked.
 >
 > The stack walk is important, if there are user-defined ClassLoaders. A
 > method that uses ObjectInputStream to read objects can not know what
 > ClassLoader to use to resolve class references in the stream, without the
 > stack walk. But if there never have been any user-defined ClassLoaders
 > constructed, then the stack walk can never return a user-defined
 > ClassLoader.
 >
 > If the stream reader is part of some simple application, then it may 
spend
 > a significant amount of its time uselessly walking thread stacks. (If the
 > stream reader is part of some simple application its author may not know 
to
 > override ObjectInputStream.resolveClass, or how to determine what
 > ClassLoader should load a class from 

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Magnus Ihse Bursie]

On 2020-06-16 15:06, Erik Joelsson wrote:



On 2020-06-15 17:39, Igor Ignatyev wrote:

@David, Erik, Magnus,

please find the answers to your comments at the bottom of this email.

@all,

David's and Erik's comments made me realize that some parts of the 
original patch were stashed away and didn't make it to webrev.00. I'm 
truly sorry for the confusion and inconvenience. I also agree w/ 
David that it's hard to follow/review, and my gaffe just made it 
worse, therefore I'd like to start it over again from a clean sate


http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02


Build changes look good to me now, except for a missing newline in 
Main.gmk. (No need for new review.)




Ditto.

/Magnus


/Erik

833 lines changed: 228 ins; 591 del; 14 mod; 


could you please review the patch which puts all tests for common 
testlibrary classes (which is ones located in /test/lib) into one 
location under /test/lib-test? please note this intentionally doesn't 
move all "testlibrary" tests, e.g. tests for CTW, hotspot-specific 
testlibrary, are left in /test/hotspot/jtreg/testlibrary_tests/ctw .


to simplify review, the patch has been split into several webrevs, 
with each of them accomplishes a more-or-less distinct thing, but is 
not necessary self-contained:


0. trivial move of tests from test/jdk and test/hotspot/jtreg test 
suites to test/lib-test:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/

1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and 
"merge" of hotspot's and jdk's OutputAnalyzerTest:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/

2. simplification of TestNativeProcessBuilder tests: converts Test 
class used by TestNativeProcessBuilder into a static nested class:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder

3. makefiles changes to build native parts of lib-test tests. in 
past, there were no tests w/ native parts in this test suite, 
TestNativeProcessBuilder is the 1st such test; this part also removes 
now unneeded lines from hotspot test suite makefile:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest

4. clean ups in hotspot test suite: adjusted location 
of SimpleClassFileLoadHookTest test, which is a test for 
hotspot-specific test library (located in 
/test/hotspot/jtreg/testlibrary/jvmti) and hence should not be moved 
to /test/lib-test; removed TestMutuallyExclusivePlatformPredicates 
from TEST.groups:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/

5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/

6. changes to make test/lib-test a fully supported and working test 
suite, and add in into tier1,  includes adding ProblemList, 
TEST.groups file, and 'randomness' k/w:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/

webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing:
 - make test TEST=tier1 locally on macosx
 - test/lib-test on  {windows,linux,macosx}-x64
 - tier1 job (in progress)

Thanks,
-- Igor


On Jun 14, 2020, at 11:23 PM, David Holmes > wrote:

<...>
This doesn't seem to move everything under 
test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
this is intentionally, ctw test-library is hotspot specific, hence 
its tests are to reside in hotspot test suite (until we decide to 
move the library to /test/lib), the same is true 
for SimpleClassFileLoadHookTest.


You did not modify hotspot/jtreg/TEST.groups but it refers to:
testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \
Plus it should probably refer to the new native_sanity tests somewhere.
the actual version of the patch removed reference to 
TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups 
and had TestNativeProcessBuilder moved to /test/test-lib, so no 
updates w.r.t. native_sanity are needed


The newly added copyright notice needs to have the correct initial 
copyright year based on when the file was first added to the repo.
 /test/lib-test/TEST.ROOT file was created by JDK-8243010 on 
2020-04-16, hence the added copyright has 2020 as the initial 
copyright year.


You used the wrong license header - makefiles don't use the 
classpath exception.
JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which 
also have classpath exception. quick grep showed that make directory 
has 794 files which has '"Classpath" exception', out of 850 which 
contain 'GNU General Public License version 2' string. And although I 
agree that makefiles shouldn't have the classpath exception, I'd 
prefer to keep JtregNativeLibTest.gmk in sync w/ the its siblings and 
would leave it up to build team to decide how it's best to handle.



On Jun 15, 2020, at 8:12 AM, Magnus Ihse Bursie 
> 

Re: RFR[16]: 8247681: Improve bootstrapping of unary concatenations

[Jim Laskey]

> On Jun 16, 2020, at 10:20 AM, Claes Redestad  
> wrote:
> 
> On 2020-06-16 15:09, Jim Laskey wrote:
>> Would it be helpful to have benchmarks for the other types, just in case?
> 
> I'm not sure.. We need to be selective or we end up increasing expected
> run time by a large factor. We should also do a pass over most of these
> micros to provide saner defaults (number of forks, warmup times, run
> time etc are generally tuned a bit too high and we can get reasonable
> results on a smaller time budget)

okay.

> 
>> Don't see any tests to ensure all cases are covered. May be relying on 
>> existing tests, but... minimally tag those tests.
> 
> test/jdk/java/lang/String/concat/ImplicitStringConcatShapes.java tests
> these unary shapes exhaustively. I'll add the bug number to that (and
> the .template for it)

+1


> 
> /Claes
> 
>> Cheers,
>> -- Jim
>>> On Jun 16, 2020, at 10:00 AM, Claes Redestad  
>>> wrote:
>>> 
>>> Hi,
>>> 
>>> this patch specializes bootstrapping of unary concatenation expressions.
>>> 
>>> Such expressions can be reduced to the canonical String.valueOf
>>> "stringifier" for any primitive argument, but needs a special
>>> stringifier for reference arguments since we need to produce a new
>>> String to be compliant with JLS.
>>> 
>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8247681
>>> Webrev: http://cr.openjdk.java.net/~redestad/8247681/open.00/
>>> 
>>> This ensures we get a similar speed-up as JDK-8247605 for
>>> "" + fooString (and fooString + "").
>>> 
>>> Also speeds up bootstrapping for all such simple unary concatenation
>>> expressions.
>>> 
>>> Testing: tier1+2
>>> 
>>> Before:
>>> 
>>> Benchmark(intValue)  Mode  Cnt   Score Error  
>>> Units
>>> StringConcat.concatEmptyConstInt   4711  avgt5  15.539 ± 0.831  
>>> ns/op
>>> StringConcat.concatEmptyConstString4711  avgt5  17.046 ± 1.047  
>>> ns/op
>>> StringConcat.concatEmptyLeft   4711  avgt5   7.506 ± 0.588  
>>> ns/op
>>> StringConcat.concatEmptyRight  4711  avgt5   7.890 ± 0.314  
>>> ns/op
>>> 
>>> After:
>>> Benchmark(intValue)  Mode  Cnt   Score Error  
>>> Units
>>> StringConcat.concatEmptyConstInt   4711  avgt5  15.410 ± 0.944  
>>> ns/op
>>> StringConcat.concatEmptyConstString4711  avgt5   7.397 ± 0.384  
>>> ns/op
>>> StringConcat.concatEmptyLeft   4711  avgt5   7.465 ± 0.328  
>>> ns/op
>>> StringConcat.concatEmptyRight  4711  avgt5   7.857 ± 0.355  
>>> ns/op
>>> 
>>> Thanks!
>>> 
>>> /Claes

Re: RFR[16]: 8247681: Improve bootstrapping of unary concatenations

[Claes Redestad]

On 2020-06-16 15:09, Jim Laskey wrote:

Would it be helpful to have benchmarks for the other types, just in case?


I'm not sure.. We need to be selective or we end up increasing expected
run time by a large factor. We should also do a pass over most of these
micros to provide saner defaults (number of forks, warmup times, run
time etc are generally tuned a bit too high and we can get reasonable
results on a smaller time budget)



Don't see any tests to ensure all cases are covered. May be relying on existing 
tests, but... minimally tag those tests.


test/jdk/java/lang/String/concat/ImplicitStringConcatShapes.java tests
these unary shapes exhaustively. I'll add the bug number to that (and
the .template for it)

/Claes



Cheers,

-- Jim



On Jun 16, 2020, at 10:00 AM, Claes Redestad  wrote:

Hi,

this patch specializes bootstrapping of unary concatenation expressions.

Such expressions can be reduced to the canonical String.valueOf
"stringifier" for any primitive argument, but needs a special
stringifier for reference arguments since we need to produce a new
String to be compliant with JLS.

Bug:https://bugs.openjdk.java.net/browse/JDK-8247681
Webrev: http://cr.openjdk.java.net/~redestad/8247681/open.00/

This ensures we get a similar speed-up as JDK-8247605 for
"" + fooString (and fooString + "").

Also speeds up bootstrapping for all such simple unary concatenation
expressions.

Testing: tier1+2

Before:

Benchmark(intValue)  Mode  Cnt   Score Error  Units
StringConcat.concatEmptyConstInt   4711  avgt5  15.539 ± 0.831  
ns/op
StringConcat.concatEmptyConstString4711  avgt5  17.046 ± 1.047  
ns/op
StringConcat.concatEmptyLeft   4711  avgt5   7.506 ± 0.588  
ns/op
StringConcat.concatEmptyRight  4711  avgt5   7.890 ± 0.314  
ns/op

After:
Benchmark(intValue)  Mode  Cnt   Score Error  Units
StringConcat.concatEmptyConstInt   4711  avgt5  15.410 ± 0.944  
ns/op
StringConcat.concatEmptyConstString4711  avgt5   7.397 ± 0.384  
ns/op
StringConcat.concatEmptyLeft   4711  avgt5   7.465 ± 0.328  
ns/op
StringConcat.concatEmptyRight  4711  avgt5   7.857 ± 0.355  
ns/op

Thanks!

/Claes

Re: RFR[16]: 8247681: Improve bootstrapping of unary concatenations

[Jim Laskey]

Would it be helpful to have benchmarks for the other types, just in case? 

Don't see any tests to ensure all cases are covered. May be relying on existing 
tests, but... minimally tag those tests.

Cheers,

-- Jim


> On Jun 16, 2020, at 10:00 AM, Claes Redestad  
> wrote:
> 
> Hi,
> 
> this patch specializes bootstrapping of unary concatenation expressions.
> 
> Such expressions can be reduced to the canonical String.valueOf
> "stringifier" for any primitive argument, but needs a special
> stringifier for reference arguments since we need to produce a new
> String to be compliant with JLS.
> 
> Bug:https://bugs.openjdk.java.net/browse/JDK-8247681
> Webrev: http://cr.openjdk.java.net/~redestad/8247681/open.00/
> 
> This ensures we get a similar speed-up as JDK-8247605 for
> "" + fooString (and fooString + "").
> 
> Also speeds up bootstrapping for all such simple unary concatenation
> expressions.
> 
> Testing: tier1+2
> 
> Before:
> 
> Benchmark(intValue)  Mode  Cnt   Score Error  
> Units
> StringConcat.concatEmptyConstInt   4711  avgt5  15.539 ± 0.831  
> ns/op
> StringConcat.concatEmptyConstString4711  avgt5  17.046 ± 1.047  
> ns/op
> StringConcat.concatEmptyLeft   4711  avgt5   7.506 ± 0.588  
> ns/op
> StringConcat.concatEmptyRight  4711  avgt5   7.890 ± 0.314  
> ns/op
> 
> After:
> Benchmark(intValue)  Mode  Cnt   Score Error  
> Units
> StringConcat.concatEmptyConstInt   4711  avgt5  15.410 ± 0.944  
> ns/op
> StringConcat.concatEmptyConstString4711  avgt5   7.397 ± 0.384  
> ns/op
> StringConcat.concatEmptyLeft   4711  avgt5   7.465 ± 0.328  
> ns/op
> StringConcat.concatEmptyRight  4711  avgt5   7.857 ± 0.355  
> ns/op
> 
> Thanks!
> 
> /Claes

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[Erik Joelsson]

On 2020-06-15 17:39, Igor Ignatyev wrote:

@David, Erik, Magnus,

please find the answers to your comments at the bottom of this email.

@all,

David's and Erik's comments made me realize that some parts of the 
original patch were stashed away and didn't make it to webrev.00. I'm 
truly sorry for the confusion and inconvenience. I also agree w/ David 
that it's hard to follow/review, and my gaffe just made it worse, 
therefore I'd like to start it over again from a clean sate


http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02


Build changes look good to me now, except for a missing newline in 
Main.gmk. (No need for new review.)


/Erik

833 lines changed: 228 ins; 591 del; 14 mod; 


could you please review the patch which puts all tests for common 
testlibrary classes (which is ones located in /test/lib) into one 
location under /test/lib-test? please note this intentionally doesn't 
move all "testlibrary" tests, e.g. tests for CTW, hotspot-specific 
testlibrary, are left in /test/hotspot/jtreg/testlibrary_tests/ctw .


to simplify review, the patch has been split into several webrevs, 
with each of them accomplishes a more-or-less distinct thing, but is 
not necessary self-contained:


0. trivial move of tests from test/jdk and test/hotspot/jtreg test 
suites to test/lib-test:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/

1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and 
"merge" of hotspot's and jdk's OutputAnalyzerTest:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/

2. simplification of TestNativeProcessBuilder tests: converts Test 
class used by TestNativeProcessBuilder into a static nested class:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder

3. makefiles changes to build native parts of lib-test tests. in past, 
there were no tests w/ native parts in this test suite, 
TestNativeProcessBuilder is the 1st such test; this part also removes 
now unneeded lines from hotspot test suite makefile:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest

4. clean ups in hotspot test suite: adjusted location 
of SimpleClassFileLoadHookTest test, which is a test for 
hotspot-specific test library (located in 
/test/hotspot/jtreg/testlibrary/jvmti) and hence should not be moved 
to /test/lib-test; removed TestMutuallyExclusivePlatformPredicates 
from TEST.groups:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/

5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/

6. changes to make test/lib-test a fully supported and working test 
suite, and add in into tier1,  includes adding ProblemList, 
TEST.groups file, and 'randomness' k/w:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/

webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing:
 - make test TEST=tier1 locally on macosx
 - test/lib-test on  {windows,linux,macosx}-x64
 - tier1 job (in progress)

Thanks,
-- Igor


On Jun 14, 2020, at 11:23 PM, David Holmes > wrote:

<...>
This doesn't seem to move everything under 
test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
this is intentionally, ctw test-library is hotspot specific, hence its 
tests are to reside in hotspot test suite (until we decide to move the 
library to /test/lib), the same is true for SimpleClassFileLoadHookTest.


You did not modify hotspot/jtreg/TEST.groups but it refers to:
testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \
Plus it should probably refer to the new native_sanity tests somewhere.
the actual version of the patch removed reference to 
TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups and 
had TestNativeProcessBuilder moved to /test/test-lib, so no updates 
w.r.t. native_sanity are needed


The newly added copyright notice needs to have the correct initial 
copyright year based on when the file was first added to the repo.
 /test/lib-test/TEST.ROOT file was created by JDK-8243010 on 
2020-04-16, hence the added copyright has 2020 as the initial 
copyright year.


You used the wrong license header - makefiles don't use the classpath 
exception.
JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which 
also have classpath exception. quick grep showed that make directory 
has 794 files which has '"Classpath" exception', out of 850 which 
contain 'GNU General Public License version 2' string. And although I 
agree that makefiles shouldn't have the classpath exception, I'd 
prefer to keep JtregNativeLibTest.gmk in sync w/ the its siblings and 
would leave it up to build team to decide how it's best to handle.



On Jun 15, 2020, at 8:12 AM, Magnus Ihse Bursie 
> wrote:


A few comments:

This seems like code copied from 

RFR[16]: 8247681: Improve bootstrapping of unary concatenations

[Claes Redestad]

Hi,

this patch specializes bootstrapping of unary concatenation expressions.

Such expressions can be reduced to the canonical String.valueOf
"stringifier" for any primitive argument, but needs a special
stringifier for reference arguments since we need to produce a new
String to be compliant with JLS.

Bug:https://bugs.openjdk.java.net/browse/JDK-8247681
Webrev: http://cr.openjdk.java.net/~redestad/8247681/open.00/

This ensures we get a similar speed-up as JDK-8247605 for
"" + fooString (and fooString + "").

Also speeds up bootstrapping for all such simple unary concatenation
expressions.

Testing: tier1+2

Before:

Benchmark(intValue)  Mode  Cnt   Score 
Error  Units
StringConcat.concatEmptyConstInt   4711  avgt5  15.539 ± 
0.831  ns/op
StringConcat.concatEmptyConstString4711  avgt5  17.046 ± 
1.047  ns/op
StringConcat.concatEmptyLeft   4711  avgt5   7.506 ± 
0.588  ns/op
StringConcat.concatEmptyRight  4711  avgt5   7.890 ± 
0.314  ns/op


After:
Benchmark(intValue)  Mode  Cnt   Score 
Error  Units
StringConcat.concatEmptyConstInt   4711  avgt5  15.410 ± 
0.944  ns/op
StringConcat.concatEmptyConstString4711  avgt5   7.397 ± 
0.384  ns/op
StringConcat.concatEmptyLeft   4711  avgt5   7.465 ± 
0.328  ns/op
StringConcat.concatEmptyRight  4711  avgt5   7.857 ± 
0.355  ns/op


Thanks!

/Claes

Re: (15) RFR: JDK-8247444: Trust final fields in records

[Remi Forax]

Hi Mndy,
Looks good,
I don't like too much the fact that there is a new boolean field in j.l.r.Field 
instead of using the modifiers like in the VM counterpart,
but it will require masking and unmasking the modifiers which is a far bigger 
change, too big to worth it in my opinion.

So +1

Rémi

- Mail original -
> De: "mandy chung" 
> À: "hotspot-dev" , "core-libs-dev" 
> , "amber-dev"
> 
> Envoyé: Lundi 15 Juin 2020 23:58:19
> Objet: (15) RFR: JDK-8247444: Trust final fields in records

> This patch is joint contribution from Christoph Dreis [1] and me.
> 
> Webrev: http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8247444/webrev.00/
> CSR: https://bugs.openjdk.java.net/browse/JDK-8247517
> 
> This proposes to make final fields in records notmodifiable via
> reflection.  Field::set throws IAE if a Field is not modifiable.
> Thecurrent specification specifies the following Fields not modifiable:
> - static final fields in any class
> - final fields in a hidden class
> 
> The spec of Field::set is changed to specify that records are not
> modifiable via reflection.
>  Noe that no change in Field::setAccessible(true), i.e. it will succeed
> to allow existing frameworks to have read access to final fields in
> records.  Just no write access.
> 
> VarHandle does not support write access if it's a static final field or
> an instance final field.
> 
> This patch also proposes `sun.misc.Unsafe::objectFieldOffset` and
> `sun.misc.Unsafe::staticField{Offset/Base}` not to support records.
> 
> No change is made in JNI.  JNI could be considered to disallow
> modification of final fields in records and hidden classes (static and
> instance fields).  But JNI has superpower and the current spec already
> allows to modify the value of a final static field even after it's
> constant folded (via JNI SetField and SetStaticField), then
> all bets are off.  This should be re-visited when we consider "final is
> truly final" for all classes.
> 
> Make final fields in records not modifiable via reflection enables JIT
> optimization as these final fields are effectively truly final.
> 
> This change impacts 3rd-party frameworks including 3rd-party
> serialization framework that rely on core reflection `setAccessible` or
> `sun.misc.Unsafe::allocateInstance` and `objectFieldOffset` etc to
> construct records but not using the canonical constructor.
> These frameworks would need to be updated to construct records via its
> canonical constructor as done by the Java serialization.
> 
> I see this change gives a good opportunity to engage the maintainers of
> the serialization frameworks and work together to support new features
> including records, inline classes and the new serialization mechanism
> and which I think it is worth the investment.
> 
> This is a low risk enhancement.  I'd like to request approval for a late
> enhancement in JDK 15.  It extends the pre-existing code path with
> refactoring the hidden classes to prepare for new kinds of classes with
> trusted final fields.  The change is straight-forward.
> 
> Can this wait to integrate in JDK 16?
> 
>   It's important to get this enhancement in when record is a preview
> feature that we can get feedback and give time to 3rd-party
> serialization frameworks to look into adding the support for records.
> If we delayed this change in 16 and records exit preview, it would be
> bad for frameworks if they verify that they support records in 15 but
> fail in 16.  OTOH the risk of this patch is low.
> 
> Performance Impact
> 
> I addressed the performance concern I raised earlier myself.  For
> reflection, VM creates the reflective Field objects and fills in
> MemberName when resolving a member.  VM will tag if this
> Field/MemberName is trusted final field.  I think this is a cleaner
> approach rather than in each place to check for final static and final
> fields in hidden or record class to determine if it has write access or
> not.
> 
> `sun.misc.Unsafe` does not use Field::isTrustedFinalField because (1)
> Unsafe has been allowing access of static final fields of any classes
> but isTrustedFinalField is not limited to instance fields (2) Unsafe
> disallows access to all fields in a hidden class (not limited to trusted
> final fields).  So it follows the precedence and simply checks if the
> declaring class is a record. `Class::isRecord` calls
> `Class::getSuperclass` to check if it's a subtype of `Record`.  As
> `Class::getSuperclass` is intrinsified, the call on isRecord on a normal
> class is fast. Christoph has contributed the microbenchmarks that
> confirm that no performance regression.
> 
> Thanks
> Mandy
> [1]
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-June/040096.html

Re: RFR(S) : 8211977 : move testlibrary tests into one place

[David Holmes]

Hi Igor,

On 16/06/2020 10:39 am, Igor Ignatyev wrote:

@David, Erik, Magnus,

please find the answers to your comments at the bottom of this email.

@all,

David's and Erik's comments made me realize that some parts of the 
original patch were stashed away and didn't make it to webrev.00. I'm 
truly sorry for the confusion and inconvenience. I also agree w/ David 
that it's hard to follow/review, and my gaffe just made it worse, 
therefore I'd like to start it over again from a clean sate


http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
833 lines changed: 228 ins; 591 del; 14 mod; 


could you please review the patch which puts all tests for common 
testlibrary classes (which is ones located in /test/lib) into one 
location under /test/lib-test? please note this intentionally doesn't 
move all "testlibrary" tests, e.g. tests for CTW, hotspot-specific 
testlibrary, are left in /test/hotspot/jtreg/testlibrary_tests/ctw .


Ok.

to simplify review, the patch has been split into several webrevs, with 
each of them accomplishes a more-or-less distinct thing, but is not 
necessary self-contained:


Many thanks for doing this!

0. trivial move of tests from test/jdk and test/hotspot/jtreg test 
suites to test/lib-test:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.00-move/


Looks good.

1. removal of hotspot's AssertsTest, OutputAnalyzerReportingTest and 
"merge" of hotspot's and jdk's OutputAnalyzerTest:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.01-merge/


Looks good.

2. simplification of TestNativeProcessBuilder tests: converts Test class 
used by TestNativeProcessBuilder into a static nested class:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.02-TestNativeProcessBuilder


Looks good.

3. makefiles changes to build native parts of lib-test tests. in past, 
there were no tests w/ native parts in this test suite, 
TestNativeProcessBuilder is the 1st such test; this part also removes 
now unneeded lines from hotspot test suite makefile:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.03-NativeLibTest


Hmm okay. Not sure this needed its own category of build logic but ...

Aside: Makefiles should not have the classpath exception version of the 
license header. But they all do for some reason. I'll check if we need 
to do a separate cleanup of that.


4. clean ups in hotspot test suite: adjusted location 
of SimpleClassFileLoadHookTest test, which is a test for 
hotspot-specific test library (located in 
/test/hotspot/jtreg/testlibrary/jvmti) and hence should not be moved to 
/test/lib-test; removed TestMutuallyExclusivePlatformPredicates from 
TEST.groups:

http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.04-hotspot/


Looks good. Took me a while to understand the connection to the test 
library here.



5. LingeredAppTest fix (which apparently has never been run before):
http://cr.openjdk.java.net/~iignatyev/8211977/webrev.02.05-LingeredAppTest/


Someone from serviceability should evaluate this test. It may not be needed.

6. changes to make test/lib-test a fully supported and working test 
suite, and add in into tier1,  includes adding ProblemList, TEST.groups 
file, and 'randomness' k/w:

http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02.06-TESTROOT/


Seems okay.

Thanks,
David
-


webrev: http://cr.openjdk.java.net/~iignatyev//8211977/webrev.02
JBS: https://bugs.openjdk.java.net/browse/JDK-8211977
testing:
  - make test TEST=tier1 locally on macosx
  - test/lib-test on  {windows,linux,macosx}-x64
  - tier1 job (in progress)

Thanks,
-- Igor


On Jun 14, 2020, at 11:23 PM, David Holmes > wrote:

<...>
This doesn't seem to move everything under 
test/hotspot/jtreg/testlibrary_tests/ e.g. ctw directory.
this is intentionally, ctw test-library is hotspot specific, hence its 
tests are to reside in hotspot test suite (until we decide to move the 
library to /test/lib), the same is true for SimpleClassFileLoadHookTest.


You did not modify hotspot/jtreg/TEST.groups but it refers to:
testlibrary_tests/TestMutuallyExclusivePlatformPredicates.java \
Plus it should probably refer to the new native_sanity tests somewhere.
the actual version of the patch removed reference to 
TestMutuallyExclusivePlatformPredicates from hotspot's TEST.groups and 
had TestNativeProcessBuilder moved to /test/test-lib, so no updates 
w.r.t. native_sanity are needed


The newly added copyright notice needs to have the correct initial 
copyright year based on when the file was first added to the repo.
  /test/lib-test/TEST.ROOT file was created by JDK-8243010 on 
2020-04-16, hence the added copyright has 2020 as the initial copyright 
year.


You used the wrong license header - makefiles don't use the classpath 
exception.
JtregNativeLibTest.gmk is based on make/test/JtregNativeJdk.gmk which 
also have classpath exception. quick grep showed that make directory has 
794 files which has '"Classpath" exception', out of 850 which