Re: Review Request JDK-8186050: StackFrame should provide the method signature

2017-08-29 Thread Brent Christian
For providing something informational, option #3 is a good choice. It's simple (no need for new StackWalker access options), yet sufficient. The changes look good to me. Thanks, -Brent On 08/28/2017 07:37 PM, mandy chung wrote: On 8/28/17 4:23 PM, Paul Sandoz wrote: Hi Mandy,

Re: RFR [10] 8186217 : Remove erroneous @hidden JavaDoc tag from java.util.Properties.replace(Object, Object, Object)

2017-08-25 Thread Brent Christian
Thanks! -B On 08/25/2017 10:29 AM, Brian Burkhalter wrote: +2 On Aug 25, 2017, at 10:28 AM, Naoto Sato <naoto.s...@oracle.com <mailto:naoto.s...@oracle.com>> wrote: +1 Naoto On 8/25/17 10:03 AM, Brent Christian wrote: Hi, Please review this simple doc fi

RFR [10] 8186217 : Remove erroneous @hidden JavaDoc tag from java.util.Properties.replace(Object, Object, Object)

2017-08-25 Thread Brent Christian
Hi, Please review this simple doc fix for: https://bugs.openjdk.java.net/browse/JDK-8186217 When working on JDK-8029891, solutions were sought to avoid cluttering up the Properties JavaDoc with new overridden methods. The @hidden tag was considered, then later rejected. Here[1] is the

Re: RFR: 8186334: JarFile throws ArrayIndexOutOfBoundsException when the manifest contains certain characters

2017-08-21 Thread Brent Christian
Hi, Claes Fix looks good, test looks good. Thanks, -Brent On 08/21/2017 02:53 AM, Claes Redestad wrote: Hi, this patch addresses an unfortunate regression where backtick characters in a manifest can cause an AIOOBE. Webrev: http://cr.openjdk.java.net/~redestad/8186334/jdk.00/ Bug:

Re: RFR: JDK-8186160 Fix a11y issues in java.security package

2017-08-14 Thread Brent Christian
FYI, I plan to fix this by way of: https://bugs.openjdk.java.net/browse/JDK-8186217 Remove erroneous @hidden JavaDoc tag from java.util.Properties.replace(Object, Object, Object) -Brent On 8/14/17 10:02 AM, Brent Christian wrote: Hi, Max This tag snuck in by mistake. Before pushing

Re: RFR: JDK-8186160 Fix a11y issues in java.security package

2017-08-14 Thread Brent Christian
Hi, Max This tag snuck in by mistake. Before pushing the fix for JDK-8029891, we looked into ways to avoid addition of JavaDoc for all the trivial method overrides. I tried adding @hidden tags, but they didn't do what we wanted, so they were taken out. Except one - sorry about that.

Re: [10] RFR (XS) 8183902: Remove unnecessary definitions in locale_str.h for macOS

2017-07-19 Thread Brent Christian
The fix looks good, Naoto. Thanks, -Brent On 7/18/17 3:23 PM, Naoto Sato wrote: Hello, Please review this small fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8183902 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8183902/webrev.00/ This is a

Re: [10] RFR (XS) 8183902: Remove unnecessary definitions in locale_str.h for macOS

2017-07-19 Thread Brent Christian
The fix looks good, Naoto. Thanks, -Brent On 7/18/17 3:23 PM, Naoto Sato wrote: Hello, Please review this small fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8183902 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8183902/webrev.00/ This is a

Re: JDK 10 RFR of JDK-8183378: Refactor java/lang/System/MacEncoding/MacJNUEncoding.sh to java

2017-07-06 Thread Brent Christian
Thanks a lot for fixing that, Amy. Looks good. -Brent On 07/05/2017 07:16 PM, Felix Yang wrote: Hi Amy, looks fine. Just one comment on sentence below. "LOCALE" looks to be a local variable, though used several times. Switch to usual naming? 50 final String LOCALE = args[2];

Re: [10] RFR: 8160199: Language's script should be reflected in user.script on Mac OS X

2017-06-29 Thread Brent Christian
Thanks, Naoto. The updated changes look good. -Brent On 06/28/2017 03:13 PM, Naoto Sato wrote: Thanks, Brent! Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8160199/webrev.04/ And my comments embedded below: On 6/28/17 2:16 PM, Brent Christian wrote: Hi, Naoto This looks

Re: [10] RFR: 8160199: Language's script should be reflected in user.script on Mac OS X

2017-06-29 Thread Brent Christian
Thanks, Naoto. The updated changes look good. -Brent On 06/28/2017 03:13 PM, Naoto Sato wrote: Thanks, Brent! Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8160199/webrev.04/ And my comments embedded below: On 6/28/17 2:16 PM, Brent Christian wrote: Hi, Naoto This looks

