Re: RFR: jsr166 jdk integration 2018-02

[Martin Buchholz]

On Thu, Feb 8, 2018 at 10:39 PM, David Holmes 
wrote:

>
> Well at least it's only an issue if there does exist a finalize() method
> that could interfere with the executing method. I have to assume that
> somehow the VM is clever enough that if 'this' is in a register and we will
> later access a field at this+offset, that the GC will not free the memory
> used by 'this'.
>

I expect the opposite, that the VM may see an opportunity to prefetch that
other field.  Maybe it needed to read an adjacent field, and was able to
read both fields with a single instruction.  Maybe escape analysis allowed
it to infer that the object could never be accessed by another thread that
might write to the field.

Even more extreme - in the case of
 newSingleThreadExecutor().execute(someTask);
the VM can do enough analysis to conclude the executor will never be used
after invoking super.execute, so the VM can "optimize" by constructing the
executor, immediately finalizing it, and only then calling the body of
execute.

Re: RFR: jsr166 jdk integration 2018-02

[David Holmes]

On 9/02/2018 3:35 PM, Martin Buchholz wrote:
On Thu, Feb 8, 2018 at 8:39 PM, David Holmes > wrote:

Wow! DelegatedExecutorService doesn't even have a finalize() method
that does a shutdown. So we have to put the reachabilityFence in it
to prevent the delegatee from becoming unreachable due to the
delegator becoming unreachable!

