Re: RFR: 8194154: JDK crashes parsing path string contains '//' on linux

[yumin qi]

David, Thanks!

On Wed, Feb 7, 2018 at 7:32 PM, David Holmes 
wrote:

> Moving to core-libs-dev. Code reviews don't take place on jdk-dev but on
> the component area mailing lists.
>
> Thanks,
> David
>
>
> On 8/02/2018 6:10 AM, yumin qi wrote:
>
>> Hi,
>>
>>Please review the fix (extra small) for:
>>bug 8194154: https://bugs.openjdk.java.net/browse/JDK-8194154
>>webrev: http://cr.openjdk.java.net/~minqi/8194154/
>>
>>Summary: canonicalize will check and fold double (or more) slashes, but
>> in function collapsible(char*) and splitNames(char*, char**), it failed to
>> process strings like "//". This results in the former does not give a
>> correct number of substrings and the later does not give a correct array
>> of
>> substrings. The fix add a check if the character is '/' after a '/'.
>>
>>  The test case added will crash without the fix.
>>
>>   Thanks
>>   Yumin
>>
>>

Re: RFR JDK-8164278: java.util.Base64.EncOutputStream/DecInputStream is slower than corresponding version in javax.mail package

[Xueming Shen]

Hi Roger,

Given the concern of the possible incompatible behavior change of over 
reading bytes
from the underlying stream. I decided to give up last proposed changes 
for DecInputStream
for now. With some "minor" cleanup and tuning I still have about 10%+ 
improvement with
various input size sampling. Let's address the decoder stream in 
separate rfe later, if desired.


The updated webrev and the jmh results are at

http://cr.openjdk.java.net/~sherman/8164278/webrev
http://cr.openjdk.java.net/~sherman/8164278/base64.bm

Last version of webrev and corresponding jmh result can be found at
http://cr.openjdk.java.net/~sherman/8164278/webrev.02
http://cr.openjdk.java.net/~sherman/8164278/base64.bm.old

Thanks,
Sherman

On 2/5/18, 6:00 PM, Xueming Shen wrote:

Hi,

Please help review the change for  JDK-8164278.

issue: https://bugs.openjdk.java.net/browse/JDK-8164278
webrev: http://cr.openjdk.java.net/~sherman/8164278/webrev

jmh.src: http://cr.openjdk.java.net/~sherman/8164278/Base64BM.java
jmh.result: http://cr.openjdk.java.net/~sherman/8164278/base64.bm

Base64.Decoder.decode0:
Adopted the "similar" optimization approach we took in 
Base64.Encoder.encode0()
to add a "fast path" to decode a block of 4-byte units together 
(current implementation
decodes one single byte per while/loop. The jmh benchmark result 
indicates a big speed
boost  (those decodeArray/Mime/Url results, from 30% to 2 times 
faster, depends on

input size).

Base64.Encoder.encode0()
It appears encode0() was fully optimized in 1.8. Can't get it 
faster :-) Tried to use
Unsafe.getLong/putLong instead of byte by byte access. But it 
appears the 8-byte
"vectorization" does not bring us enough speed up, the performance 
is the same as the

current one. See encode00() at
http://cr.openjdk.java.net/~sherman/8164278/webrev.00

Base64.Encoder.wrap(OutputStream)/EncOutputStream.write():
If my memory serves me right, the current implementation was under 
the assumption that
the underlying output stream probably is buffered (if invoker 
cares). It would be a redundant
if EncOutputStream buffers bytes again. It appears this is a wrong 
assumption. It is much
slower to write 4 bytes separately, compared to bundle them 
together in a byte[4] and write
into underlying, even the underlying output stream is a 
ByteArrayOutputStream.
Again, the proposed change is to add a fast path loop, as we do in 
encode0(), to decode/
write a block of 3-byte->4-byte unites. It appears this fast loop 
can help the compiler to

optimize away some boundary checks, therefor is much faster.
The jmh result Base64BM.encodeOS suggests the new implementation 
is almost 4 times faster

and is almost the same as java.mail's stream encoder.

Base64.Decoder.wrap(InputStream)/DecInputStream.read():
Same as the approach we take for decode0(), to add a fast path 
decode block of 4-byte unites

together.
The jmh result Base64BM.decodeOS (the name probably should be 
decodeIS, for InputStream,
but anyway...) shows the proposed one is 4 times faster than the 
existing impl and double

the  java.mail (Base64BM.decodeOS_javamail) implementation.

However, there is a side-effect of adding a buffering mechanism 
into DecInputStream. The
current implementation read bytes from the underlying stream one 
by one, it never reads
more bytes than it needs, which means it should/is supposed to 
just stop at the last byte

that it needs to decode, when there is "=" present in the stream.
With buffering, it's possible more bytes (after "=", which 
indicates "end of base64 stream") might
be read/consumed in and buffered.  A concern? if this is indeed a 
concern, the only alternative
might be to add a separate method to support this 
"faster-buffered-decoder"?


Thanks,
Sherman

Re: RFR: 8194154: JDK crashes parsing path string contains '//' on linux

[David Holmes]

Moving to core-libs-dev. Code reviews don't take place on jdk-dev but on 
the component area mailing lists.


Thanks,
David

On 8/02/2018 6:10 AM, yumin qi wrote:

Hi,

   Please review the fix (extra small) for:
   bug 8194154: https://bugs.openjdk.java.net/browse/JDK-8194154
   webrev: http://cr.openjdk.java.net/~minqi/8194154/

   Summary: canonicalize will check and fold double (or more) slashes, but
in function collapsible(char*) and splitNames(char*, char**), it failed to
process strings like "//". This results in the former does not give a
correct number of substrings and the later does not give a correct array of
substrings. The fix add a check if the character is '/' after a '/'.

 The test case added will crash without the fix.

  Thanks
  Yumin

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

[David Holmes]

Hi Robin,

Adding in hotspot-runtime-dev as all the hotspot changes belong to runtime.

I had an initial look through this.