Re: [10] RFR: 8160199: Language's script should be reflected in user.script on Mac OS X

2017-06-28 Thread Brent Christian
Hi, Naoto This looks quite good. I will test a bit with some of the trickier locales. BTW, the script will now also be reflected in Locale.getScript() and Locale.getDisplayScript(). Just a few comments: --- src/java.base/macosx/native/libjava/java_props_macosx.h Update copyright year

Re: [10] RFR: 8160199: Language's script should be reflected in user.script on Mac OS X

2017-06-28 Thread Brent Christian
Hi, Naoto This looks quite good. I will test a bit with some of the trickier locales. BTW, the script will now also be reflected in Locale.getScript() and Locale.getDisplayScript(). Just a few comments: --- src/java.base/macosx/native/libjava/java_props_macosx.h Update copyright year

Re: [10] RFR: 8182487: Add Unsafe.objectFieldOffset(Class, String)

2017-06-20 Thread Brent Christian
On 06/20/2017 06:14 AM, Claes Redestad wrote: it felt reasonable to simplify the code to consistently throw InternalError and remove a number of distracting try-catch blocks. With the try-catches gone, it looks like quite a few static fields could now be initialized inline, and the static

Re: JDK 10 RFR of JDK-8181126: Refactor shell test java/nio/Buffer/LimitDirectMemory.sh to java

2017-05-26 Thread Brent Christian
That all looks fine to me, Amy. (You'll also need a JDK 10 Reviewer). Thanks, -Brent On 5/25/17 7:42 PM, Amy Lu wrote: java/nio/Buffer/LimitDirectMemory.sh Please review this patch to refactor the shell test to java. bug: https://bugs.openjdk.java.net/browse/JDK-8181126 webrev:

Re: Request Review JDK-8180574: tools/launcher/modules/patch/systemmodules/PatchSystemModules.java failed in upgradeHashedModule() and patchHashedModule() intermittently

2017-05-26 Thread Brent Christian
Hi, Mandy Should "@key intermittent" be added, or is this pretty likely to resolve the problem ? Thanks, -Brent On 5/26/17 11:17 AM, Mandy Chung wrote: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8180574/webrev.00/ The test fails intermittently that the hash of new_m1.jar might be the

Re: jdk10 RFR of JDK-8180732: add test to check temp file permission

2017-05-23 Thread Brent Christian
Hi, Hamlin That looks pretty good. A couple comments: * If I'm not mistaken, main/othervm/policy=... is sufficient to enable the default security manager, and specifying "-Djava.security.manager" is unnecessary. * Please add 8180732 to the @bug tag You will also need approval from a JDK

RFR 9 : 8180633 : Remove intermittent key from java/lang/ClassLoader/Assert.java

2017-05-18 Thread Brent Christian
Hi, The test: java/lang/ClassLoader/Assert.java has been passing consistently on our automated test systems for quite a while. The last recorded failure I found happened ~18 months ago. I propose that the 'intermittent' jtreg key be removed from this test: -- diff -r ee3280639210

Re: JDK 10 RFR of 8177809: File.lastModified() is losing milliseconds (always ends in 000)

2017-05-18 Thread Brent Christian
Hi, Brian This looks fine to me. This will get some good bake time in 10, a chance for incompatibilities (if any) to be revealed. -Brent On 5/17/17 1:54 PM, Brian Burkhalter wrote: Please review at your convenience: Issue: https://bugs.openjdk.java.net/browse/JDK-8177809 Patch:

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-18 Thread Brent Christian
Brent Christian wrote: > Mandy Chung wrote: >> >> I think we could take out the automatic module case as named module >> would verify it. One refactor you can consider by separating them >> in several @run actions. ... I think it would make the test easier to re

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-18 Thread Brent Christian
On 5/9/17 4:27 PM, Mandy Chung wrote: Webrev: http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/ Thanks for the clean up. Does each @run action need 4 mins timeout? Can it restore to the default timeout? Yes. Between the additional @runs and no longer testing automated modules, I

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-18 Thread Brent Christian
On 5/12/17 9:11 AM, Mandy Chung wrote: Minor comment: 95 private final List COMMON_ARGS; This is an instance field and you can use lower case as the convention. 238 return !s.equals(""); You can consider using String::isEmpty. Thanks, Mandy. I will make these

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-18 Thread Brent Christian
for Xcomp run, the test would probably now be able to run for over an hour before timing out. I suggest making the test run faster, or seeing if there has been a regressions in Xcomp to make test perform more poorly there. Thanks, -Joe On 4/27/2017 12:08 PM, Brent Christian wrote: Hi, This test times

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-18 Thread Brent Christian
On 5/9/17 4:39 PM, Brent Christian wrote: On 5/9/17 4:27 PM, Mandy Chung wrote: Webrev: http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/ Can it restore to the default timeout? Yes. Between the additional @runs and no longer testing automated modules, I believe that should be fine

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-18 Thread Brent Christian
not affect test execution.) http://cr.openjdk.java.net/~bchristi/8177328/webrev.02/ Thanks, -Brent On 5/10/17 9:42 AM, Brent Christian wrote: On 5/9/17 4:39 PM, Brent Christian wrote: On 5/9/17 4:27 PM, Mandy Chung wrote: Webrev: http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/ Can

RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-18 Thread Brent Christian
Hi, This test times out under our automated testing configurations that include -Xcomp. Please review my change to increase the timeout for this test. It is sufficient for the test configurations in question to pass on two different local machines (Mac & Linux). diff -r 7c04ab31b4d6

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-18 Thread Brent Christian
On 5/1/17 12:50 PM, Mandy Chung wrote: On May 1, 2017, at 12:47 PM, Brent Christian <brent.christ...@oracle.com> wrote: >>> One refactor you can consider by separating them in several @run actions. The timeout will apply to each action but that does not help shorten the test

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-12 Thread Brent Christian
On 5/12/17 9:11 AM, Mandy Chung wrote: Minor comment: 95 private final List COMMON_ARGS; This is an instance field and you can use lower case as the convention. 238 return !s.equals(""); You can consider using String::isEmpty. Thanks, Mandy. I will make these

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-11 Thread Brent Christian
not affect test execution.) http://cr.openjdk.java.net/~bchristi/8177328/webrev.02/ Thanks, -Brent On 5/10/17 9:42 AM, Brent Christian wrote: On 5/9/17 4:39 PM, Brent Christian wrote: On 5/9/17 4:27 PM, Mandy Chung wrote: Webrev: http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/ Can

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-10 Thread Brent Christian
On 5/9/17 4:39 PM, Brent Christian wrote: On 5/9/17 4:27 PM, Mandy Chung wrote: Webrev: http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/ Can it restore to the default timeout? Yes. Between the additional @runs and no longer testing automated modules, I believe that should be fine

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-09 Thread Brent Christian
On 5/9/17 4:27 PM, Mandy Chung wrote: Webrev: http://cr.openjdk.java.net/~bchristi/8177328/webrev.01/ Thanks for the clean up. Does each @run action need 4 mins timeout? Can it restore to the default timeout? Yes. Between the additional @runs and no longer testing automated modules, I

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-09 Thread Brent Christian
Brent Christian wrote: > Mandy Chung wrote: >> >> I think we could take out the automatic module case as named module >> would verify it. One refactor you can consider by separating them >> in several @run actions. ... I think it would make the test easier to re

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-01 Thread Brent Christian
On 5/1/17 12:50 PM, Mandy Chung wrote: On May 1, 2017, at 12:47 PM, Brent Christian <brent.christ...@oracle.com> wrote: >>> One refactor you can consider by separating them in several @run actions. The timeout will apply to each action but that does not help shorten the test

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-01 Thread Brent Christian
to avoid such issues? I'm not sure how that can be assured though. Roger On 5/1/2017 3:47 PM, Brent Christian wrote: On 5/1/17 12:41 PM, Mandy Chung wrote: Looks like this test execs a new VM for 66 times to exhaust the combination of with and without security manager, with a valid or malformed

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-01 Thread Brent Christian
On 5/1/17 12:41 PM, Mandy Chung wrote: Looks like this test execs a new VM for 66 times to exhaust the combination of with and without security manager, with a valid or malformed policy, client & custom loader in unnamed, named, automatic module. Right. I think we could take out the

Re: RFR 9 test-only RFR 8177328 : java/lang/ClassLoader/securityManager/ClassLoaderTest.java times out with -Xcomp

2017-05-01 Thread Brent Christian
for Xcomp run, the test would probably now be able to run for over an hour before timing out. I suggest making the test run faster, or seeing if there has been a regressions in Xcomp to make test perform more poorly there. Thanks, -Joe On 4/27/2017 12:08 PM, Brent Christian wrote: Hi, This test times

Re: JDK 9 RFR(s): 8177789 fix collections framework links to point to java.util package doc

2017-04-17 Thread Brent Christian
On 4/15/17 10:34 AM, Martin Buchholz wrote: @index is new to me; I couldn't find any documentation for it. @index is part of JEP 225, Javadoc Search (right?) https://bugs.openjdk.java.net/browse/JDK-8044243 -Brent

Re: JDK 9 RFR(s): 8177789 fix collections framework links to point to java.util package doc

2017-04-14 Thread Brent Christian
Hi, Stuart The changes look fine. You found and changed all the references I could find. Having the @index I think will be nice. -Brent On 4/14/17 3:36 PM, Stuart Marks wrote: Hi all, Please review this changeset that fixes up links around the JDK that point to the old Collections