This whole notion that "this" can be unreachable whilst a method on
it is still executing is just broken - it's an unusable programming
model. Any code that does "new Foo().bar()" could fail unless
Foo.bar() has a reachability fence in it. :( Sheesh!

I've argued similarly even in the past month that "this" should always 
remain reachable while a method on "this" is executing, in the presence 
of a finalize method.  (It would also be nice if java language scope 
mapped to reachability range, but that information is already lost in 
the translation to bytecode.)  But apparently it is quite a burden on 
any implementation to ensure reachability, especially after inlining.


We've never had a report of this problem occurring in practice.


Well at least it's only an issue if there does exist a finalize() method 
that could interfere with the executing method. I have to assume that 
somehow the VM is clever enough that if 'this' is in a register and we 
will later access a field at this+offset, that the GC will not free the 
memory used by 'this'.


Somewhat ironic we add the reachabilityFence in the same changeset that 
removes the action of the finalize() method  in TPE. (But of course the 
ExecutorService passed to the delegating service may still have one.)


David

Re: RFR: jsr166 jdk integration 2018-02

[Martin Buchholz]

On Thu, Feb 8, 2018 at 8:39 PM, David Holmes 
wrote:

>
> Wow! DelegatedExecutorService doesn't even have a finalize() method that
> does a shutdown. So we have to put the reachabilityFence in it to prevent
> the delegatee from becoming unreachable due to the delegator becoming
> unreachable!
>
> This whole notion that "this" can be unreachable whilst a method on it is
> still executing is just broken - it's an unusable programming model. Any
> code that does "new Foo().bar()" could fail unless Foo.bar() has a
> reachability fence in it. :( Sheesh!
>

I've argued similarly even in the past month that "this" should always
remain reachable while a method on "this" is executing, in the presence of
a finalize method.  (It would also be nice if java language scope mapped to
reachability range, but that information is already lost in the translation
to bytecode.)  But apparently it is quite a burden on any implementation to
ensure reachability, especially after inlining.

We've never had a report of this problem occurring in practice.

Re: RFR: jsr166 jdk integration 2018-02

[David Holmes]

Hi Martin,

On 9/02/2018 2:07 PM, Martin Buchholz wrote:



On Thu, Feb 8, 2018 at 7:39 PM, David Holmes > wrote:


On 9/02/2018 11:19 AM, Martin Buchholz wrote:


http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html



8190324: ThreadPoolExecutor should not specify a dependency on
finalization

http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ThreadPoolExecutor-finalize/index.html


https://bugs.openjdk.java.net/browse/JDK-8190324



Looks okay.

Another set of uninteresting changes, some suggested by intellij:

8195590: Miscellaneous changes imported from jsr166 CVS 2018-02

http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html


https://bugs.openjdk.java.net/browse/JDK-8195590



H ... a summary of changes in the bug report would be nice.


Done.

I don't find the addition of reachability fences uninteresting
though. Why are they needed in those specific cases?


perhaps the standard should be for every method of a class with a 
finalizer to have reachabilityFence(this) at the end?


Imagine

newSingleThreadExecutor().execute(someTask);

Is is unlikely but possible for the "instant garbage" executor to be 
finalized while super.execute is in progress, causing failure with 
RejectedExecutionException because the delegatee is already shutdown.


Wow! DelegatedExecutorService doesn't even have a finalize() method that 
does a shutdown. So we have to put the reachabilityFence in it to 
prevent the delegatee from becoming unreachable due to the delegator 
becoming unreachable!


This whole notion that "this" can be unreachable whilst a method on it 
is still executing is just broken - it's an unusable programming model. 
Any code that does "new Foo().bar()" could fail unless Foo.bar() has a 
reachability fence in it. :( Sheesh!


Cheers,
David

RFR 8197462 : Inconsistent exception messages for invalid capturing group names

[Ivan Gerasimov]

Hello!

Capturing group name can be used in a regular expression in two 
contexts:  When introducing a group (?...) or when referring it 
\k.
If the name is invalid (i.e. does not start with a Latin letter, or 
contains wrong chars) then we may see different error messages, some of 
which look confusing.


Here are examples of the messages produced by the current JDK:
Unknown look-behind group near index 3
(?<>)
   ^
named capturing group is missing trailing '>' near index 4
\\k<>
^
Unknown look-behind group near index 4
(?<.>)
^
(named capturing group <.> does not exit near index 4
\\k<.>
^
named capturing group is missing trailing '>' near index 4
(?)
^
named capturing group is missing trailing '>' near index 4
\\k
^

In particular, this diversity is caused by that the internal 
Pattern.groupname() function lacks a check for the very first character 
of the name.
So that when \k is parsed, the first char is always accepted, no 
matter what it was.


Some cleanup was also done along the way.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8197462
WEBREV: http://cr.openjdk.java.net/~igerasim/8197462/00/webrev/

Thanks in advance!

--
With kind regards,
Ivan Gerasimov

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

[David Holmes]

Hi Robin,

On 9/02/2018 1:50 AM, Robin Westberg wrote:

Hi David,


On 8 Feb 2018, at 04:28, David Holmes  wrote:

Hi Robin,

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


Thanks, sorry about that..


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?


Right, almost all the runtime changes are made in order to try to figure out 
what the process exit code from the launcher will eventually be. For example, 
the launcher returns 1 if the main thread exited with an unhandled exception, 
but 0 otherwise. But I actually agree that this information is probably only of 
marginal use (it could always be captured from wherever Java is launched if 
someone really wants it), so it is perhaps not worth the trouble.


Yes and that particular example of launcher behaviour is an archaic 
throwback to C programming - where end of "main" means end of the 
program - IMHO. I don't think it is of interest - and certainly not 
worth jumping through so many hoops to make the info available.



Discussed it a bit with Erik Gahlin, and perhaps the best option here is to 
simply remove the status code field from the event, that would simplify things 
and make the code you mention above go away.


That sounds good to me. :)


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.


In this case I think it actually conveys useful information if you are trying 
to diagnose an unexpected shutdown. It should be useful to find the initial 
request of an orderly shutdown, even if something else happens during the 
shutdown sequence like a finalizer calling exit (in which case you could 
possibly end up with two shutdown events, but both may contain interesting 
information).


Yes that is useful. But I find the need to code things the way they are, 
unfortunate. But given current constraints, so be it.


Thanks,
David


Best regards,
Robin


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: RFR: jsr166 jdk integration 2018-02

[Martin Buchholz]

On Thu, Feb 8, 2018 at 7:39 PM, David Holmes 
wrote:

> On 9/02/2018 11:19 AM, Martin Buchholz wrote:
>
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integr
>> ation/overview.html
>>
>> 8190324: ThreadPoolExecutor should not specify a dependency on
>> finalization
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integr
>> ation/ThreadPoolExecutor-finalize/index.html
>> https://bugs.openjdk.java.net/browse/JDK-8190324
>>
>
> Looks okay.
>
> Another set of uninteresting changes, some suggested by intellij:
>>
>> 8195590: Miscellaneous changes imported from jsr166 CVS 2018-02
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integr
>> ation/miscellaneous/index.html
>> https://bugs.openjdk.java.net/browse/JDK-8195590
>>
>
> H ... a summary of changes in the bug report would be nice.
>
>
Done.


> I don't find the addition of reachability fences uninteresting though. Why
> are they needed in those specific cases?
>

perhaps the standard should be for every method of a class with a finalizer
to have reachabilityFence(this) at the end?

Imagine

newSingleThreadExecutor().execute(someTask);

Is is unlikely but possible for the "instant garbage" executor to be
finalized while super.execute is in progress, causing failure with
RejectedExecutionException because the delegatee is already shutdown.

Re: RFR: jsr166 jdk integration 2018-02

[David Holmes]

On 9/02/2018 11:19 AM, Martin Buchholz wrote:

http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html

8190324: ThreadPoolExecutor should not specify a dependency on finalization
http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ThreadPoolExecutor-finalize/index.html
https://bugs.openjdk.java.net/browse/JDK-8190324


Looks okay.


Another set of uninteresting changes, some suggested by intellij:

8195590: Miscellaneous changes imported from jsr166 CVS 2018-02
http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html
https://bugs.openjdk.java.net/browse/JDK-8195590


H ... a summary of changes in the bug report would be nice.

I don't find the addition of reachability fences uninteresting though. 
Why are they needed in those specific cases?


Thanks,
David

RFR: jsr166 jdk integration 2018-02

[Martin Buchholz]

http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/overview.html

8190324: ThreadPoolExecutor should not specify a dependency on finalization
http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/ThreadPoolExecutor-finalize/index.html
https://bugs.openjdk.java.net/browse/JDK-8190324

Another set of uninteresting changes, some suggested by intellij:

8195590: Miscellaneous changes imported from jsr166 CVS 2018-02
http://cr.openjdk.java.net/~martin/webrevs/jdk/jsr166-integration/miscellaneous/index.html
https://bugs.openjdk.java.net/browse/JDK-8195590

Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer

[Joe Wang]

Hi all,

The CSR for the enhancement is now approved. Thanks Joe!

The webrev has been updated accordingly. Please let me know if you have 
any further comment on the implementation.

JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/

Thanks,
Joe

On 2/2/2018 12:46 PM, Joe Wang wrote:

Thanks Jason. Will update that accordingly.

Best,
Joe

On 2/2/2018 11:22 AM, Jason Mehrens wrote:

Joe,

The identity check in CS.compare makes sense.  However, it won't be 
null hostile if we call CS.compare(null, null) and that doesn't seem 
right.

Usually when writing comparator classes I end up with:
===
if (Objects.requireNonNull(o1) == Objects.requireNonNull(o2)) {
 return 0;
}
===

Jason

From: core-libs-dev  on 
behalf of Joe Wang 

Sent: Friday, February 2, 2018 1:01 PM
To: core-libs-dev
Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, 
StringBuilder, and StringBuffer


Hi,

Thanks all for comments and suggestions. I've updated the webrev. Please
review.

JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/

Thanks,
Joe

On 1/31/2018 9:31 PM, Joe Wang wrote:

Hi Tagir,

Thanks for the comment. I will consider adding that to the javadoc
emphasizing that the comparison is performed from 0 to length() - 1 of
the two sequences.

Best,
Joe

On 1/29/18, 8:07 PM, Tagir Valeev wrote:

Hello!

An AbstractStringBuilder#compareTo implementation is wrong. You cannot
simply compare the whole byte array. Here's the test-case:

public class Test {
    public static void main(String[] args) {
  StringBuilder sb1 = new StringBuilder("test1");
  StringBuilder sb2 = new StringBuilder("test2");
  sb1.setLength(4);
  sb2.setLength(4);
  System.out.println(sb1.compareTo(sb2));
System.out.println(sb1.toString().compareTo(sb2.toString()));
    }
}

We truncated the stringbuilders making their content equal, so
sb1.toString().compareTo(sb2.toString()) is 0, but compareTo compares
the original content (before the truncation) as truncation, of course,
does not zero the truncated bytes, neither does it reallocate the
array (unless explicitly asked via trimToSize).

With best regards,
Tagir Valeev.


On Fri, Jan 26, 2018 at 10:00 AM, Joe Wang
wrote:

Hi,

Adding methods for comparing CharSequence, StringBuilder, and
StringBuffer.

The Comparable implementations for StringBuilder/Buffer are similar
to that
of String, allowing comparison operations between two
StringBuilders/Buffers, e.g.
aStringBuilder.compareTo(anotherStringBuilder).
For CharSequence however, refer to the comments in JIRA, a static
method
'compare' is added instead of implementing the Comparable interface.
This
'compare' method may take CharSequence implementations such as 
String,

StringBuilder and StringBuffer, making it possible to perform
comparison
among them. The previous example for example is equivalent to
CharSequence.compare(aStringBuilder, anotherStringBuilder).

Tests for java.base have been independent from each other. The new
tests are
therefore created to have no dependency on each other or sharing any
code.

JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/

Thanks,
Joe

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

[Joseph D. Darcy]

Since other people who work more closely in the area than me don't seem 
to find the wording confusing or misleading, I'll just close out the bug.


Thanks,

-Joe

On 2/8/2018 11:13 AM, Xueming Shen wrote:

On 2/8/18, 10:59 AM, joe darcy wrote:

Hello,

On 2/8/2018 3:53 AM, Alan Bateman wrote:

On 07/02/2018 22:12, 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.
I'm not sure that this is worth changing. You could replace 
"classes" with "API" and add a note to say that an implementation 
may use an more optimization representation but I don't think it's 
really needed.




In response to this feedback and others, how about:

 [...] The Java
  * platform uses the UTF-16 representation in {@code char} arrays and
- * in the {@code String} and {@code StringBuffer} classes. In
+ * presents a UTF-16 model in the string-related API.

IMO anyway, I think saying "uses a UTF-16 representation for String" 
is at best misleading with the current implementation since 8 != 16 
for the compressed representation is used for all Latin-1 strings.




Well, encoding/charset is the concept of a mapping between a character 
and a corresponding
code point value. We are still using the UTF16 encoding scheme to 
represent a character in
jvm. How to represent/store that UTF16 code point value in String 
class is an implementation
detail. A 16-bit for "char"  and a 1-byte for "latin1" (still in 
Unicode charset) + 2 byte for the

rest in String class.

As I said in my previous email. The mention of 8859-1 in the JEP might 
cause the confusion.
At early stage of the project we were really experimenting on using 
different "encoding", including
utf8. But the project ended up with staying with UTF-16, with a 
"customized/compressed" storage

mechanism to store the UTF16 codepoint value.

-Sherman

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

[Martin Buchholz]

Instead of saying something specific about char[], String etc you can be
more general and say something about the entire Java Platform.

"The Java Platform generally processes human language text represented as
sequences of {@code char} values in the UTF-16 encoding of Unicode"

But doing this well would involve a greater overhaul of the class doc.

Character will always be confusing because it is ambiguous about whether
referring to a Unicode character or a UTF-16 code unit.

On Thu, Feb 8, 2018 at 10:59 AM, joe darcy  wrote:

> Hello,
>
> On 2/8/2018 3:53 AM, Alan Bateman wrote:
>
>> On 07/02/2018 22:12, 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.
>>>
>> I'm not sure that this is worth changing. You could replace "classes"
>> with "API" and add a note to say that an implementation may use an more
>> optimization representation but I don't think it's really needed.
>>
>>
> In response to this feedback and others, how about:
>
>  [...] The Java
>   * platform uses the UTF-16 representation in {@code char} arrays and
> - * in the {@code String} and {@code StringBuffer} classes. In
> + * presents a UTF-16 model in the string-related API.
>
> IMO anyway, I think saying "uses a UTF-16 representation for String" is at
> best misleading with the current implementation since 8 != 16 for the
> compressed representation is used for all Latin-1 strings.
>
> Thanks,
>
> -Joe
>

Re: RFR 8178342: Missing @modules in jdk/jdk/nio/zipfs

[mandy chung]

+1 Mandy


On 2/8/18 10:40 AM, Alexandre (Shura) Iline wrote:

Hi.

Can you took a quick look on this fix. While adding missing @modules 
declaration in jdk/nio/zipfs/ZeroDate.java, I noticed that @modules declaration 
is not in the right location in other tests. Per the agreement it should come 
before any @run.

Bug: https://bugs.openjdk.java.net/browse/JDK-8178342
Webrev: http://cr.openjdk.java.net/~shurailine/8178342/webrev.00/

Thank you.

Shura

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

[Xueming Shen]

On 2/8/18, 10:59 AM, joe darcy wrote:

Hello,

On 2/8/2018 3:53 AM, Alan Bateman wrote:

On 07/02/2018 22:12, 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.
I'm not sure that this is worth changing. You could replace "classes" 
with "API" and add a note to say that an implementation may use an 
more optimization representation but I don't think it's really needed.




In response to this feedback and others, how about:

 [...] The Java
  * platform uses the UTF-16 representation in {@code char} arrays and
- * in the {@code String} and {@code StringBuffer} classes. In
+ * presents a UTF-16 model in the string-related API.

IMO anyway, I think saying "uses a UTF-16 representation for String" 
is at best misleading with the current implementation since 8 != 16 
for the compressed representation is used for all Latin-1 strings.




Well, encoding/charset is the concept of a mapping between a character 
and a corresponding
code point value. We are still using the UTF16 encoding scheme to 
represent a character in
jvm. How to represent/store that UTF16 code point value in String class 
is an implementation
detail. A 16-bit for "char"  and a 1-byte for "latin1" (still in Unicode 
charset) + 2 byte for the

rest in String class.

As I said in my previous email. The mention of 8859-1 in the JEP might 
cause the confusion.
At early stage of the project we were really experimenting on using 
different "encoding", including
utf8. But the project ended up with staying with UTF-16, with a 
"customized/compressed" storage

mechanism to store the UTF16 codepoint value.

-Sherman

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

[joe darcy]

Hello,

On 2/8/2018 3:53 AM, Alan Bateman wrote:

On 07/02/2018 22:12, 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.
I'm not sure that this is worth changing. You could replace "classes" 
with "API" and add a note to say that an implementation may use an 
more optimization representation but I don't think it's really needed.




In response to this feedback and others, how about:

 [...] The Java
  * platform uses the UTF-16 representation in {@code char} arrays and
- * in the {@code String} and {@code StringBuffer} classes. In
+ * presents a UTF-16 model in the string-related API.

IMO anyway, I think saying "uses a UTF-16 representation for String" is 
at best misleading with the current implementation since 8 != 16 for the 
compressed representation is used for all Latin-1 strings.


Thanks,

-Joe

RFR 8178342: Missing @modules in jdk/jdk/nio/zipfs

[Alexandre (Shura) Iline]

Hi.

Can you took a quick look on this fix. While adding missing @modules 
declaration in jdk/nio/zipfs/ZeroDate.java, I noticed that @modules declaration 
is not in the right location in other tests. Per the agreement it should come 
before any @run.

Bug: https://bugs.openjdk.java.net/browse/JDK-8178342
Webrev: http://cr.openjdk.java.net/~shurailine/8178342/webrev.00/

Thank you.

Shura

Re: RFR: 8194154 patch for crash at File.getCanonicalPath()

[yumin qi]

HI, Alan

  As in your email to RFR of 8194154 which now has a new fix in
canonicalize_md.c, switch back to discuss solution here again.
  The current fix is not in java, instead, I put it in C function. In the
function before the fix, it assumes no more "//" pattern in the string so
failed to get correct substrings with the case as the test case.
  The fix should not cause other problem since it does double check if
double or more slashes in sequential.

  The APIs indicates 'user.dir' etc system property should not be
rewritten, but did not prevent modifying them in practise.

Thanks
Yumin


On Tue, Jan 2, 2018 at 6:09 PM, Wenqian Pei 
wrote:

>
> Can you please review this patch?
>
> Thanks in advance!
>
> Wenqian Pei
> --发件人：Alan
> Bateman 发送时间：2018年1月3日(星期三) 01:30收件人：裴文谦(右席) <
> wenqian.pe...@alibaba-inc.com>; core-libs-dev  net>抄 送：陆传胜(传胜) ; 李三红(三红) <
> sanhong@alibaba-inc.com>主 题：Re: RFR: 8194154 patch for crash at
> File.getCanonicalPath()
> On 25/12/2017 09:40, Wenqian Pei wrote:
> > Hi:
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8194154
> >
> > We found that if user defines -Duser.dir like "/
> home/a/b/c/", jvm will crash at File.getCanonicalPath() in
> java process bootstrap (before invoking user's java code). The native
> implematation of canonicalize_md.c:collapsible(char *names)
> has problem in processing double '/', parameter 'names'
> need normalized before JNI_CALL.
> >
> user.dir is a "read-only" property and should never be changed on the
> command-line or in a running VM (it breaks APIs in other areas to have
> user.dir be different to the actual working directory). Someday we need
> to figure out how to enforce this.
>
> In the mean-time, the runtime shouldn't crash so I agree it
> should be fixed.
>
> -Alan.
>

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

[Paul Sandoz]

Hi Ben,

Thanks. I anticipated a performance hit but not necessarily a 10x. Without 
looking at the generated code of the benchmark method it is hard to be sure 
[*], but i believe the fence is interfering with loop unrolling and/or 
vectorization, the comparative differences between byte and int may be related 
to vectorization (for byte there may be less or limited support for 
vectorization).

How about we now try another experiment commenting out the @DontInline on the 
fence method and re-run the benchmarks. From Peter’s observations and 
Vladimir’s analysis we should be able to remove that, or even, contrary to what 
we initial expected when adding this feature, change to @ForceInline!

Thanks,
Paul.

[*] If you are running on linux you can use the excellent JMH perfasm feature 
to dump the hot parts of HotSpots generated code.

> On Feb 8, 2018, at 8:22 AM, Ben Walsh  wrote:
> 
> Hi Paul,
> 
> Following up with the requested loop and vectorization benchmarks ...
> 
> 
> (Do the vectorization benchmark results imply that the Hotspot compiler 
> has been unable to perform the vectorization optimisation due to the 
> presence of the reachabilityFence ?)
> 
> 
> ---
> 
> 
> Loop Benchmarking
>  
> 
> package org.sample;
> 
> import org.openjdk.jmh.annotations.Benchmark;
> import org.openjdk.jmh.annotations.Level;
> import org.openjdk.jmh.annotations.Param;
> import org.openjdk.jmh.annotations.Scope;
> import org.openjdk.jmh.annotations.Setup;
> import org.openjdk.jmh.annotations.State;
> 
> import java.nio.ByteBuffer;
> 
> @State(Scope.Benchmark)
> public class ByteBufferBenchmark {
> 
>@Param({"1", "10", "100", "1000", "1"})
>public int L;
> 
>@State(Scope.Benchmark)
>public static class ByteBufferContainer {
> 
>ByteBuffer bb;
> 
>@Setup(Level.Invocation)
>public void initByteBuffer() {
>bb = ByteBuffer.allocateDirect(1);
>}
> 
>ByteBuffer getByteBuffer() {
>return bb;
>}
>}
> 
>@Benchmark
>public ByteBuffer benchmark_byte_buffer_put(ByteBufferContainer bbC) {
> 
>ByteBuffer bb = bbC.getByteBuffer();
> 
>for (int i = 0; i < L; i++) {
>bb.put((byte)i);
>}
> 
>return bb;
>}
> 
> }
> 
> 
> Without Changes
> 
> Benchmark(L)   Mode  Cnt Score  
> Error  Units
> ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
> 29303145.752 ± 635979.750  ops/s
> ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
> 24260859.017 ± 528891.303  ops/s
> ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
> 8512366.637 ± 136615.070  ops/s
> ByteBufferBenchmark.benchmark_byte_buffer_put   1000  thrpt  200 
> 1323756.037 ±  21485.369  ops/s
> ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
> 145965.305 ±   1301.469  ops/s
> 
> 
> With Changes
> 
> Benchmark(L)   Mode  Cnt Score  
> Error  Units  Impact
> ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
> 28893540.122 ± 754554.747  ops/s  -1.398%
> ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
> 15317696.355 ± 231621.608  ops/s  -36.863%
> ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
> 2546599.578 ±  32136.873  ops/s  -70.084%
> ByteBufferBenchmark.benchmark_byte_buffer_put   1000  thrpt  200 
> 288832.514 ±   3854.522  ops/s  -78.181%
> ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 29747.386 
> ±214.831  ops/s  -79.620%
> 
> 
> ---
> 
> 
> Vectorization Benchmarking
> - 
> 
> package org.sample;
> 
> import org.openjdk.jmh.annotations.Benchmark;
> import org.openjdk.jmh.annotations.Level;
> import org.openjdk.jmh.annotations.Param;
> import org.openjdk.jmh.annotations.Scope;
> import org.openjdk.jmh.annotations.Setup;
> import org.openjdk.jmh.annotations.State;
> 
> import java.nio.ByteBuffer;
> 
> @State(Scope.Benchmark)
> public class ByteBufferBenchmark {
> 
>@Param({"1", "10", "100", "1000", "1"})
>public int L;
> 
>@State(Scope.Benchmark)
>public static class ByteBufferContainer {
> 
>ByteBuffer bb;
> 
>@Setup(Level.Invocation)
>public void initByteBuffer() {
>bb = ByteBuffer.allocateDirect(4 * 1);
> 
>for (int i = 0; i < 1; i++) {
>bb.putInt(i);
>}
>}
> 
>ByteBuffer getByteBuffer() {
>return bb;
>}
> 
>}
> 
>@Benchmark
>public int benchmark_byte_buffer_put(ByteBufferContainer bbC) {
> 
>ByteBuffer bb = bbC.getByteBuffer();
> 
>bb.position(0);
> 
> 

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

[Roger Riggs]

Hi Robin,

Saving useful information about failures during shutdown is useful.

Can the shutdown of JFR be put off? Is the use of shutdown hook still 
necessary?


Thanks, Roger


On 2/8/2018 10:50 AM, Robin Westberg wrote:

Hi David,


On 8 Feb 2018, at 04:28, David Holmes  wrote:

...

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?

Right, almost all the runtime changes are made in order to try to figure out 
what the process exit code from the launcher will eventually be. For example, 
the launcher returns 1 if the main thread exited with an unhandled exception, 
but 0 otherwise. But I actually agree that this information is probably only of 
marginal use (it could always be captured from wherever Java is launched if 
someone really wants it), so it is perhaps not worth the trouble.

Discussed it a bit with Erik Gahlin, and perhaps the best option here is to 
simply remove the status code field from the event, that would simplify things 
and make the code you mention above go away.


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.

In this case I think it actually conveys useful information if you are trying 
to diagnose an unexpected shutdown. It should be useful to find the initial 
request of an orderly shutdown, even if something else happens during the 
shutdown sequence like a finalizer calling exit (in which case you could 
possibly end up with two shutdown events, but both may contain interesting 
information).

Best regards,
Robin


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: [PATCH] Reduce Chance Of Mistakenly Early Backing Memory Cleanup

[Ben Walsh]

Hi Paul,

Following up with the requested loop and vectorization benchmarks ...


(Do the vectorization benchmark results imply that the Hotspot compiler 
has been unable to perform the vectorization optimisation due to the 
presence of the reachabilityFence ?)


---


Loop Benchmarking
 

package org.sample;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;

import java.nio.ByteBuffer;

@State(Scope.Benchmark)
public class ByteBufferBenchmark {

@Param({"1", "10", "100", "1000", "1"})
public int L;

@State(Scope.Benchmark)
public static class ByteBufferContainer {

ByteBuffer bb;

@Setup(Level.Invocation)
public void initByteBuffer() {
bb = ByteBuffer.allocateDirect(1);
}

ByteBuffer getByteBuffer() {
return bb;
}
}

@Benchmark
public ByteBuffer benchmark_byte_buffer_put(ByteBufferContainer bbC) {
 
ByteBuffer bb = bbC.getByteBuffer();

for (int i = 0; i < L; i++) {
bb.put((byte)i);
}

return bb;
}

}


Without Changes

Benchmark(L)   Mode  Cnt Score  
Error  Units
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
29303145.752 ± 635979.750  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
24260859.017 ± 528891.303  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
8512366.637 ± 136615.070  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put   1000  thrpt  200 
1323756.037 ±  21485.369  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
145965.305 ±   1301.469  ops/s


With Changes

Benchmark(L)   Mode  Cnt Score  
Error  Units  Impact
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
28893540.122 ± 754554.747  ops/s  -1.398%
ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
15317696.355 ± 231621.608  ops/s  -36.863%
ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
2546599.578 ±  32136.873  ops/s  -70.084%
ByteBufferBenchmark.benchmark_byte_buffer_put   1000  thrpt  200 
288832.514 ±   3854.522  ops/s  -78.181%
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 29747.386 
±214.831  ops/s  -79.620%


---


Vectorization Benchmarking
- 

package org.sample;

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Param;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;

import java.nio.ByteBuffer;

@State(Scope.Benchmark)
public class ByteBufferBenchmark {

@Param({"1", "10", "100", "1000", "1"})
public int L;

@State(Scope.Benchmark)
public static class ByteBufferContainer {

ByteBuffer bb;

@Setup(Level.Invocation)
public void initByteBuffer() {
bb = ByteBuffer.allocateDirect(4 * 1);
 
for (int i = 0; i < 1; i++) {
bb.putInt(i);
}
}

ByteBuffer getByteBuffer() {
return bb;
}

}

@Benchmark
public int benchmark_byte_buffer_put(ByteBufferContainer bbC) {
 
ByteBuffer bb = bbC.getByteBuffer();

bb.position(0);

int sum = 0;

for (int i = 0; i < L; i++) {
sum += bb.getInt();
}

return sum;

}

}


Without Changes

Benchmark(L)   Mode  Cnt Score  
Error  Units
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
29677205.748 ± 544721.142  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
18219951.454 ± 320724.793  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
7767650.826 ± 121798.910  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put   1000  thrpt  200 
1646075.010 ±   9804.499  ops/s
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
183489.418 ±   1355.967  ops/s


With Changes

Benchmark(L)   Mode  Cnt Score  
Error  Units  Impact
ByteBufferBenchmark.benchmark_byte_buffer_put  1  thrpt  200 
15230086.695 ± 390174.190  ops/s  -48.681%
ByteBufferBenchmark.benchmark_byte_buffer_put 10  thrpt  200 
8126310.728 ± 123661.342  ops/s  -55.399%
ByteBufferBenchmark.benchmark_byte_buffer_put100  thrpt  200 
1582699.233 ±   7278.744  ops/s  -79.624%

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

[Robin Westberg]

Hi David,

> On 8 Feb 2018, at 04:28, David Holmes  wrote:
> 
> Hi Robin,
> 
> Adding in hotspot-runtime-dev as all the hotspot changes belong to runtime.

Thanks, sorry about that..

> 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?

Right, almost all the runtime changes are made in order to try to figure out 
what the process exit code from the launcher will eventually be. For example, 
the launcher returns 1 if the main thread exited with an unhandled exception, 
but 0 otherwise. But I actually agree that this information is probably only of 
marginal use (it could always be captured from wherever Java is launched if 
someone really wants it), so it is perhaps not worth the trouble.

Discussed it a bit with Erik Gahlin, and perhaps the best option here is to 
simply remove the status code field from the event, that would simplify things 
and make the code you mention above go away. 

> 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.

In this case I think it actually conveys useful information if you are trying 
to diagnose an unexpected shutdown. It should be useful to find the initial 
request of an orderly shutdown, even if something else happens during the 
shutdown sequence like a finalizer calling exit (in which case you could 
possibly end up with two shutdown events, but both may contain interesting 
information).  

Best regards,
Robin

> 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: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

[Roger Riggs]

Looks fine  +1

Thanks, Roger


On 2/8/2018 10:13 AM, Jason Mehrens wrote:

Aleksei,

Looks good to me.

Jason

From: Aleks Efimov 
Sent: Wednesday, February 7, 2018 7:24 PM
To: Roger Riggs; Jason Mehrens
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not 
correctly set Exception

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 

Re: RFR : 8196854 :TestFlushableGZIPOutputStream failing with IndexOutOfBoundsException

[Xueming Shen]

+1

On Feb 7, 2018, at 10:03 AM, Seán Coffey  wrote:

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 [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

[Jason Mehrens]

Aleksei,

Looks good to me.

Jason

From: Aleks Efimov 
Sent: Wednesday, February 7, 2018 7:24 PM
To: Roger Riggs; Jason Mehrens
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not 
correctly set Exception

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

 

Re: RFR : 8196854 :TestFlushableGZIPOutputStream failing with IndexOutOfBoundsException

[Roger Riggs]

Hi Sean,

Looks fine,

Roger


On 2/7/2018 1:03 PM, Seán Coffey wrote:
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.

Re: RFR 8190378: Java EE and CORBA modules removal

[Lance Andersen]

> On Feb 8, 2018, at 3:04 AM, Alan Bateman  wrote:
> 
> On 07/02/2018 16: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/ 
>> 
>> 
> 800 KLOC deleted, wonderful!
> 
> The update to technology-summary.html page means its html title no longer 
> matches the contents. We should probably change it to "JCP Technologies in 
> JDK 11" for now.

I updated the webrev. Thanks for catching that (btw we missed this for JDK 10)
> 
> The removal of test cases from the tests in tools/launcher/modules removes 
> most of the test coverage for the upgrade module path. We'll need to replace 
> these sub-tests. Can you create an issue to track that?

I can do that 
> 
> Everything else looks good and it's okay to track residual issues with other 
> JIRA issues. I think the important thing is to get this monster patch into 
> JDK builds soon so that libraries and the eco system can start to adjust.

Thank you Alan for the review

Best
Lance
> 
> -Alan

 
  

 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: JDK 11 RFR of JDK-8196995: java.lang.Character should not state UTF-16 encoding is used for strings

[Alan Bateman]

On 07/02/2018 22:12, 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.
I'm not sure that this is worth changing. You could replace "classes" 
with "API" and add a note to say that an implementation may use an more 
optimization representation but I don't think it's really needed.


-Alan

Re: RFR 8190378: Java EE and CORBA modules removal

[Alan Bateman]

On 07/02/2018 16: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/ 



800 KLOC deleted, wonderful!

The update to technology-summary.html page means its html title no 
longer matches the contents. We should probably change it to "JCP 
Technologies in JDK 11" for now.


The removal of test cases from the tests in tools/launcher/modules 
removes most of the test coverage for the upgrade module path. We'll 
need to replace these sub-tests. Can you create an issue to track that?


Everything else looks good and it's okay to track residual issues with 
other JIRA issues. I think the important thing is to get this monster 
patch into JDK builds soon so that libraries and the eco system can 
start to adjust.


-Alan