To be honest I don't like it. We seem to have to record little bits of 
information all over the place just so they can be reported by the 
shutdown event. It seems untidy. :(


Can you rename _starting_thread to _main_thread please. The use of 
"starting" in thread.hpp/cpp is really a naming anomaly. The main thread 
is the thread that loads the JVM. And that would be consistent with 
set_exception_in_main_thread.


Though why do we care if the main thread exited with an unhandled 
exception? And if we do care, why do we only care when the shutdown 
reason is ""No remaining non-daemon Java threads"?


I don't like the need to add os::get_abort_exit_code. Do we really need 
it? What useful information does it convey?


It is unfortunate that you need to add beforeHalt to deal with the event 
mechanism itself being turned off (?) by a shutdown hook. That would 
seem to potentially lose a lot of event information given hooks can run 
in arbitrary order and execute arbitrary Java code. And essentially you 
end up recording the initial reason shutdown started, though potentially 
it could end up terminating for a different reason.


Let's see what others think ...

Thanks,
David

On 8/02/2018 1:18 AM, Robin Westberg wrote:

Hi all,

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


Issue: https://bugs.openjdk.java.net/browse/JDK-8041626
Webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00/
Testing: hs-tier1,hs-tier2,jdk-tier1,jdk-tier2

Best regards,
Robin

Re: JDK 11 RFR of JDK-8196995: java.lang.Character should not state UTF-16 encoding is used for strings

[Martin Buchholz]

I'm not sure about this.  The LATIN-1 optimization is an implementation
detail.  The important thing is the API, and it remains char-oriented
(although we've added codepoint APIs over the years).  Strictly speaking,
you can store any 16-bit integer sequences in char[], String, or
StringBuffer - no encoding is enforced.  What is really UTF-16 is the way
that such sequences are expected to be interpret by *other* APIs, e.g.
System.out.println (and methods like codePointAt)

On Wed, Feb 7, 2018 at 2:12 PM, joe darcy  wrote:

> Hello,
>
> Text in java.lang.Character states a UTF-16 character encoding is used for
> java.lang.String. While was true for many years, it is not necessarily true
> and not true in practice as of JDK 9 due to the improvements from JEP 254:
> Compact Strings.
>
> The statement about the encoding should be corrected.
>
> Please review the patch below which does this. (I've formatted the patch
> so that the change is text is made clear; I'll re-flow the paragraph before
> pushing.
>
> Thanks,
>
> -Joe
>
> diff -r 0b1138ce244f src/java.base/share/classes/java/lang/Character.java
> --- a/src/java.base/share/classes/java/lang/Character.javaTue Feb 06
> 10:17:31 2018 -0800
> +++ b/src/java.base/share/classes/java/lang/Character.javaWed Feb 07
> 11:38:06 2018 -0800
> @@ -75,7 +75,7 @@
>   * Characters whose code points are greater
>   * than U+ are called supplementary characters.  The Java
>   * platform uses the UTF-16 representation in {@code char} arrays and
> - * in the {@code String} and {@code StringBuffer} classes. In
> + * may use it elsewhere. In
>   * this representation, supplementary characters are represented as a pair
>   * of {@code char} values, the first from the high-surrogates
>   * range, (\uD800-\uDBFF), the second from the
>
>

Re: RFR: 8196869: Optimize Locale creation

[Paul Sandoz]

This is looking much better, definitely easier to understand.

Paul.

> On Feb 7, 2018, at 4:38 PM, Claes Redestad  wrote:
> 
> Hi,
> 
> On 2018-02-07 15:23, Claes Redestad wrote:
>> - use map.replace to safely update the entry when putIfAbsent returns an 
>> object
>>   but it points to a cleared value, so that (benign) races to create new 
>> Locale objects
>>   will canonicalize
> 
> turns out the implementation attempted here with map.replace was problematic,
> causing certain tests to fail. Seems there are subtle issues here around
> establishing a stable equality relationship which may or may not be easy to
> resolve, so I reverted back these particular changes to LocaleObjectCache 
> from this
> RFE.
> 
> I also did some cleanup in BaseLocale based on offline feedback from Paul:
> 
> http://cr.openjdk.java.net/~redestad/8196869/jdk.03/
> 
> Thanks!
> 
> /Claes

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

[Aleks Efimov]

Hi Jason, Roger,

Thanks for your comments.
The new webrev with suggested/recommended edits can be viewed at this 
location:

http://cr.openjdk.java.net/~aefimov/6857903/dev/03/

Thanks,
Aleksei

PS: Also configured my IDE to use explicit imports =)

On 02/06/2018 03:31 PM, Roger Riggs wrote:

Hi Aleksei,

SAXException.java x 2, SAXExceptionInitCause:
  Please use explicit imports (not java.io.*).

SAXEXception.java:145: I would have expected:
if (cause instanceof Exception) {...}

SAXExceptionInitCause.java:
  Great to see all the test cases.

  Generally, we recommend using try-with-resources but in this case it 
does not add much because

  exceptions can't occur when using BAOS.

Thanks, Roger

On 2/5/2018 9:46 PM, Aleks Efimov wrote:

Thank you for your comments, Jason!
New webrev can be found at this location:
http://cr.openjdk.java.net/~aefimov/6857903/dev/02/

The list of changes:
- this.initCause() -> super.initCause()
- 'readObject' has been modified to convert IllegalStateException to 
InvalidClassException. The test case that triggers 
IllegalStateException in SAXException::readObject has been added to 
'testReadObjectIllegalStateException' test method.


Best Regards,
Aleksei

On 02/05/2018 05:09 PM, Jason Mehrens wrote:

Aleksei,

Looks good.  We should avoid this.initCause in readObject and prefer 
super.initCause so subclasses can't alter the deserialization behavior.
While I can't think of a case off the top of my head, I would prefer 
that we trap and convert IllegalStateException into a 
InvalidClassException in readObject.
That keeps the readObject contract intact in case something 
malicious is going on.  That is just my preference though.


Jason

From: Aleks Efimov 
Sent: Monday, February 5, 2018 10:51 AM
To: Jason Mehrens; 'Roger Riggs'
Cc: core-libs-dev
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does 
not correctly set Exception


Hi Roger, Jason,

I've tried to avoid doing the same stuff as I did for XPathException to
minimize the changes to the SAXException class. But I was naive to 
think so:

Tried to keep the exception field in place to avoid modifications
required to keep serial form intact (similar as it was done in
JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' 
method

(spotted by Roger, thank you!) that can make cause/exception values
inconsistent. To avoid the API modification and for the sake of clarity
I proceeded with removal of the 'exception' field. The new version of
the fix:
- Removes 'exception' field
- Maintains the serial form of the SAXException class
- Handles the difference in SAXException:exception and Throwable:cause
types 'SAXException::getException', i.e. SAXException:exception is
Exception typed and Throwable:cause is Throwable typed.
- Avoids any changes to API and serial form of the class, but
maintaining it similar to JDK-8009581)
- There is a copy of SAXException class in java.base module that is
required for j.u.Properties::loadFromXML, it has been update with the
same changes.

New regression test has been added to test:
- Serial compatibility with legacy JDKs (similar to JDK-8009581)
- usages of SAXException::initCause/getException

Webrev with the fix and new regression:
http://cr.openjdk.java.net/~aefimov/6857903/dev/01/

Testing shows no JCK/regression tests failures with the proposed fix.

Best Regards,
Aleksei


On 12/21/2017 07:00 PM, Jason Mehrens wrote:

Aleksei,

You have to override all of the constructors to always disable 
initCause.  Actually a similar issue was covered in:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html 

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html 



Pretty much everything was hashed out in those threads.

Here is the final webrev for that:

http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html 



That should be a good cookbook for changes and tests required for 
this issue.  Note that it gets rid of the Exception field and 
maintains serial compatibility.
Looking back on that change, I would have changed it so the 
InvalidClassException added the double cause as suppressed exceptions.


Jason


From: core-libs-dev  on 
behalf of Aleks Efimov 

Sent: Thursday, December 21, 2017 11:27 AM
To: core-libs-dev
Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does 
not correctly set Exception


Hello,

Please, help to review the fix for jaxp bug that fixes SAXException to
correctly set exception cause with 'setCause' method:
http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
I've tried to keep the fix miminal with respect to serial form of this
API class, i.e. kept private 'exception' field.

The new test case was added to IssueTracker56Test unit test. Testing
showed no related JCK/JTREG failures.

With Best Regards,
Aleksei

Re: RFR: 8196869: Optimize Locale creation

[Claes Redestad]

Hi,

On 2018-02-07 15:23, Claes Redestad wrote:
- use map.replace to safely update the entry when putIfAbsent returns 
an object
  but it points to a cleared value, so that (benign) races to create 
new Locale objects

  will canonicalize


turns out the implementation attempted here with map.replace was 
problematic,

causing certain tests to fail. Seems there are subtle issues here around
establishing a stable equality relationship which may or may not be easy to
resolve, so I reverted back these particular changes to 
LocaleObjectCache from this

RFE.

I also did some cleanup in BaseLocale based on offline feedback from Paul:

http://cr.openjdk.java.net/~redestad/8196869/jdk.03/

Thanks!

/Claes

Re: RFR 8190378: Java EE and CORBA modules removal

[Lance Andersen]

Hi Mandy
> On Feb 7, 2018, at 5:00 PM, mandy chung  wrote:
> 
> Hi Lance,
> 
> Great to see this JEP moving along.  I reviewed all changes except
> test/langtools/tools/javac tests.
> 
> Looks fine overall. 

Thank you for the review.
>  Minor comments:
> 
> src/java.base/share/lib/security/default.policy
> - no change in this file.

Weird,  not sure what happened, but it is now fixed.
> 
> test/jdk/tools/jmod/hashes/HashesTest.java
> test/jdk/tools/launcher/modules/addexports/AddExportsTest.java
> - I think we should replace this test case with a different upgradeable 
> module.
>   It's okay to remove this case in this patch and follow up separately with
>   a new JBS issue.
I can do that.
> 
> test/langtools/tools/jdeps/modules/patches/java/sql/NonNull.java
> - copyright start year needs update.
Updated.  

Best
Lance
> 
> Mandy
> 
> On 2/7/18 8:57 AM, Lance Andersen wrote:
>> Hi all,
>> 
>> I think we are at a point where we are ready to start reviewing  the changes 
>> to remove the Java EE and CORBA modules as JEP 320, JDK-8189188,  has been  
>> targeted to JDK 11.
>> The CSR for removing the modules has been approved: 
>> https://bugs.openjdk.java.net/browse/JDK-8193757 
>>  
>>  
>> 
>> 
>>  The open webrev can be found at:  
>> http://cr.openjdk.java.net/~lancea/8190378/open_changes/ 
>>  
>>  
>> 
>> 
>> To make the open review easier, I have broken the changes into 5 webrevs:
>> build changes are: 
>> http://cr.openjdk.java.net/~lancea/8190378/open_changes/build_webrev/ 
>>  
>>  
>> 
>> miscellaneous changes are at:  
>> http://cr.openjdk.java.net/~lancea/8190378/open_changes/misc_webrev/ 
>>  
>>  
>> 
>> module changes are at: 
>> http://cr.openjdk.java.net/~lancea/8190378/open_changes/modules_webrev/ 
>>  
>>  
>> 
>> rmic changes are at:  
>> http://cr.openjdk.java.net/~lancea/8190378/open_changes/rmic_webrev/ 
>>  
>>  
>> 
>> test changes are at: 
>> http://cr.openjdk.java.net/~lancea/8190378/open_changes/tests_webrev/ 
>>  
>>  
>> 
>> 
>> As part of  the removal, the following issues have also been logged:
>> Removal of the Java EE and CORBA tools from the documentation: 
>> https://bugs.openjdk.java.net/browse/JDK-8193906 
>>  
>>  
>> 
>> Updating the RMIC man pages for the removal of the -iiop and -idl options: 
>> https://bugs.openjdk.java.net/browse/JDK-8196510 
>>  
>>  
>> 
>> Hotspot tests may require further updating or just removed: 
>> https://bugs.openjdk.java.net/browse/JDK-8194310 
>>  
>>  
>> 
>> jdeprescan will need updates due to the removal of the Java EE and CORBA 
>> modules: https://bugs.openjdk.java.net/browse/JDK-8194308 
>>  
>>  
>> 
>> 
>> 
>> 
>> Best,
>> Lance
>> 
>>   
>> 
>>   
>>  
>>  
>> 

JDK 11 RFR of JDK-8196995: java.lang.Character should not state UTF-16 encoding is used for strings

[joe darcy]

Hello,

Text in java.lang.Character states a UTF-16 character encoding is used 
for java.lang.String. While was true for many years, it is not 
necessarily true and not true in practice as of JDK 9 due to the 
improvements from JEP 254: Compact Strings.


The statement about the encoding should be corrected.

Please review the patch below which does this. (I've formatted the patch 
so that the change is text is made clear; I'll re-flow the paragraph 
before pushing.


Thanks,

-Joe

diff -r 0b1138ce244f src/java.base/share/classes/java/lang/Character.java
--- a/src/java.base/share/classes/java/lang/Character.java    Tue Feb 06 
10:17:31 2018 -0800
+++ b/src/java.base/share/classes/java/lang/Character.java    Wed Feb 07 
11:38:06 2018 -0800

@@ -75,7 +75,7 @@
  * Characters whose code points are greater
  * than U+ are called supplementary characters.  The Java
  * platform uses the UTF-16 representation in {@code char} arrays and
- * in the {@code String} and {@code StringBuffer} classes. In
+ * may use it elsewhere. In
  * this representation, supplementary characters are represented as a pair
  * of {@code char} values, the first from the high-surrogates
  * range, (\uD800-\uDBFF), the second from the

Re: RFR 8190378: Java EE and CORBA modules removal

[mandy chung]

Hi Lance,

Great to see this JEP moving along.  I reviewed all changes except
test/langtools/tools/javac tests.

Looks fine overall.  Minor comments:

src/java.base/share/lib/security/default.policy
- no change in this file.

test/jdk/tools/jmod/hashes/HashesTest.java
test/jdk/tools/launcher/modules/addexports/AddExportsTest.java
- I think we should replace this test case with a different upgradeable module.
  It's okay to remove this case in this patch and follow up separately with
  a new JBS issue.

test/langtools/tools/jdeps/modules/patches/java/sql/NonNull.java
- copyright start year needs update.

Mandy


On 2/7/18 8:57 AM, Lance Andersen wrote:

Hi all,

I think we are at a point where we are ready to start reviewing  the changes to 
remove the Java EE and CORBA modules as JEP 320, JDK-8189188,  has been  
targeted to JDK 11.
The CSR for removing the modules has been approved: 
https://bugs.openjdk.java.net/browse/JDK-8193757 


  The open webrev can be found at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/ 


To make the open review easier, I have broken the changes into 5 webrevs:
build changes are: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/build_webrev/ 

miscellaneous changes are at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/misc_webrev/ 

module changes are at: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/modules_webrev/ 

rmic changes are at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/rmic_webrev/ 

test changes are at: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/tests_webrev/ 


As part of  the removal, the following issues have also been logged:
Removal of the Java EE and CORBA tools from the documentation: 
https://bugs.openjdk.java.net/browse/JDK-8193906 

Updating the RMIC man pages for the removal of the -iiop and -idl options: 
https://bugs.openjdk.java.net/browse/JDK-8196510 

Hotspot tests may require further updating or just removed: 
https://bugs.openjdk.java.net/browse/JDK-8194310 

jdeprescan will need updates due to the removal of the Java EE and CORBA modules: 
https://bugs.openjdk.java.net/browse/JDK-8194308 




Best,
Lance

  
   

  Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR 8190378: Java EE and CORBA modules removal

[Lance Andersen]

Hi Goetz,

I updated the webrev with the suggested update 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/tests_webrev/test/jdk/tools/launcher/HelpFlagsTest.java.sdiff.html

Best
Lance
> On Feb 7, 2018, at 3:20 PM, Lindenmaier, Goetz  
> wrote:
> 
> Hi Lance, 
> 
> Would you mind to add similar change as for 
> test/jdk/tools/launcher/VersionCheck.java
> also for 
> test/jdk/tools/launcher/HelpFlagsTest.java
> 
> That test is pretty recent which is why you may have missed it.
> It will not fail though, there will just be left over stuff in it.
> 
> Best regards,
>  Goetz.
> 
> 
> -Original Message-
> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf 
> Of Lance Andersen
> Sent: Wednesday, February 7, 2018 5:57 PM
> To: core-libs-dev ; build-dev 
> 
> Subject: RFR 8190378: Java EE and CORBA modules removal
> 
> Hi all,
> 
> I think we are at a point where we are ready to start reviewing  the changes 
> to remove the Java EE and CORBA modules as JEP 320, JDK-8189188,  has been  
> targeted to JDK 11.
> The CSR for removing the modules has been approved: 
> https://bugs.openjdk.java.net/browse/JDK-8193757 
> 
> 
> The open webrev can be found at:  
> http://cr.openjdk.java.net/~lancea/8190378/open_changes/ 
> 
> 
> To make the open review easier, I have broken the changes into 5 webrevs:
> build changes are: 
> http://cr.openjdk.java.net/~lancea/8190378/open_changes/build_webrev/ 
> 
> miscellaneous changes are at:  
> http://cr.openjdk.java.net/~lancea/8190378/open_changes/misc_webrev/ 
> 
> module changes are at: 
> http://cr.openjdk.java.net/~lancea/8190378/open_changes/modules_webrev/ 
> 
> rmic changes are at:  
> http://cr.openjdk.java.net/~lancea/8190378/open_changes/rmic_webrev/ 
> 
> test changes are at: 
> http://cr.openjdk.java.net/~lancea/8190378/open_changes/tests_webrev/ 
> 
> 
> As part of  the removal, the following issues have also been logged:
> Removal of the Java EE and CORBA tools from the documentation: 
> https://bugs.openjdk.java.net/browse/JDK-8193906 
> 
> Updating the RMIC man pages for the removal of the -iiop and -idl options: 
> https://bugs.openjdk.java.net/browse/JDK-8196510 
> 
> Hotspot tests may require further updating or just removed: 
> https://bugs.openjdk.java.net/browse/JDK-8194310 
> 
> jdeprescan will need updates due to the removal of the Java EE and CORBA 
> modules: https://bugs.openjdk.java.net/browse/JDK-8194308 
> 
> 
> 
> 
> Best,
> Lance
> 
> 
>  
> 
> Lance Andersen| 
> Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com 
> 
> 
> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

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

[Robin Westberg]

Hi Alan,

> On 7 Feb 2018, at 16:30, Alan Bateman  wrote:
> 
> On 07/02/2018 15:18, Robin Westberg wrote:
>> Hi all,
>> 
>> Please review the following change that adds an event-based tracing event 
>> that is generated when the VM shuts down. The intent is to capture shutdowns 
>> that occur after the VM has been properly initialized (as initialization 
>> problems would most likely mean that the tracing framework hasn’t been 
>> properly started either).
>> 
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8041626
>> Webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00/ 
>> 
>> Testing: hs-tier1,hs-tier2,jdk-tier1,jdk-tier2
>> 
> Can you elaborate a bit on why this isn't in JVM_Halt? Is this partially to 
> help with cases where the shutdown hooks or finalizers run at exit cause 
> issues?

Sure, the main problem I had with that approach is actually that the tracing 
framework shuts down from a shutdown hook, so events generated in JVM_Halt will 
be lost. And I couldn’t think of any good way to capture the exit code and 
proper stack trace from inside that shutdown hook, but I could perhaps explore 
that option a bit further.

Best regards,
Robin

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

[Erik Gahlin]

Hi Robin,

I can sponsor this.

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


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


Thanks
Erik


Hi all,

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


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


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

Best regards,
Robin

RE: RFR 8190378: Java EE and CORBA modules removal

[Lindenmaier, Goetz]

Hi Lance, 

Would you mind to add similar change as for 
test/jdk/tools/launcher/VersionCheck.java
also for 
test/jdk/tools/launcher/HelpFlagsTest.java

That test is pretty recent which is why you may have missed it.
It will not fail though, there will just be left over stuff in it.

Best regards,
  Goetz.


-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf 
Of Lance Andersen
Sent: Wednesday, February 7, 2018 5:57 PM
To: core-libs-dev ; build-dev 

Subject: RFR 8190378: Java EE and CORBA modules removal

Hi all,

I think we are at a point where we are ready to start reviewing  the changes to 
remove the Java EE and CORBA modules as JEP 320, JDK-8189188,  has been  
targeted to JDK 11.
The CSR for removing the modules has been approved: 
https://bugs.openjdk.java.net/browse/JDK-8193757 


 The open webrev can be found at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/ 


To make the open review easier, I have broken the changes into 5 webrevs:
build changes are: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/build_webrev/ 

miscellaneous changes are at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/misc_webrev/ 

module changes are at: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/modules_webrev/ 

rmic changes are at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/rmic_webrev/ 

test changes are at: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/tests_webrev/ 


As part of  the removal, the following issues have also been logged:
Removal of the Java EE and CORBA tools from the documentation: 
https://bugs.openjdk.java.net/browse/JDK-8193906 

Updating the RMIC man pages for the removal of the -iiop and -idl options: 
https://bugs.openjdk.java.net/browse/JDK-8196510 

Hotspot tests may require further updating or just removed: 
https://bugs.openjdk.java.net/browse/JDK-8194310 

jdeprescan will need updates due to the removal of the Java EE and CORBA 
modules: https://bugs.openjdk.java.net/browse/JDK-8194308 




Best,
Lance

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

RFR : 8196854 :TestFlushableGZIPOutputStream failing with IndexOutOfBoundsException

[Seán Coffey]

A jdk8u (and earlier) issue where some new/recent test code meant that 
IndexOutOfBoundsException could be thrown.


https://bugs.openjdk.java.net/browse/JDK-8196854
http://cr.openjdk.java.net/~coffeys/webrev.8196854/webrev/

The buf array was never required and I've verified that the original 
8189789 issue can still be reproduced with a buggy JDK.


--
Regards,
Sean.

Re: RFR 8196298 Add null Reader and Writer

[Patrick Reinhart]

> Am 07.02.2018 um 14:03 schrieb Alan Bateman :
> 
> On 03/02/2018 17:05, Patrick Reinhart wrote:
>> :
>> I reworked the tests and Writer implementation accordingly
>> 
>> http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01
>> 
> Just catching up on this.
> 
> nullReader’s javadoc suggests that mark(int) does not nothing but this seems 
> to conflict with the Reader's javadoc where it is specified to throw IOE when 
> mark is not supported.

So I will change the API description accordingly there…

> 
> I'm a bit uneasy with len==0 check in the read method. I realize this is 
> trying to mirror InputStream.read and maybe some Reader implementations but 
> it's not specified behavior. I think we have to re-examine the Reader spec to 
> clarify this to avoid creating more inconsistencies.

That’s exactly what I did, so for me it’s ok to look into the Reader spec also 
as I already modify that File. I personally thought that a null like reader 
will always be at the end of the stream and therefore return -1 and do no 
checking of how much space may be left on the consuming end. That seem to me 
more the natural way of thinking.

> 
> Similarly in nullWriter() where it looks like the the len==0 checks is in 
> unspecified territory. I think this will need clarifications to the Writer 
> spec. One obvious inconsistency is to close the writer and call write("") and 
> write("",  0, 0). The first will fail as the writer is closed, the second 
> does not fail.

Here I think the same holds true as I wrote for the Reader…

> 
> Another one is read(CharBuffer). Suppose CharBuffer cb has 0 bytes remaining; 
> if I invoke nullReader().read(cb) then it looks like it will return 0 whereas 
> the read(CharBuffer) method is specified to return -1.
> 

I see your point there, the implementation I did there was taken from the 
StringReader, where it will be the same len==0 check as you mentioned before… 
So it looks like we really need to look into those implementations…

-Patrick

Re: RFR 8190378: Java EE and CORBA modules removal

[Erik Joelsson]

Build changes look good.

/Erik


On 2018-02-07 08:57, Lance Andersen wrote:

Hi all,

I think we are at a point where we are ready to start reviewing  the changes to 
remove the Java EE and CORBA modules as JEP 320, JDK-8189188,  has been  
targeted to JDK 11.
The CSR for removing the modules has been approved: 
https://bugs.openjdk.java.net/browse/JDK-8193757 


  The open webrev can be found at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/ 


To make the open review easier, I have broken the changes into 5 webrevs:
build changes are: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/build_webrev/ 

miscellaneous changes are at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/misc_webrev/ 

module changes are at: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/modules_webrev/ 

rmic changes are at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/rmic_webrev/ 

test changes are at: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/tests_webrev/ 


As part of  the removal, the following issues have also been logged:
Removal of the Java EE and CORBA tools from the documentation: 
https://bugs.openjdk.java.net/browse/JDK-8193906 

Updating the RMIC man pages for the removal of the -iiop and -idl options: 
https://bugs.openjdk.java.net/browse/JDK-8196510 

Hotspot tests may require further updating or just removed: 
https://bugs.openjdk.java.net/browse/JDK-8194310 

jdeprescan will need updates due to the removal of the Java EE and CORBA modules: 
https://bugs.openjdk.java.net/browse/JDK-8194308 




Best,
Lance

  
   

  Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

RFR 8190378: Java EE and CORBA modules removal

[Lance Andersen]

Hi all,

I think we are at a point where we are ready to start reviewing  the changes to 
remove the Java EE and CORBA modules as JEP 320, JDK-8189188,  has been  
targeted to JDK 11.
The CSR for removing the modules has been approved: 
https://bugs.openjdk.java.net/browse/JDK-8193757 


 The open webrev can be found at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/ 


To make the open review easier, I have broken the changes into 5 webrevs:
build changes are: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/build_webrev/ 

miscellaneous changes are at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/misc_webrev/ 

module changes are at: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/modules_webrev/ 

rmic changes are at:  
http://cr.openjdk.java.net/~lancea/8190378/open_changes/rmic_webrev/ 

test changes are at: 
http://cr.openjdk.java.net/~lancea/8190378/open_changes/tests_webrev/ 


As part of  the removal, the following issues have also been logged:
Removal of the Java EE and CORBA tools from the documentation: 
https://bugs.openjdk.java.net/browse/JDK-8193906 

Updating the RMIC man pages for the removal of the -iiop and -idl options: 
https://bugs.openjdk.java.net/browse/JDK-8196510 

Hotspot tests may require further updating or just removed: 
https://bugs.openjdk.java.net/browse/JDK-8194310 

jdeprescan will need updates due to the removal of the Java EE and CORBA 
modules: https://bugs.openjdk.java.net/browse/JDK-8194308 




Best,
Lance

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: [PATCH] Reduce Chance Of Mistakenly Early Backing Memory Cleanup

[Vladimir Ivanov]

Vladimir, just to be sure I presume your analysis applies to both C1 and C2? 
what about compilers such as Graal?


I only looked at C1 & C2, but I'm sure it applies to Graal as well: GC 
interaction mechanism is the same for all JIT-compilers in HotSpot.


Best regards,
Vladimir Ivanov

Re: [PATCH] Reduce Chance Of Mistakenly Early Backing Memory Cleanup

[Paul Sandoz]

> On Feb 7, 2018, at 7:22 AM, Vladimir Ivanov  
> wrote:
> 
> Peter,
> 
>>> Objects.requireNonNull() shows zero overhead here.
>>> 
>>> I guess the main question is whether Objects.requireNonNull(this) behavior 
>>> in the former test is a result of chance and current Hotspot behavior or is 
>>> it somehow guaranteed by the spec.
>> I haven't looked into what actually happens in JIT-compilers on your 
>> benchmark, but I'm surprised it works at all.
> 
> So, here's why Objects.requireNonNull() keeps the receiver alive in your test 
> case.
> 
> JIT-compilers in HotSpot aggressively prune dead locals [1], but they do that 
> based on method bytecode analysis [2] (and not on optimized IR). So, any 
> usage of a local extends its live range, even if that usage is eliminated in 
> generated code. It means an oop in that local will live past its last usage 
> in generated code and all safepoints (in generated code) till last usage (on 
> bytecode level) will enumerate the local it is held in.
> 
> If there were GC-only safepoints supported, then JITs could still prune 
> unused locals from oop maps, but HotSpot doesn't have them and all safepoints 
> in generated code keep full JVM state, so it's always possible to deoptimize 
> at any one of them (and then run into the code which is eliminated in 
> generated code).
> 
> If there are no safepoints till the end of the method, then nothing will keep 
> the object alive. But there's no way for GC to collect it, since GCs rely on 
> safepoints to mark thread stack. (That's why I mentioned GC-only safepoints 
> earlier.)
> 
> As a conclusion: removing @DontInline on Reference.reachabilityFence() should 
> eliminate most of the overhead (no call anymore, additional spill may be 
> needed) and still keep it working. It's not guaranteed by JVMS, but at least 
> should work on HotSpot JVM (in its current state).
> 
> So, nice discovery, Peter! :-)

Yes, Peter, thank you, for pushing on this. We would of course need to be 
careful and revisit if any changes occur.

Vladimir, just to be sure I presume your analysis applies to both C1 and C2? 
what about compilers such as Graal?

Ben, i still think additional performance analysis is still valuable (such 
performance tests are also useful for another reason, consolidating unsafe 
accesses using the double addressing mode, thereby removing another difference 
between heap and direct buffers).

Paul.

> Want to file an RFE & fix it?
> 
> Best regards,
> Vladimir Ivanov
> 
> [1] 
> http://hg.openjdk.java.net/jdk/hs/file/45b6aae769cc/src/hotspot/share/opto/graphKit.cpp#l736
> 
> [2] 
> http://hg.openjdk.java.net/jdk/hs/file/45b6aae769cc/src/hotspot/share/compiler/methodLiveness.cpp#l37
> 
>> Explicit null check on the receiver is an easy target for elimination and 
>> should be effectively a no-op in generated code. (And that's what you 
>> observe with the benchmark!) Once the check is gone, nothing keeps receiver 
>> alive anymore (past the last usage).
>> So, I'd say such behavior it's a matter of chance in your case and can't be 
>> relied on in general. And definitely not something guaranteed by JVMS.
>> Best regards,
>> Vladimir Ivanov

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

[Alan Bateman]

On 07/02/2018 15:18, Robin Westberg wrote:

Hi all,

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


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


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

Can you elaborate a bit on why this isn't in JVM_Halt? Is this partially 
to help with cases where the shutdown hooks or finalizers run at exit 
cause issues?


-Alan

Re: [PATCH] Reduce Chance Of Mistakenly Early Backing Memory Cleanup

[Vladimir Ivanov]

Peter,




Objects.requireNonNull() shows zero overhead here.

I guess the main question is whether Objects.requireNonNull(this) 
behavior in the former test is a result of chance and current Hotspot 
behavior or is it somehow guaranteed by the spec.


I haven't looked into what actually happens in JIT-compilers on your 
benchmark, but I'm surprised it works at all.


So, here's why Objects.requireNonNull() keeps the receiver alive in your 
test case.


JIT-compilers in HotSpot aggressively prune dead locals [1], but they do 
that based on method bytecode analysis [2] (and not on optimized IR). 
So, any usage of a local extends its live range, even if that usage is 
eliminated in generated code. It means an oop in that local will live 
past its last usage in generated code and all safepoints (in generated 
code) till last usage (on bytecode level) will enumerate the local it is 
held in.


If there were GC-only safepoints supported, then JITs could still prune 
unused locals from oop maps, but HotSpot doesn't have them and all 
safepoints in generated code keep full JVM state, so it's always 
possible to deoptimize at any one of them (and then run into the code 
which is eliminated in generated code).


If there are no safepoints till the end of the method, then nothing will 
keep the object alive. But there's no way for GC to collect it, since 
GCs rely on safepoints to mark thread stack. (That's why I mentioned 
GC-only safepoints earlier.)


As a conclusion: removing @DontInline on Reference.reachabilityFence() 
should eliminate most of the overhead (no call anymore, additional spill 
may be needed) and still keep it working. It's not guaranteed by JVMS, 
but at least should work on HotSpot JVM (in its current state).


So, nice discovery, Peter! :-) Want to file an RFE & fix it?

Best regards,
Vladimir Ivanov

[1] 
http://hg.openjdk.java.net/jdk/hs/file/45b6aae769cc/src/hotspot/share/opto/graphKit.cpp#l736


[2] 
http://hg.openjdk.java.net/jdk/hs/file/45b6aae769cc/src/hotspot/share/compiler/methodLiveness.cpp#l37


Explicit null check on the receiver is an easy target for elimination 
and should be effectively a no-op in generated code. (And that's what 
you observe with the benchmark!) Once the check is gone, nothing keeps 
receiver alive anymore (past the last usage).


So, I'd say such behavior it's a matter of chance in your case and can't 
be relied on in general. And definitely not something guaranteed by JVMS.


Best regards,
Vladimir Ivanov

RFR: 8041626: [Event Request] Shutdown reason

[Robin Westberg]

Hi all,

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

Issue: https://bugs.openjdk.java.net/browse/JDK-8041626 

Webrev: http://cr.openjdk.java.net/~rwestberg/8041626/webrev.00/ 

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

Best regards,
Robin

Re: RFR: 8196869: Optimize Locale creation

[Claes Redestad]

On 2018-02-07 16:01, Peter Levart wrote:

Hi Claes,

On 02/07/2018 03:23 PM, Claes Redestad wrote:

Updated webrev:

http://cr.openjdk.java.net/~redestad/8196869/jdk.02/

- use map.remove(entry.getKey(), entry) instead of 
map.remove(entry.getKey())
- for most Locales, Locale$LocaleKey.exts is null, so using the 
BaseLocale as key
  directly in Locale allows us to avoid loading Locale$LocaleKey 
except in exceptional

  circumstances.
- use map.replace to safely update the entry when putIfAbsent returns 
an object
  but it points to a cleared value, so that (benign) races to create 
new Locale objects
  will canonicalize 


I don't know it it's only me, but I see an old version of 
LocaleObjectCache in webrev at above URL.


Sorry about that - the intended webrev have now replace the old on in-place.

/Claes

Re: RFR: 8196869: Optimize Locale creation

[Peter Levart]

Hi Claes,

On 02/07/2018 03:23 PM, Claes Redestad wrote:

Updated webrev:

http://cr.openjdk.java.net/~redestad/8196869/jdk.02/

- use map.remove(entry.getKey(), entry) instead of 
map.remove(entry.getKey())
- for most Locales, Locale$LocaleKey.exts is null, so using the 
BaseLocale as key
  directly in Locale allows us to avoid loading Locale$LocaleKey 
except in exceptional

  circumstances.
- use map.replace to safely update the entry when putIfAbsent returns 
an object
  but it points to a cleared value, so that (benign) races to create 
new Locale objects
  will canonicalize 


I don't know it it's only me, but I see an old version of 
LocaleObjectCache in webrev at above URL.


Regards, Peter

Re: RFR: 8196869: Optimize Locale creation

[Claes Redestad]

Hi,

On 2018-02-07 13:55, Peter Levart wrote:


Would making CacheEntry extend jdk.internal.ref.SoftCleanable instead 
of SoftReference help here? You could remove the cleanStaleEntries 
method entirely and just remove the Map entry in SoftCleanable's 
performCleanup method.


possible, but that'd be a larger change than I'm comfortable with for now.

As Locale is initialized on bootstrap, a Cleaner-based impl. would mean 
starting
an innocuous thread unconditionally, which would defeat the intent to 
optimize
the minimal time to bootstrap the JVM. If we could tease things apart 
even further

so that a SoftCleanable and Cleaners are only set up when initializing any
non-constant Locale then I think we should contemplate this as a follow up.

Updated webrev:

http://cr.openjdk.java.net/~redestad/8196869/jdk.02/

- use map.remove(entry.getKey(), entry) instead of 
map.remove(entry.getKey())
- for most Locales, Locale$LocaleKey.exts is null, so using the 
BaseLocale as key
  directly in Locale allows us to avoid loading Locale$LocaleKey except 
in exceptional

  circumstances.
- use map.replace to safely update the entry when putIfAbsent returns an 
object
  but it points to a cleared value, so that (benign) races to create 
new Locale objects

  will canonicalize

/Claes

[PATCH] ObjectInputStream Reading Performance Optimisation

[Ben Walsh]

As per the guidance here - 
http://mail.openjdk.java.net/pipermail/valhalla-dev/2018-February/003772.html
, I am resubmitting this patch to this mailing list ...


The main advantage of this optimisation is that a suitably "aware" 
compiler can avoid stack walking by using a private method named 
redirectedReadObject that can be called from a call site that was calling 
readObject originally (in some cases). The conditions when this can be 
done are essentially when we know that the caller method's class is a 
"user defined class" because we have that caller method/class at compile 
time and can check if it was loaded by a user defined class loader. If so, 
the compiler emits code to call the private redirectedReadObject method 
(instead of readObject that was there in the byte codes) and it avoids 
doing a stack walk to discover the latest user class loader because it has 
an extra parameter that is a class loader that can be passed in, in cases 
when the compiler knows the answer at compile time. In such cases, the 
compiler generates code to pass in the latest user defined class loader 
known to redirectedReadObject.

I would like to pair with a sponsor to contribute this patch ...

---

diff -r 60c19c384333 src/java.base/share/classes/java/io/ClassCache.java
--- /dev/null   Thu Jan 01 00:00:00 1970 +
+++ b/src/java.base/share/classes/java/io/ClassCache.java   Fri Feb 02 
11:52:41 2018 +
@@ -0,0 +1,403 @@
+/*
+ * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but 
WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file 
that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License 
version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 
USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package java.io;
+
+import java.lang.ref.ReferenceQueue;
+import java.lang.ref.WeakReference;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+/* ClassCache is Primarily responsible for Caching the results of the 
className lookups and hence to avoid 
+ * multiple Lookup for same Class Instance.
+ * ClassCache provides a ConcurrentHash based ClassCache which is looked 
up prior to calling the class.forName
+ *  Method resolveClass() from ObjectInputStream uses this Cache.
+ * 
+ *  Caching is done only when the actually used loader for a Class is one 
of the Sytem loaders (ie) App Class Loader,
+ *  System Extension loader and BootStrap loader
+ * 
+ */
+final class ClassCache {
+/* Main Cache for storing the Class.forName results Here Key used would 
be CacheKey */
+private final ConcurrentHashMap cache =
+new ConcurrentHashMap(); 
+/* Initiating Loader to CacheKey mapping in the Cache used by Reaper 
thread for removal on stale Loaders */
+private final ConcurrentHashMap loaderKeys =
+new ConcurrentHashMap();
+ /* Keeps a link of Actual System Loader used to Initiating Loader 
mapping */
+private final ConcurrentHashMap 
canonicalLoaderRefs =
+new ConcurrentHashMap();
+ /* Reference Queue registered for notification on stale Loaders */
+private final ReferenceQueue staleLoaderRefs =
+new ReferenceQueue();
+/*
+ * Constructor Populates Canonical Loader Refs with System Loader Entries 
and initializes the reaper thread which
+ * monitors the ReferenceQueue for stale loaders
+ */
+ 
+public ClassCache() {
+ClassLoader loader = ClassLoader.getSystemClassLoader(); 
+while (loader != null) {
+setCanonicalSystemLoaderRef(loader);
+loader = loader.getParent();
+}
+setCanonicalSystemLoaderRef(null);
+AccessController.doPrivileged(
+new CreateReaperAction(this, staleLoaderRefs)).start();
+}
+/*
+ * sets Canonical Loader reference for the loader
+ */
+
+private void setCanonicalSystemLoaderRef(ClassLoader loader) {
+ 

Re: RFR 8196298 Add null Reader and Writer

[Alan Bateman]

On 03/02/2018 17:05, Patrick Reinhart wrote:

:
I reworked the tests and Writer implementation accordingly

http://cr.openjdk.java.net/~reinhapa/reviews/8196298/webrev.01


Just catching up on this.

nullReader's javadoc suggests that mark(int) does not nothing but this 
seems to conflict with the Reader's javadoc where it is specified to 
throw IOE when mark is not supported.


I'm a bit uneasy with len==0 check in the read method. I realize this is 
trying to mirror InputStream.read and maybe some Reader implementations 
but it's not specified behavior. I think we have to re-examine the 
Reader spec to clarify this to avoid creating more inconsistencies.


Similarly in nullWriter() where it looks like the the len==0 checks is 
in unspecified territory. I think this will need clarifications to the 
Writer spec. One obvious inconsistency is to close the writer and call 
write("") and write("",  0, 0). The first will fail as the writer is 
closed, the second does not fail.


Another one is read(CharBuffer). Suppose CharBuffer cb has 0 bytes 
remaining; if I invoke nullReader().read(cb) then it looks like it will 
return 0 whereas the read(CharBuffer) method is specified to return -1.


-Alan

Re: RFR: 8196869: Optimize Locale creation

[Peter Levart]

Hi Claes,

On 02/07/2018 01:48 PM, Claes Redestad wrote:

Hi,

On 2018-02-07 13:12, Peter Levart wrote:

Hi Claes,

I studied the code briefly and understand why BaseLocale.Key now has 
to hold a SoftReference to a BaseLocale object when the same object 
is also part of CacheEntry which is also a SoftReference. But I don't 
see a reason why pre-patch BaseLocale.Key had to hold 
SoftReference(s) to individual String attributes. Couldn't it simply 
hold strong references to individual String attributes instead? The 
LocaleObjectCache.cleanStaleEntryies() would remove cleared 
CacheEntry(s) together with corresponding Key(s) in that case too. So 
one SoftReference less, do you agree?


cleanStaleEntries is sufficient for making sure the Key gets cleared 
eventually, yes, but having
part of the Key softly reachable expedites memory reclamation in some 
important cases.


When no cleanStaleEntries() is called for a long time, right?

Would making CacheEntry extend jdk.internal.ref.SoftCleanable instead of 
SoftReference help here? You could remove the cleanStaleEntries method 
entirely and just remove the Map entry in SoftCleanable's performCleanup 
method.


Regards, Peter





I don't know if it is important for LocaleObjectCache.get() to always 
return a canonicalized instance per key so that this always holds:


    (cache.get(k1) == cache.get(k2)) == k1.equals(k2)


I believe LocaleObjectCache is intended as a best effort cache 
solution with soft memory
semantics for performance, not a strict canonicalization facility. 
That said I think removing

the race you pointed out here is probably a good thing to do.

/Claes



If it is important, then I noticed a pre-existing race that violates 
above invariant:


  67 CacheEntry newEntry = new CacheEntry<>(key, 
newVal, queue);

  68
  69 entry = map.putIfAbsent(key, newEntry);
  70 if (entry == null) {
  71 value = newVal;
  72 } else {
  73 value = entry.get();
  74 if (value == null) {
  75 map.put(key, newEntry);
  76 value = newVal;
  77 }
  78 }

...which can simply be fixed:

    CacheEntry newEntry = new CacheEntry<>(key, newVal, 
queue);


    while (true) {
    entry = map.putIfAbsent(key, newEntry);
    if (entry == null) {
    value = newVal;
    break;
    } else {
    value = entry.get();
    if (value == null) {
    if (map.replace(key, entry, newEntry)) {
    value = newVal;
    break;
    }
    }
    }
    }


Regards, Peter


On 02/07/2018 11:26 AM, Claes Redestad wrote:

Hi Paul,


On 2018-02-06 20:55, Paul Sandoz wrote:

Quick observation:

  261 private BaseLocale getBaseLocale() {
  262 return (holder == null) ? holderRef.get() : holder;
  263 }

This method can return null if the soft ref has been cleared.


But you don’t check in equals:

  270 if (obj instanceof Key && this.hash == 
((Key)obj).hash) {

  271 BaseLocale other = ((Key) obj).getBaseLocale();
  272 BaseLocale locale = this.getBaseLocale();
  273 if 
(LocaleUtils.caseIgnoreMatch(other.getLanguage(), 
locale.getLanguage())


good eye!

It seems this wasn't caught by the existing regression tests since 
none of them
recreate Locales in that are likely to have been reclaimed, but 
still likely to still
be in the CHM (it's a race of sorts since they'll be removed when 
the ReferenceQueue

processing happen).

I added a regression test with the smallest and quickest reproducer 
I could come up
with that provokes a NPE if we don't check null along with the fix 
to Key#equals:


http://cr.openjdk.java.net/~redestad/8196869/jdk.01/

For the normalize(Key) case we can deduce that a !normalized Key 
will always have
a strongly referenced BaseLocale and thus not need to deal with 
getBaseLocale()
returning null. I clarified this in the code and added an assert 
(that would be triggered

by the added test if it wasn't true).

Thanks!

/Claes

Re: RFR: 8196869: Optimize Locale creation

[Claes Redestad]

Hi,

On 2018-02-07 13:12, Peter Levart wrote:

Hi Claes,

I studied the code briefly and understand why BaseLocale.Key now has 
to hold a SoftReference to a BaseLocale object when the same object is 
also part of CacheEntry which is also a SoftReference. But I don't see 
a reason why pre-patch BaseLocale.Key had to hold SoftReference(s) to 
individual String attributes. Couldn't it simply hold strong 
references to individual String attributes instead? The 
LocaleObjectCache.cleanStaleEntryies() would remove cleared 
CacheEntry(s) together with corresponding Key(s) in that case too. So 
one SoftReference less, do you agree?


cleanStaleEntries is sufficient for making sure the Key gets cleared 
eventually, yes, but having
part of the Key softly reachable expedites memory reclamation in some 
important cases.




I don't know if it is important for LocaleObjectCache.get() to always 
return a canonicalized instance per key so that this always holds:


    (cache.get(k1) == cache.get(k2)) == k1.equals(k2)


I believe LocaleObjectCache is intended as a best effort cache solution 
with soft memory
semantics for performance, not a strict canonicalization facility. That 
said I think removing

the race you pointed out here is probably a good thing to do.

/Claes



If it is important, then I noticed a pre-existing race that violates 
above invariant:


  67 CacheEntry newEntry = new CacheEntry<>(key, 
newVal, queue);

  68
  69 entry = map.putIfAbsent(key, newEntry);
  70 if (entry == null) {
  71 value = newVal;
  72 } else {
  73 value = entry.get();
  74 if (value == null) {
  75 map.put(key, newEntry);
  76 value = newVal;
  77 }
  78 }

...which can simply be fixed:

    CacheEntry newEntry = new CacheEntry<>(key, newVal, 
queue);


    while (true) {
    entry = map.putIfAbsent(key, newEntry);
    if (entry == null) {
    value = newVal;
    break;
    } else {
    value = entry.get();
    if (value == null) {
    if (map.replace(key, entry, newEntry)) {
    value = newVal;
    break;
    }
    }
    }
    }


Regards, Peter


On 02/07/2018 11:26 AM, Claes Redestad wrote:

Hi Paul,


On 2018-02-06 20:55, Paul Sandoz wrote:

Quick observation:

  261 private BaseLocale getBaseLocale() {
  262 return (holder == null) ? holderRef.get() : holder;
  263 }

This method can return null if the soft ref has been cleared.


But you don’t check in equals:

  270 if (obj instanceof Key && this.hash == 
((Key)obj).hash) {

  271 BaseLocale other = ((Key) obj).getBaseLocale();
  272 BaseLocale locale = this.getBaseLocale();
  273 if 
(LocaleUtils.caseIgnoreMatch(other.getLanguage(), locale.getLanguage())


good eye!

It seems this wasn't caught by the existing regression tests since 
none of them
recreate Locales in that are likely to have been reclaimed, but still 
likely to still
be in the CHM (it's a race of sorts since they'll be removed when the 
ReferenceQueue

processing happen).

I added a regression test with the smallest and quickest reproducer I 
could come up
with that provokes a NPE if we don't check null along with the fix to 
Key#equals:


http://cr.openjdk.java.net/~redestad/8196869/jdk.01/

For the normalize(Key) case we can deduce that a !normalized Key will 
always have
a strongly referenced BaseLocale and thus not need to deal with 
getBaseLocale()
returning null. I clarified this in the code and added an assert 
(that would be triggered

by the added test if it wasn't true).

Thanks!

/Claes

Re: RFR: 8196869: Optimize Locale creation

[Peter Levart]

Hi Claes,

Maybe I was to quick with my clicking on Send button... If the Key 
simply held strong references to individual String attributes, 
LocaleObjectCache.cleanStaleEntries would also have to be modified to 
make sure it does not remove valid entries that happen to share equal 
Key(s) with cleared entries. So instead of this:


    private void cleanStaleEntries() {
    CacheEntry entry;
    while ((entry = (CacheEntry)queue.poll()) != null) {
    map.remove(entry.getKey());
    }
    }

The method would have to be like this:

    private void cleanStaleEntries() {
    CacheEntry entry;
    while ((entry = (CacheEntry)queue.poll()) != null) {
    map.remove(entry.getKey(), entry);
    }
    }

(Notice the use of two-argument Map.remove() method in the modified 
variant).


Regards, Peter

P.S. I now understand the hypothetical need to have individual String 
attributes wrapped with SoftReference(s) in pre-patched Key. The code 
maybe relied on the fact that SoftReference(s) to individual String 
attributes were cleared together with CacheEntry(s). When they were 
cleared, such Keys suddenly only matched themselves (i.e. no other Key 
instance would be equal to them). But if Key's SoftReference(s) were not 
cleared before corresponding CacheEntry was cleared, cleanStaleEntries() 
running concurrently with get() could remove freshly inserted entries 
too. This would not be observed as wrong behavior though. Just 
sub-optimal performance.


On 02/07/2018 01:12 PM, Peter Levart wrote:

Hi Claes,

I studied the code briefly and understand why BaseLocale.Key now has 
to hold a SoftReference to a BaseLocale object when the same object is 
also part of CacheEntry which is also a SoftReference. But I don't see 
a reason why pre-patch BaseLocale.Key had to hold SoftReference(s) to 
individual String attributes. Couldn't it simply hold strong 
references to individual String attributes instead? The 
LocaleObjectCache.cleanStaleEntryies() would remove cleared 
CacheEntry(s) together with corresponding Key(s) in that case too. So 
one SoftReference less, do you agree?


I don't know if it is important for LocaleObjectCache.get() to always 
return a canonicalized instance per key so that this always holds:


    (cache.get(k1) == cache.get(k2)) == k1.equals(k2)

If it is important, then I noticed a pre-existing race that violates 
above invariant:


  67 CacheEntry newEntry = new CacheEntry<>(key, 
newVal, queue);

  68
  69 entry = map.putIfAbsent(key, newEntry);
  70 if (entry == null) {
  71 value = newVal;
  72 } else {
  73 value = entry.get();
  74 if (value == null) {
  75 map.put(key, newEntry);
  76 value = newVal;
  77 }
  78 }

...which can simply be fixed:

    CacheEntry newEntry = new CacheEntry<>(key, newVal, 
queue);


    while (true) {
    entry = map.putIfAbsent(key, newEntry);
    if (entry == null) {
    value = newVal;
    break;
    } else {
    value = entry.get();
    if (value == null) {
    if (map.replace(key, entry, newEntry)) {
    value = newVal;
    break;
    }
    }
    }
    }


Regards, Peter


On 02/07/2018 11:26 AM, Claes Redestad wrote:

Hi Paul,


On 2018-02-06 20:55, Paul Sandoz wrote:

Quick observation:

  261 private BaseLocale getBaseLocale() {
  262 return (holder == null) ? holderRef.get() : holder;
  263 }

This method can return null if the soft ref has been cleared.


But you don’t check in equals:

  270 if (obj instanceof Key && this.hash == 
((Key)obj).hash) {

  271 BaseLocale other = ((Key) obj).getBaseLocale();
  272 BaseLocale locale = this.getBaseLocale();
  273 if 
(LocaleUtils.caseIgnoreMatch(other.getLanguage(), locale.getLanguage())


good eye!

It seems this wasn't caught by the existing regression tests since 
none of them
recreate Locales in that are likely to have been reclaimed, but still 
likely to still
be in the CHM (it's a race of sorts since they'll be removed when the 
ReferenceQueue

processing happen).

I added a regression test with the smallest and quickest reproducer I 
could come up
with that provokes a NPE if we don't check null along with the fix to 
Key#equals:


http://cr.openjdk.java.net/~redestad/8196869/jdk.01/

For the normalize(Key) case we can deduce that a !normalized Key will 
always have
a strongly referenced BaseLocale and thus not need to deal with 
getBaseLocale()
returning null. I clarified this in the code and added an assert 
(that would be triggered

by the added test if it wasn't true).

Thank

Re: RFR: 8196869: Optimize Locale creation

[Peter Levart]

Hi Claes,

I studied the code briefly and understand why BaseLocale.Key now has to 
hold a SoftReference to a BaseLocale object when the same object is also 
part of CacheEntry which is also a SoftReference. But I don't see a 
reason why pre-patch BaseLocale.Key had to hold SoftReference(s) to 
individual String attributes. Couldn't it simply hold strong references 
to individual String attributes instead? The 
LocaleObjectCache.cleanStaleEntryies() would remove cleared 
CacheEntry(s) together with corresponding Key(s) in that case too. So 
one SoftReference less, do you agree?


I don't know if it is important for LocaleObjectCache.get() to always 
return a canonicalized instance per key so that this always holds:


    (cache.get(k1) == cache.get(k2)) == k1.equals(k2)

If it is important, then I noticed a pre-existing race that violates 
above invariant:


  67 CacheEntry newEntry = new CacheEntry<>(key, 
newVal, queue);

  68
  69 entry = map.putIfAbsent(key, newEntry);
  70 if (entry == null) {
  71 value = newVal;
  72 } else {
  73 value = entry.get();
  74 if (value == null) {
  75 map.put(key, newEntry);
  76 value = newVal;
  77 }
  78 }

...which can simply be fixed:

    CacheEntry newEntry = new CacheEntry<>(key, newVal, 
queue);


    while (true) {
    entry = map.putIfAbsent(key, newEntry);
    if (entry == null) {
    value = newVal;
    break;
    } else {
    value = entry.get();
    if (value == null) {
    if (map.replace(key, entry, newEntry)) {
    value = newVal;
    break;
    }
    }
    }
    }


Regards, Peter


On 02/07/2018 11:26 AM, Claes Redestad wrote:

Hi Paul,


On 2018-02-06 20:55, Paul Sandoz wrote:

Quick observation:

  261 private BaseLocale getBaseLocale() {
  262 return (holder == null) ? holderRef.get() : holder;
  263 }

This method can return null if the soft ref has been cleared.


But you don’t check in equals:

  270 if (obj instanceof Key && this.hash == 
((Key)obj).hash) {

  271 BaseLocale other = ((Key) obj).getBaseLocale();
  272 BaseLocale locale = this.getBaseLocale();
  273 if 
(LocaleUtils.caseIgnoreMatch(other.getLanguage(), locale.getLanguage())


good eye!

It seems this wasn't caught by the existing regression tests since 
none of them
recreate Locales in that are likely to have been reclaimed, but still 
likely to still
be in the CHM (it's a race of sorts since they'll be removed when the 
ReferenceQueue

processing happen).

I added a regression test with the smallest and quickest reproducer I 
could come up
with that provokes a NPE if we don't check null along with the fix to 
Key#equals:


http://cr.openjdk.java.net/~redestad/8196869/jdk.01/

For the normalize(Key) case we can deduce that a !normalized Key will 
always have
a strongly referenced BaseLocale and thus not need to deal with 
getBaseLocale()
returning null. I clarified this in the code and added an assert (that 
would be triggered

by the added test if it wasn't true).

Thanks!

/Claes

Re: RFR: 8196869: Optimize Locale creation

[Claes Redestad]

Hi Paul,


On 2018-02-06 20:55, Paul Sandoz wrote:

Quick observation:

  261 private BaseLocale getBaseLocale() {
  262 return (holder == null) ? holderRef.get() : holder;
  263 }

This method can return null if the soft ref has been cleared.


But you don’t check in equals:

  270 if (obj instanceof Key && this.hash == ((Key)obj).hash) {
  271 BaseLocale other = ((Key) obj).getBaseLocale();
  272 BaseLocale locale = this.getBaseLocale();
  273 if (LocaleUtils.caseIgnoreMatch(other.getLanguage(), 
locale.getLanguage())


good eye!

It seems this wasn't caught by the existing regression tests since none 
of them
recreate Locales in that are likely to have been reclaimed, but still 
likely to still
be in the CHM (it's a race of sorts since they'll be removed when the 
ReferenceQueue

processing happen).

I added a regression test with the smallest and quickest reproducer I 
could come up
with that provokes a NPE if we don't check null along with the fix to 
Key#equals:


http://cr.openjdk.java.net/~redestad/8196869/jdk.01/

For the normalize(Key) case we can deduce that a !normalized Key will 
always have
a strongly referenced BaseLocale and thus not need to deal with 
getBaseLocale()
returning null. I clarified this in the code and added an assert (that 
would be triggered

by the added test if it wasn't true).

Thanks!

/Claes