Re: [8u] RFR: 8177776: Create an equivalent test case for JDK9's SupplementalJapaneseEraTest

2017-04-06 Thread Brent Christian
Hi, Naoto On 4/5/17 2:14 PM, Naoto Sato wrote: I revised the test case not to rely on shell script. Yay! Hopefully this can also happen sometime for JDK 9+. http://cr.openjdk.java.net/~naoto/816/webrev.01/ Looks fine to me, Naoto. A few comments: * I presume additional @bug values

Re: [8u] RFR: 8177776: Create an equivalent test case for JDK9's SupplementalJapaneseEraTest

2017-04-06 Thread Brent Christian
Hi, Naoto On 4/5/17 2:14 PM, Naoto Sato wrote: I revised the test case not to rely on shell script. Yay! Hopefully this can also happen sometime for JDK 9+. http://cr.openjdk.java.net/~naoto/816/webrev.01/ Looks fine to me, Naoto. A few comments: * I presume additional @bug values

Re: [9] RFR: 8177136: Caller sensitive method System.getLogger should specify what happens if there is no caller on the stack.

2017-03-22 Thread Brent Christian
On 3/22/17 11:29 AM, Daniel Fuchs wrote: I updated the webrev in place. I will now proceed with all the paperwork to get that fix in 9 ;-) http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.04 Looks good. -Brent

Re: [9] RFR: 8177136: Caller sensitive method System.getLogger should specify what happens if there is no caller on the stack.

2017-03-21 Thread Brent Christian
Hi, Daniel I think the new @throws tag gives a good explanation of the situation, with instructions for someone wanting to get a System.Logger from JNI. To nitpick on style, it's maybe on the long side for an @throws - perhaps consider a slightly more concise version: * @throws

Re: Review Request: JDK-8176815: Remove StackFramePermission and use RuntimePermission for stack walking

