Re: RFR 8207814: (proxy) upgrade the proxy class generator
Hi Roger, Thanks for doing this. Replacing ancient bytecode generators makes it much easier to support new features like value types. The diff of ProxyGenerator is not quite right (might be due to the rename?) as some code are unchanged but shown as modified. "jdk.proxy.ProxyGenerator.saveGeneratedFiles" is an existing property. Perhaps a better name for "jdk.proxy.proxy_15" would be "jdk.proxy.ProxyGenerator.useASM" with default true. Perhaps renaming ProxyGenerator_15 to LegacyProxyGenerator? The combo test is great that exercises different combinations. It validates the interfaces that the proxy class implements and the exception types in the methods of the proxy class. I checked the existing test/jdk/java/lang/reflect/Proxy tests. It looks like there is no proxy test with an invocation handler that throws checked exceptions and also undeclared exceptions. It'd be good if you can add that as a sanity test. Mandy On 8/16/19 12:15 PM, Roger Riggs wrote: Please review an enhancement to replace the java.lang.reflect.Proxy class file generation. The new generator uses ASM and generates stackmaps. The implementation follows the same structure as before but has many differences as it leverages ASM for generating the bytecode. A Combo test is included and two JMH based benchmarks. The ancient ProxyGenerator_15 implementation is temporarily retained to allow comparisons of generated class files and performance. Issue: https://bugs.openjdk.java.net/browse/JDK-8207814 Webrev: http://cr.openjdk.java.net/~rriggs/webrev-upgrade-proxy-gen-8207814/ (Upgrading bytecode generation is necessary for Valhalla but makes sense for the main line.) Thanks, Roger
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Thanks Claes,
I'll run some tests :)
Cheers,
Peter.
On 16/08/2019 9:14 PM, Claes Redestad wrote:
Hi Peter,
by explicitly ensuring the file system has been initialized before
installing a SecurityManager using a hook in System.setSecurityManager,
the patch at hand takes step to ensure things stay neutral w.r.t.
Permission initialization order when using any SecurityManager. It's not
perfectly identical, so while unlikely, there's a theoretical
possibility some implementation scenario not covered by our regression
tests might run into issues. Any help testing custom implementation on
coming EA builds would be greatly appreciated!
One thing we could do on the JDK end to reduce fragility somewhat in
this area is to provoke initialization of
sun.security.util.SecurityConstants before installing the first SM.
/Claes
On 2019-08-16 12:40, Peter Firmstone wrote:
Hello Alan,
Yes, we are aware of those issues.
I mean documenting that system Permission classes should be loaded
before setting a custom SecurityManager, accessing the file system is
important, so if you haven't loaded the necessary classes before
setting a custom SecurityManager, it won't be gracefull... The
simplest way of ensuring classes are loaded is by creating object
instances of them.
Perhaps just a note to beware of ensuring necessary classes are
loaded and let developers figure out what they need.
The recursive calls are easy enough to deal with to avoid any stack
overflows using ThreadLocal.
inTrustedCodeRecursiveCall.set(Boolean.TRUE);
try {
delegateContext = AccessController.doPrivileged(
new PrivilegedAction(){
public AccessControlContext run() {
return new
AccessControlContext(finalExecutionContext, dc);
}
}
);
}finally {
inTrustedCodeRecursiveCall.set(Boolean.FALSE); //
Must always happen, no matter what.
}
We've only really been caught out once with some jvm bootstrap
changes, otherwise it's been rock solid.
The other thing we do is once we've got more than three
ProtectionDomains on the stack is execute the ProtectionDomain
implies checks in parallel. Really helps when you're making a lot of
network calls.
Cheers,
Peter.
On 16/08/2019 5:04 PM, Alan Bateman wrote:
On 15/08/2019 23:20, Peter Firmstone wrote:
:
The following code is included in the constructor of our
SecurityManager implementation, I suspect we may need to add some
classes to this list, perhaps this is something that needs
documenting?
The checkPermission method of custom security manager can run
arbitrary code so recursive initialization, stack overflow,
bootstrap method errors, ... are always hazards. I don't know what
documentation you have in mind but I don't think there is a definite
list of classes that need to be loaded/initialized before the custom
security manager is set because it will come down to the code that
it executes in its checkPermission method.
-Alan.
Re: RFR:JDK-8229791: Code clean up regressions
Looks good. Thanks, Alexander On 8/16/2019 1:36 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8229791 [2] http://cr.openjdk.java.net/~herrick/8229791/webrev.02/ Thanks, Andy
Re: RFR: JDK-8213941: Debian linux problems in JavaPackager
I've created https://bugs.openjdk.java.net/browse/JDK-8229840 to track this task. - Alexey On 8/16/2019 5:04 PM, Alexander Matveev wrote: Looks good. Is it possible to have test for this new option? If yes, then as part of this fix or as separate issue should be fine. Thanks, Alexander On 8/16/2019 11:28 AM, Alexey Semenyuk wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). - add --linux-app-category command line option [1] https://bugs.openjdk.java.net/browse/JDK-8213941 2] http://cr.openjdk.java.net/~asemenyuk/8213941/webrev.00/ Thanks, Alexey
Re: RFR: JDK-8213941: Debian linux problems in JavaPackager
Looks good. Is it possible to have test for this new option? If yes, then as part of this fix or as separate issue should be fine. Thanks, Alexander On 8/16/2019 11:28 AM, Alexey Semenyuk wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). - add --linux-app-category command line option [1] https://bugs.openjdk.java.net/browse/JDK-8213941 2] http://cr.openjdk.java.net/~asemenyuk/8213941/webrev.00/ Thanks, Alexey
Re: RFR:JDK-8229791: Code clean up regressions
Looks good. - Alexey On 8/16/2019 4:36 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8229791 [2] http://cr.openjdk.java.net/~herrick/8229791/webrev.02/ Thanks, Andy
Re: JDK-8229786: No output after WinShortcutTest.exe is launched
Looks good. Thanks, Alexander On 8/16/2019 12:12 PM, Alexey Semenyuk wrote: Makes sense. Looks good then. - Alexey On 8/16/2019 3:08 PM, Andy Herrick wrote: On 8/16/2019 2:26 PM, Alexey Semenyuk wrote: Andy, I think we want this change in all Windows jtreg tests as they all use this windowless Hello test. yes - but except for testing launching app from shortcut of the installed app, the automated tests run the app and capture it's output, so only on these cases is--win-console arg needed to confirm the operation (creation of a shortcut by installer) succeeded. /Andy - Alexey On 8/16/2019 2:22 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8229786 [2] http://cr.openjdk.java.net/~herrick/8229786 Thanks, Andy
Re: RFR: JDK-8213941: Debian linux problems in JavaPackager
looks good /Andy On 8/16/2019 2:28 PM, Alexey Semenyuk wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). - add --linux-app-category command line option [1] https://bugs.openjdk.java.net/browse/JDK-8213941 2] http://cr.openjdk.java.net/~asemenyuk/8213941/webrev.00/ Thanks, Alexey
RFR:JDK-8229791: Code clean up regressions
Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8229791 [2] http://cr.openjdk.java.net/~herrick/8229791/webrev.02/ Thanks, Andy
Fwd: Re: Re[2]: Adaptive insertion sort in DualPivot Quicksort
Hi Vladimir, I performed lots of benchmarks: 1. I updated my own sort-bench (jmh based) and run many tests (all distributions + permutations) on integer arrays for sizes = 50, 100, 150, 200, 500, 1000, 2000: In this benchmark, the metric is (minimum + stddev) and it gives the overall score: in average, the new DPQS is 7% faster Comparing sorters DPQ_19_05_01 vs DPQ_19_08_09 winners : 349 / 321 stats (%): [1115: µ=107.99222 σ=45.746456 rms=153.73868 min=62.684937 max=524.28424] Full results are available: https://github.com/bourgesl/nearly-optimal-mergesort-code/blob/master/sort-bench/2019.08/sort-50-2000-stats.out 2. I ran Vladimir's test (from https://github.com/iaroslavski/sorting ): Before (07/24): # Data type = int, Sorting type = sequential, Length = 150 test_name;jdk_time;dpqs_time;gain random;1867.00;2074.00;-0.11 equal;224.00;98.00;0.56 ascending;476.00;112.00;0.76 descending;1185.00;260.00;0.78 period[4];564.00;782.00;-0.39 period[5];610.00;1076.00;-0.76 period[6];710.00;1028.00;-0.45 stagger[15];1788.00;2316.00;-0.30 stagger[24];709.00;1028.00;-0.45 stagger[33];2020.00;1934.00;0.04 shuffle[6];1624.00;1218.00;0.25 shuffle[7];1347.00;1189.00;0.12 shuffle[8];1345.00;1085.00;0.19 TOTAL;14469.00;14200.00;0.02 Patched (08/14): # Data type = int, Sorting type = sequential, Length = 150 test_name;jdk_time;dpqs_time;gain random;1881.00;1860.00;0.01 equal;223.00;98.00;0.56 ascending;478.00;111.00;0.77 descending;1192.00;262.00;0.78 period[4];564.00;660.00;-0.17 period[5];613.00;1014.00;-0.65 period[6];713.00;968.00;-0.36 stagger[15];1787.00;1893.00;-0.06 stagger[24];712.00;969.00;-0.36 stagger[33];2021.00;1890.00;0.06 shuffle[6];1618.00;1185.00;0.27 shuffle[7];1361.00;1129.00;0.17 shuffle[8];1341.00;1094.00;0.18 TOTAL;14504.00;13133.00;0.09 In conclusion, I confirm Vladimir's gain on my laptop (Intel(R) Core(TM) i7-6820HQ CPU @ 2.70GHz): size = 150, char, was: 0.02 -> new: 0.09 Finally both benchmarks agree 7% gain ! Cheers, Laurent
RFR 8207814: (proxy) upgrade the proxy class generator
Please review an enhancement to replace the java.lang.reflect.Proxy class file generation. The new generator uses ASM and generates stackmaps. The implementation follows the same structure as before but has many differences as it leverages ASM for generating the bytecode. A Combo test is included and two JMH based benchmarks. The ancient ProxyGenerator_15 implementation is temporarily retained to allow comparisons of generated class files and performance. Issue: https://bugs.openjdk.java.net/browse/JDK-8207814 Webrev: http://cr.openjdk.java.net/~rriggs/webrev-upgrade-proxy-gen-8207814/ (Upgrading bytecode generation is necessary for Valhalla but makes sense for the main line.) Thanks, Roger
Re: JDK-8229786: No output after WinShortcutTest.exe is launched
Makes sense. Looks good then. - Alexey On 8/16/2019 3:08 PM, Andy Herrick wrote: On 8/16/2019 2:26 PM, Alexey Semenyuk wrote: Andy, I think we want this change in all Windows jtreg tests as they all use this windowless Hello test. yes - but except for testing launching app from shortcut of the installed app, the automated tests run the app and capture it's output, so only on these cases is--win-console arg needed to confirm the operation (creation of a shortcut by installer) succeeded. /Andy - Alexey On 8/16/2019 2:22 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8229786 [2] http://cr.openjdk.java.net/~herrick/8229786 Thanks, Andy
Re: JDK-8229786: No output after WinShortcutTest.exe is launched
On 8/16/2019 2:26 PM, Alexey Semenyuk wrote: Andy, I think we want this change in all Windows jtreg tests as they all use this windowless Hello test. yes - but except for testing launching app from shortcut of the installed app, the automated tests run the app and capture it's output, so only on these cases is--win-console arg needed to confirm the operation (creation of a shortcut by installer) succeeded. /Andy - Alexey On 8/16/2019 2:22 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8229786 [2] http://cr.openjdk.java.net/~herrick/8229786 Thanks, Andy
RFR: JDK-8213941: Debian linux problems in JavaPackager
Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). - add --linux-app-category command line option [1] https://bugs.openjdk.java.net/browse/JDK-8213941 2] http://cr.openjdk.java.net/~asemenyuk/8213941/webrev.00/ Thanks, Alexey
Re: JDK-8229786: No output after WinShortcutTest.exe is launched
Andy, I think we want this change in all Windows jtreg tests as they all use this windowless Hello test. - Alexey On 8/16/2019 2:22 PM, Andy Herrick wrote: Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8229786 [2] http://cr.openjdk.java.net/~herrick/8229786 Thanks, Andy
JDK-8229786: No output after WinShortcutTest.exe is launched
Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] https://bugs.openjdk.java.net/browse/JDK-8229786 [2] http://cr.openjdk.java.net/~herrick/8229786 Thanks, Andy
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
+1 On 8/16/19 12:51 PM, Sean Mullan wrote: +1 from me as well. --Sean On 8/16/19 12:38 PM, Alan Bateman wrote: On 16/08/2019 13:30, Claes Redestad wrote: How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. Looks good. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
+1 from me as well. --Sean On 8/16/19 12:38 PM, Alan Bateman wrote: On 16/08/2019 13:30, Claes Redestad wrote: How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. Looks good. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 16/08/2019 13:30, Claes Redestad wrote: How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. Looks good. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 2019-08-15 21:21, Alan Bateman wrote: On 15/08/2019 16:22, Claes Redestad wrote: (adding back core-libs-dev) Hi Roger, seems easy enough to add a writeReplace: http://cr.openjdk.java.net/~redestad/8229773/webrev.02 This mostly looks good. In LazyCodeSourcePermissionCollection it think "initialize" should be renamed to "ensureAdded" as that more accurately describes what it does. Also the class description could do with a bit of word smiting to make it clear that permissions for a CodeSource are lazily added to the underlying permission collection. How about this: http://cr.openjdk.java.net/~redestad/8229773/webrev.03/ Also simplified BuiltinClassLoader#getPermissions since the jrt-specific optimization is now redundant. /Claes
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hi Peter,
by explicitly ensuring the file system has been initialized before
installing a SecurityManager using a hook in System.setSecurityManager,
the patch at hand takes step to ensure things stay neutral w.r.t.
Permission initialization order when using any SecurityManager. It's not
perfectly identical, so while unlikely, there's a theoretical
possibility some implementation scenario not covered by our regression
tests might run into issues. Any help testing custom implementation on
coming EA builds would be greatly appreciated!
One thing we could do on the JDK end to reduce fragility somewhat in
this area is to provoke initialization of
sun.security.util.SecurityConstants before installing the first SM.
/Claes
On 2019-08-16 12:40, Peter Firmstone wrote:
Hello Alan,
Yes, we are aware of those issues.
I mean documenting that system Permission classes should be loaded
before setting a custom SecurityManager, accessing the file system is
important, so if you haven't loaded the necessary classes before setting
a custom SecurityManager, it won't be gracefull... The simplest way of
ensuring classes are loaded is by creating object instances of them.
Perhaps just a note to beware of ensuring necessary classes are loaded
and let developers figure out what they need.
The recursive calls are easy enough to deal with to avoid any stack
overflows using ThreadLocal.
inTrustedCodeRecursiveCall.set(Boolean.TRUE);
try {
delegateContext = AccessController.doPrivileged(
new PrivilegedAction(){
public AccessControlContext run() {
return new
AccessControlContext(finalExecutionContext, dc);
}
}
);
}finally {
inTrustedCodeRecursiveCall.set(Boolean.FALSE); // Must
always happen, no matter what.
}
We've only really been caught out once with some jvm bootstrap changes,
otherwise it's been rock solid.
The other thing we do is once we've got more than three
ProtectionDomains on the stack is execute the ProtectionDomain implies
checks in parallel. Really helps when you're making a lot of network
calls.
Cheers,
Peter.
On 16/08/2019 5:04 PM, Alan Bateman wrote:
On 15/08/2019 23:20, Peter Firmstone wrote:
:
The following code is included in the constructor of our
SecurityManager implementation, I suspect we may need to add some
classes to this list, perhaps this is something that needs documenting?
The checkPermission method of custom security manager can run
arbitrary code so recursive initialization, stack overflow, bootstrap
method errors, ... are always hazards. I don't know what documentation
you have in mind but I don't think there is a definite list of classes
that need to be loaded/initialized before the custom security manager
is set because it will come down to the code that it executes in its
checkPermission method.
-Alan.
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
Hello Alan,
Yes, we are aware of those issues.
I mean documenting that system Permission classes should be loaded
before setting a custom SecurityManager, accessing the file system is
important, so if you haven't loaded the necessary classes before setting
a custom SecurityManager, it won't be gracefull... The simplest way of
ensuring classes are loaded is by creating object instances of them.
Perhaps just a note to beware of ensuring necessary classes are loaded
and let developers figure out what they need.
The recursive calls are easy enough to deal with to avoid any stack
overflows using ThreadLocal.
inTrustedCodeRecursiveCall.set(Boolean.TRUE);
try {
delegateContext = AccessController.doPrivileged(
new PrivilegedAction(){
public AccessControlContext run() {
return new
AccessControlContext(finalExecutionContext, dc);
}
}
);
}finally {
inTrustedCodeRecursiveCall.set(Boolean.FALSE); // Must
always happen, no matter what.
}
We've only really been caught out once with some jvm bootstrap changes,
otherwise it's been rock solid.
The other thing we do is once we've got more than three
ProtectionDomains on the stack is execute the ProtectionDomain implies
checks in parallel. Really helps when you're making a lot of network calls.
Cheers,
Peter.
On 16/08/2019 5:04 PM, Alan Bateman wrote:
On 15/08/2019 23:20, Peter Firmstone wrote:
:
The following code is included in the constructor of our
SecurityManager implementation, I suspect we may need to add some
classes to this list, perhaps this is something that needs documenting?
The checkPermission method of custom security manager can run
arbitrary code so recursive initialization, stack overflow, bootstrap
method errors, ... are always hazards. I don't know what documentation
you have in mind but I don't think there is a definite list of classes
that need to be loaded/initialized before the custom security manager
is set because it will come down to the code that it executes in its
checkPermission method.
-Alan.
Re: RFR: 8224974: Implement JEP 352
On 15/08/2019 12:53, Andrew Dinn wrote: Hi Alan, Any further input on this patch forthcoming? I think the main changes since I looked at it previously have been in the tests. On non-Linux platforms, FileChannel.map should throw UOE when invoked with a mode map of READ_ONLY_SYNC or READ_WRITE_SYNC so I think we need a test for that. MapFail seems fragile. If you add @modules java.base/jdk.internal.misc to the test description then it could use Unsafe::isWritebackEnabled and I think would simplify the test and checks. It would mean it could run on all platforms. Also "MapFail" is probably too general a name for a test in MBB because its specific to the extended map modes, not a general test of map failing. In passing, MappedByteBuffer load/isLoaded check the fd value before isSync, can force() do the same? Also the @return on the private isSync method is very wordy and I don't think needs to duplicate the method description. -Alan
Re: RFR: 8229773: Resolve permissions for code source URLs lazily
On 15/08/2019 23:20, Peter Firmstone wrote: : The following code is included in the constructor of our SecurityManager implementation, I suspect we may need to add some classes to this list, perhaps this is something that needs documenting? The checkPermission method of custom security manager can run arbitrary code so recursive initialization, stack overflow, bootstrap method errors, ... are always hazards. I don't know what documentation you have in mind but I don't think there is a definite list of classes that need to be loaded/initialized before the custom security manager is set because it will come down to the code that it executes in its checkPermission method. -Alan.