2017-03-15 Thread Brent Christian
Hi, Mandy I agree that the new permission type is overkill. The changes look good to me. -Brent On 3/15/17 12:42 PM, Mandy Chung wrote: StackWalker::getInstance is currently specified to check for StackFramePermission("retainClassReference“) due to an early review feedback . Given it has

Re: [9] RFR: 8174736: [JCP] [Mac]Cannot launch JCP on Mac os with language set to "Chinese, Simplified" while region is not China

2017-03-06 Thread Brent Christian
Hi, Naoto That fix looks fine. The "Portuguese (Brazil)" fix now happens before the Language ID fix, but that shouldn't matter, as the "pt_BR" ID won't trigger that code anyway (doesn't contain '-'). One very mall nit I noticed: one more space on line 97 will realign it with line 96, which

Re: [9] RFR: 8174779: Locale issues with Mac 10.12

2017-02-14 Thread Brent Christian
Looks good to me, Naoto. -Brent On 02/14/2017 11:05 AM, Naoto Sato wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8174779 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8174779/webrev.01/ The root cause of the problem

Re: [9] RFR: 8174779: Locale issues with Mac 10.12

2017-02-14 Thread Brent Christian
Looks good to me, Naoto. -Brent On 02/14/2017 11:05 AM, Naoto Sato wrote: Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8174779 The proposed fix is located at: http://cr.openjdk.java.net/~naoto/8174779/webrev.01/ The root cause of the problem

Re: RFR: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

2017-02-03 Thread Brent Christian
This looks fine to me, Daniel. Fun test case. :) Thanks, -Brent On 02/03/2017 11:51 AM, Daniel Fuchs wrote: Hi, Please find below a simple fix for: 8173898: StackWalker.walk throws InternalError if called from a constructor invoked through reflection.

Re: RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect (updated)

2017-01-31 Thread Brent Christian
dn't need any changes. Please make sure JPRT -testset hotspot works with this (or check it in that way). Yes, that runs cleanly, and I do plan to push through hs. Thanks for having a look, Coleen. -Brent On 1/26/17 8:07 PM, Brent Christian wrote: Hi, Please review my updated approach for f

Re: RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect (updated)

2017-01-27 Thread Brent Christian
On 01/26/2017 05:07 PM, Brent Christian wrote: I also removed the more simplistic CountLocalSlots.java test, given that the updated LocalsAndOperands.java will now better cover testing -Xcomp. I also noticed that we no longer need the LocalsCrash.java test. Removed. -Brent

Re: RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect (updated)

2017-01-27 Thread Brent Christian
On 01/27/2017 08:26 AM, Mandy Chung wrote: On Jan 26, 2017, at 5:07 PM, Brent Christian <brent.christ...@oracle.com> wrote: Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.03/ I agree with Remi to make the MODE_* constants in LiveStackFrameInfo final and PrimitiveSlot

RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect (updated)

2017-01-26 Thread Brent Christian
Hi, Please review my updated approach for fixing 8156073 ("2-slot LiveStackFrame locals (long and double) are incorrect") in the experimental portion of the StackWalker API, added in JDK 9. Bug: https://bugs.openjdk.java.net/browse/JDK-8156073 Webrev:

Re: RFR 8169389 : Use a bitmap to control StackTraceElement::toString format and save footprint

2016-12-13 Thread Brent Christian
really good now! best regards, -- daniel On 10/12/16 01:16, Brent Christian wrote: On 12/07/2016 04:05 PM, Mandy Chung wrote: I suggest to add two utility methods rather than the has method: boolean dropClassLoaderName() boolean dropModuleVersion() Done. 430if (m != null

RFR 8169389 : Use a bitmap to control StackTraceElement::toString format and save footprint

2016-12-07 Thread Brent Christian
Hi, Please review my fix for 8169389. Bug: https://bugs.openjdk.java.net/browse/JDK-8169389 Webrev: http://cr.openjdk.java.net/~bchristi/8169389/webrev.03/ The addition of classloader names (JDK-6749237) updated the format of StackTraceElement.toString(). A field was added to store the

Re: RFR 8169435 : ClassLoader.isParallelCapable is final and conflicting method may get VerifyError

2016-11-21 Thread Brent Christian
On 11/11/16 2:16 AM, Alan Bateman wrote: >>> Since there is already a @CallerSensitive protected static method called "registerAsParallelCapable" for subclasses to call from their blocks, the query could be called: isRegisteredAsParallelCapable() ? I doubt this name is already taken by any

Re: RFR 8136831 : Undefined null behavior in Class[Loader].getResourceXXXX()

2016-11-17 Thread Brent Christian
On 11/17/16 11:09 AM, Alan Bateman wrote: On 17/11/2016 18:25, Paul Sandoz wrote: >> Looks ok, but has indicated by Alan i would leave Class alone and untested until Jake gets integrated, then follow up with another issue. Actually no follow-up needed because it's updated in jake to specify

Re: RFR 8136831 : Undefined null behavior in Class[Loader].getResourceXXXX()

2016-11-16 Thread Brent Christian
/~bchristi/8136831/webrev.01/ You will need to file a CCC for the change in behaviour of the Enumeration returning getResources. And for the new @throws, I should think. Thanks, Paul -Brent On 16 Nov 2016, at 15:22, Brent Christian <brent.christ...@oracle.com> wrote: Hi, Please review

RFR 8136831 : Undefined null behavior in Class[Loader].getResourceXXXX()

2016-11-16 Thread Brent Christian
Hi, Please review my fix for 8136831 - Undefined null behavior in Class[Loader].getResource(). Bug: https://bugs.openjdk.java.net/browse/JDK-8136831 Webrev: http://cr.openjdk.java.net/~bchristi/8136831/webrev.00/ Class and ClassLoader have the following public methods for locating

RFR 8169435 : ClassLoader.isParallelCapable is final and conflicting method may get VerifyError

2016-11-09 Thread Brent Christian
Hi, It seems that the method name used in 8165793[1], "isParallelCapable", was a little *too* intuitive. An existing use of that method name has been uncovered (via NetBeans, in Eclipse Equinox). Because the newly added method was marked final, the pre-existing method results in a

Re: Request Review: JDK-6479237 (cl) Add support for classloader names

2016-10-28 Thread Brent Christian
On 10/28/16 1:44 PM, Mandy Chung wrote: On Oct 28, 2016, at 11:11 AM, Brent Christian <brent.christ...@oracle.com> wrote: Should something be done for STEs returned from StackFrameInfo.toStackTraceElement() ? Good catch - I missed it. I added package-private static m

Re: Request Review: JDK-6479237 (cl) Add support for classloader names

2016-10-27 Thread Brent Christian
Hi, Mandy It looks pretty good to me. Just a couple small things: * StackTraceElement.java 379 ClassLoader loader = cls.getClassLoader0(); It looks as if 'loader' isn't used...? * Throwable.java 832 // VM to fill in StackTraceElement 833

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

2016-10-12 Thread Brent Christian
On 10/11/16 8:25 AM, Alan Bateman wrote: > One thing that would be good is to beef up the test to cover more scenarios, esp. loader L1 extends loader L2 where you've got 4 combination of capable/non-capable to test. I updated the test case to provide better coverage:

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

2016-10-10 Thread Brent Christian
On 10/10/16 3:51 PM, Mandy Chung wrote: Do you mind fixing @return in the registerAsParallelCapable method with {@code true} amd {@code false} since you are in this method. No need for a new webrev. Sure, no problem. Thanks, -Brent

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

2016-10-10 Thread Brent Christian
On 10/10/16 2:30 PM, Mandy Chung wrote: The patch looks fine. It would be good to add @see #registerAsParallelCapable in this new method. Also the first “parallel capable” occurrance in the class spec and the registerAsParallelCapable method spec to @linkplain #isParallelCapable. Thanks for

Re: Using non-parallel custom class loaders for Layer configurations

2016-10-10 Thread Brent Christian
On 9/9/16 12:05 PM, Mandy Chung wrote: I file a RFE and Brent Christian has agreed to work on it and will go through JDK 9 FC approval request. https://bugs.openjdk.java.net/browse/JDK-8165793 FYI, the review thread is here: http://mail.openjdk.java.net/pipermail/core-libs-dev/2016

RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

2016-10-10 Thread Brent Christian
Hi, Please review my fix for 8165793. This follows the discussion here: http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-September/009328.html The proposal is to add a new public method on ClassLoader: /** * Returns {@code true} if this class loader is * {@linkplain

Re: RFR 8151486 : Class.forName causes memory leak

2016-10-07 Thread Brent Christian
On 10/5/16 4:43 PM, David Holmes wrote: Okay but this will still affect the lifecycle of the PDs because without the strong reference in L, those weak references in the VM will quickly be cleared. There's also a strong reference held by the Class object itself (on the VM side [1]). Thanks

Re: RFR 8151486 : Class.forName causes memory leak

2016-10-05 Thread Brent Christian
Yes! Good catch, thanks. -Brent On 10/5/16 2:08 PM, Naoto Sato wrote: Typo in ClassForNameLeak.java? At line 103, "change" -> "chance"? Naoto On 10/5/16 12:38 PM, Brent Christian wrote: Hi, Please review my fix for 8151486, "Class.forName caus

RFR 8151486 : Class.forName causes memory leak

2016-10-05 Thread Brent Christian
Hi, Please review my fix for 8151486, "Class.forName causes memory leak". Bug: https://bugs.openjdk.java.net/browse/JDK-8151486 Webrev: http://cr.openjdk.java.net/~bchristi/8151486/webrev.00/ The test case reproduces the bug as such: With a SecurityManager enabled, a class ("ClassForName") is

Re: [9] RFR of JDK-8085192: java/rmi/activation/Activatable tests fail intermittently due to "Port already in use"

2016-09-27 Thread Brent Christian
Thanks for making some improvements to these intermittent RMI tests. I agree with Roger that I don't think we want to add the id to the @bug of every test. Also, it looks like there's an indentation change in JavaVM.java: 53 public static final long POLLTIME_MS = 100L; (I believe

Re: RFR 8165372 : StackWalker performance regression following JDK-8147039

2016-09-19 Thread Brent Christian
Thanks for the good suggestions. Webrev updated in place: http://cr.openjdk.java.net/~bchristi/8165372/webrev.00/ -Brent

RFR 8165372 : StackWalker performance regression following JDK-8147039

2016-09-19 Thread Brent Christian
Hi, Please review my fix for 8165372 : "StackWalker performance regression following JDK-8147039" Bug: https://bugs.openjdk.java.net/browse/JDK-8165372 Webrev: http://cr.openjdk.java.net/~bchristi/8165372/webrev.00/ 8147039 reimplemented stack walking using javaVFrames in place of

Re: Review Request: JDK-8157464: StackWalker.getCallerClass() is not

2016-09-14 Thread Brent Christian
v.03/ Mandy On Sep 13, 2016, at 4:09 PM, Brent Christian <brent.christ...@oracle.com> wrote: Hi, Mandy Is a new JVM_STACKWALK_GET_CALLER_CLASS bit needed in jvm.h? AFAICT, JVM_STACKWALK_FILL_CLASS_REFS_ONLY is already only set for the getCallerClass() case.

Re: Review Request: JDK-8157464: StackWalker.getCallerClass() is not

2016-09-13 Thread Brent Christian
Hi, Mandy Is a new JVM_STACKWALK_GET_CALLER_CLASS bit needed in jvm.h? AFAICT, JVM_STACKWALK_FILL_CLASS_REFS_ONLY is already only set for the getCallerClass() case. Could the new get_caller_class() instead check if JVM_STACKWALK_FILL_CLASS_REFS_ONLY is set? (Yes, this would be a third

Re: RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect

2016-08-19 Thread Brent Christian
appened to have high-order bits set, it would be interpreted as a 64-bit long value, and the preceding slot could have its (legitimate) value overwritten: slot 0: slot for real local int slot 1: unused slot containing garbage w/ upper bits set slot 2: 2nd slot of a long, containing the lo

RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect

2016-08-17 Thread Brent Christian
Hi, Please review this StackWalker fix in hotspot for 8156073, "2-slot LiveStackFrame locals (long and double) are incorrect" Bug: https://bugs.openjdk.java.net/browse/JDK-8156073 Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.01/ The experimental LiveStackFrame[1] interface for

Re: RFR 9 7180225 : SecurityExceptions not defined in some class loader methods

2016-08-17 Thread Brent Christian
On 8/16/16 8:33 PM, Mandy Chung wrote: On Aug 16, 2016, at 1:54 PM, Brent Christian <brent.christ...@oracle.com> wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-7180225 Webrev: http://cr.openjdk.java.net/~bchristi/7180225/webrev.01/webrev/ Specdiff: http://cr.openjdk.ja

RFR 9 7180225 : SecurityExceptions not defined in some class loader methods

2016-08-16 Thread Brent Christian
Hi, Please review this change which does some cleanup around documenting conditions for throwing SecurityExceptions. It adds a missing @throws tag to Class.forName(String, boolean, ClassLoader), and consolidates specifics in the @throws tags of ClassLoader & Thread. Bug:

Re: java.io.Writer uses CharSequence.toString()

2016-07-29 Thread Brent Christian
Hi, This idea has been brought up before [1]. I concur with Pavel's assessment. I would add that now that latin-1 Strings are stored in a more compact form in JDK 9 ("Compact Strings" [2]), the performance profile of string data is further complicated. Thanks, -Brent 1.

RFR 9 8161039 : System.getProperty("os.version") returns incorrect version number on Mac

2016-07-21 Thread Brent Christian
Hi, The fix for 7131356 introduced a minor behavior change in the value of the "os.version" system property on MacOS. On systems running an OS with a patch version of 0, we've started including this in the value of os.version (e.g. "10.11.0"), whereas before the trailing ".0" was omitted.

Re: RFR 9 : 8161718 : Copyright/License updates to corba, jdk

2016-07-19 Thread Brent Christian
Thank you, Brian and Naoto! -Brent On 7/19/16 4:46 PM, Naoto Sato wrote: +1 Naoto On 7/19/16 4:45 PM, Brian Burkhalter wrote: Hi Brent, Looks OK to me. Brian On Jul 19, 2016, at 4:26 PM, Brent Christian <brent.christ...@oracle.com> wrote: Hi, Please review some spacing and punct

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-30 Thread Brent Christian
On 6/30/16 9:53 AM, Brent Christian wrote: When the minimum Mac build platform is SDK 10.10, we'll be able to call operatingSystemVersion directly without using msgSend. We can also consider removing this then. FYI: https://bugs.openjdk.java.net/browse/JDK-8160676 -Brent

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-30 Thread Brent Christian
On 6/30/16 9:53 AM, Brent Christian wrote: When the minimum Mac build platform is SDK 10.10, we'll be able to call operatingSystemVersion directly without using msgSend. We can also consider removing this then. FYI: https://bugs.openjdk.java.net/browse/JDK-8160676 -Brent

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-30 Thread Brent Christian
On 6/29/16 4:47 PM, Mandy Chung wrote: On Jun 29, 2016, at 12:36 PM, Brent Christian <brent.christ...@oracle.com> wrote: The code to restore behavior on older Mac systems is only a few lines, so that seems like a good way to get testing going again. I think we should move away from t

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-30 Thread Brent Christian
On 6/29/16 4:47 PM, Mandy Chung wrote: On Jun 29, 2016, at 12:36 PM, Brent Christian <brent.christ...@oracle.com> wrote: The code to restore behavior on older Mac systems is only a few lines, so that seems like a good way to get testing going again. I think we should move away from t

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-30 Thread Brent Christian
Thanks, Brian On 6/29/16 4:35 PM, Brian Burkhalter wrote: Approved. Brian On Jun 29, 2016, at 4:33 PM, Brent Christian <brent.christ...@oracle.com <mailto:brent.christ...@oracle.com>> wrote: Thank you, Dave and Gerard. I believe I still need to hear from a JDK9 Reviewer.

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-30 Thread Brent Christian
Thanks, Brian On 6/29/16 4:35 PM, Brian Burkhalter wrote: Approved. Brian On Jun 29, 2016, at 4:33 PM, Brent Christian <brent.christ...@oracle.com <mailto:brent.christ...@oracle.com>> wrote: Thank you, Dave and Gerard. I believe I still need to hear from a JDK9 Reviewer.

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-30 Thread Brent Christian
Will do - thanks, Roger. -Brent On 6/30/16 6:46 AM, Roger Riggs wrote: +1 Can you wrap a couple of very long lines to make the diff easier to read. - 187 - 202 Thanks, Roger On 6/29/2016 7:47 PM, Mandy Chung wrote: On Jun 29, 2016, at 12:36 PM, Brent Christian <brent.christ...@oracle.

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-29 Thread Brent Christian
fixing the original issue and for putting in this follow-up fix! Looks good! (for full disclosure I proposed the workaround) cheers On Jun 29, 2016, at 2:36 PM, Brent Christian <brent.christ...@oracle.com> wrote: Hi, Please review the following change for JDK 9: Bug: https://bugs.openjdk.jav

Re: RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-29 Thread Brent Christian
fixing the original issue and for putting in this follow-up fix! Looks good! (for full disclosure I proposed the workaround) cheers On Jun 29, 2016, at 2:36 PM, Brent Christian <brent.christ...@oracle.com> wrote: Hi, Please review the following change for JDK 9: Bug: https://bugs.openjdk.jav

RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-29 Thread Brent Christian
Hi, Please review the following change for JDK 9: Bug: https://bugs.openjdk.java.net/browse/JDK-8160370 Webrev: http://cr.openjdk.java.net/~bchristi/8160370/webrev.00/ The fix for 7131356 fills in the "os.version" system property on Mac using [NSProcessInfo operatingSystemVersion], available

RFR 9 : 8160370 : System.getProperty("os.version") returns "Unknown" on Mac

2016-06-29 Thread Brent Christian
Hi, Please review the following change for JDK 9: Bug: https://bugs.openjdk.java.net/browse/JDK-8160370 Webrev: http://cr.openjdk.java.net/~bchristi/8160370/webrev.00/ The fix for 7131356 fills in the "os.version" system property on Mac using [NSProcessInfo operatingSystemVersion], available

Re: [9] RFR: 8022291: Mac OS: Unexpected JavaLaunchHelper message displaying

2016-06-27 Thread Brent Christian
I'm not familiar with this area. The changes looks reasonable enough to me. -Brent On 06/27/2016 01:19 PM, Kumar Srinivasan wrote: I am not an expert in this area, if Sergey is ok with it. I am fine with it. Brent ? Kumar Thanks, Sergey. Can someone from the core-libs launcher group

Re: [9] RFR: 8022291: Mac OS: Unexpected JavaLaunchHelper message displaying

2016-06-27 Thread Brent Christian
I'm not familiar with this area. The changes looks reasonable enough to me. -Brent On 06/27/2016 01:19 PM, Kumar Srinivasan wrote: I am not an expert in this area, if Sergey is ok with it. I am fine with it. Brent ? Kumar Thanks, Sergey. Can someone from the core-libs launcher group

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

2016-06-23 Thread Brent Christian
h case when we call CFLocaleGetIdentifier(), which I guess was the source of my confusion. I've removed that comment line, and reworked the comment a little bit. http://cr.openjdk.java.net/~bchristi/7131356/webrev.05/ Thanks, -Brent On 6/23/16 8:24 AM, Naoto Sato wrote: On 6/22/16 9:48 PM, Brent Chris

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

2016-06-23 Thread Brent Christian
h case when we call CFLocaleGetIdentifier(), which I guess was the source of my confusion. I've removed that comment line, and reworked the comment a little bit. http://cr.openjdk.java.net/~bchristi/7131356/webrev.05/ Thanks, -Brent On 6/23/16 8:24 AM, Naoto Sato wrote: On 6/22/16 9:48 PM, Brent Chris

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

2016-06-23 Thread Brent Christian
On 6/23/16 8:24 AM, Naoto Sato wrote: On 6/22/16 9:48 PM, Brent Christian wrote: On 6/22/16 3:58 PM, Naoto Sato wrote: 2. I think mapping language/script combination to a typical locale is ok to keep the compatibility (e.g., "sr-Latn" to "sr_CS", done by the JRS, right?

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

2016-06-22 Thread Brent Christian
In the meantime, I would like to have "user.script" set to "Latn" in such a case. Is this something that I could file a follow-on issue for? The existing code doesn't set "user.script" in these cases, either, and it looks like that would take some digging into java_

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

2016-06-22 Thread Brent Christian
In the meantime, I would like to have "user.script" set to "Latn" in such a case. Is this something that I could file a follow-on issue for? The existing code doesn't set "user.script" in these cases, either, and it looks like that would take some digging into java_

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

2016-06-22 Thread Brent Christian
In the meantime, I would like to have "user.script" set to "Latn" in such a case. Is this something that I could file a follow-on issue for? The existing code doesn't set "user.script" in these cases, either, and it looks like that would take some digging into java_

Re: RFR 9 7131356 : (props) "No Java runtime present, requesting install" when creating VM from JNI [macosx]

2016-06-22 Thread Brent Christian
On 22/06/16 15:10, Alex Strange wrote: On Jun 20, 2016, at 3:44 PM, Brent Christian <brent.christ...@oracle.com> wrote: I'd prefer something that can build on SDK 10.9 and 10.10, etc. There might be a way to #ifdef it out (not sure), but it's not worth it. I came to the same conc

<    2   3   4   5   6   7   8   9   >