Re: [PATCH] Support for UTC Zones with TimeZone.getTimeZone()

[Mohamed Naufal]

Hi Naoto,

Thank you for the feedback, I'll send an updated patch shortly.

Best regards,
Naufal

On 21 December 2017 at 05:19, Naoto Sato  wrote:

> Hi Naufal,
>
> Thank you for the patch. It does seem to be a bug. However, TimeZone class
> defines CustomID as "GMT+(offset)", so I'd rather fix it in
> GregorianCalendar.from() to recognize those "UTC+offset" zones.
>
> Naoto
>
>
> On 12/16/17 2:27 AM, Mohamed Naufal wrote:
>
>> Hi,
>>
>> I noticed that with the following data:
>>
>> LocalDateTime ldt = LocalDateTime.parse("2017-01-01T00:00:00");
>> ZonedDateTime dt1 = ZonedDateTime.of(ldt, ZoneId.of("GMT+10"));
>> ZonedDateTime dt2 = ZonedDateTime.of(ldt, ZoneId.of("UTC+10"));
>>
>> dt1.equals(dt2) returns true as expected, but with:
>>
>> GregorianCalendar gc1 = GregorianCalendar.from(dt1);
>> GregorianCalendar gc2 = GregorianCalendar.from(dt2);
>>
>> gc1.equals(gc2) returns false.
>>
>> Looking at the code, I see when a GregorianCalendar is being constructed,
>> TimeZone.getTimeZone() gets called, but it doesn't recognise UTC
>> time-zones
>> with offsets, and falls back to GMT(+0), whereas ZoneId treats GMT and UTC
>> based zones equivalently.
>>
>> PFA a patch to fix this.
>>
>> Thank you,
>> Naufal
>>
>>

Re: RFR(s): 8140281 add no-arg Optional.orElseThrow() as preferred alternative to get()

[Stuart Marks]

On 12/18/17 2:31 AM, Tagir Valeev wrote:

I think that both methods could co-exist with slightly different
semantics (even though they implementation is identical):
...
Think of get() as
an assertion-like method: if get() throws, then it's a bug in the code
(most likely right in this method).
...
If orElseThrow() throws, then
it's an acceptable situation (e.g. we are inside the library method
and it documents that NoSuchElementException could be thrown if some
precondition is not met).


Right, so the semantic difference is between an assertion check and a 
precondition check. I agree that there is a difference, but get() doesn't really 
mean "assertion check" to me. The orElseThrow() method could be used for both, 
without much loss. I think it's because we experts are used to get(), and it's 
all we had for both cases in JDK 8 and 9, that there is discomfort with the 
prospect of reducing the status of get().



However I don't like issuing a warning if
get() is used like in the code above. And if get() will be deprecated,
then we would need to issue warning on every get() usage. Thus I'm
against the deprecation.


Yes, better warnings control would clearly be necessary before we would proceed 
with deprecation.



I believe that for any kind of API you can find bad code which abuses
it regardless on how many years passed since the API was released.
That's not always the fault of API creators.


It's certainly possible to write bad code in any language, using any API, 
certainly right after it's been introduced. As time goes on, people learn more, 
articles are written and conference talks presented, etc., and good usage 
patterns tend to emerge.


The situation with get() seems qualitatively different. I consistently and 
repeatedly see it misused, and it doesn't seem to be getting any better. When 
this occurs, it indicates that there's something wrong with the API.


s'marks

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[David Holmes]

Hi Chris,

Adding in hotspot-runtime-dev now that you have included the VM side of 
the cleanup. What repo are you planning on pushing this to?


On 21/12/2017 9:45 AM, Chris Hegarty wrote:



On 20 Dec 2017, at 19:21, mandy chung  wrote:

The native side and hotspot side should also be cleaned up.


Thanks Mandy, I was about call that out too :)


Good call. I think the following is probably as far as we want to
go. Maybe a follow-on issue could be filed if deeper VM clean
up is needed?

http://cr.openjdk.java.net/~chegar/8179424/webrev.01/ 



src/hotspot/share/include/jvm.h

Can you fix an existing typo please:

!  * error if it is not marked propertly.

propertly -> properly

Also you seem to have missed this test reference:

./langtools/tools/jdeps/jdkinternals/src/p/Main.java:Class 
caller = Reflection.getCallerClass(2);


Otherwise all changes seem fine.

Thanks,
David


-Chris.

P.S. jdk and hotspot tests are still running...

Re: [10] RFR 8193856 takeWhile produces incorrect result with elements produced by flatMap

[Stuart Marks]

On 12/20/17 1:28 PM, Paul Sandoz wrote:

Please review this fix for a bug in the stream takeWhile operation:

   
http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8193856-takeWhile-incorrect-results/webrev/
 


The flatMap operation is currently aggressive and does not detect if a 
downstream operation may or has cancelled processing, and will push all of it’s 
elements downstream. Short-circuiting operations should be guarded against such 
behaviour but unfortunately takeWhile was not guarded.


Hi Paul,

Change looks fine. Good to get this one into JDK 10.

Thanks,

s'marks

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[mandy chung]

On 12/20/17 3:45 PM, Chris Hegarty wrote:


On 20 Dec 2017, at 19:21, mandy chung > wrote:


The native side and hotspot side should also be cleaned up.


Good call. I think the following is probably as far as we want to
go. Maybe a follow-on issue could be filed if deeper VM clean
up is needed?

http://cr.openjdk.java.net/~chegar/8179424/webrev.01/ 





Looks good.  This hotspot change is adequate for this fix.

Mandy

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Peter Levart]

Hi Brian,

Brian Burkhalter je 20. 12. 2017 ob 22:54 napisal:

Hi Peter,

On Dec 20, 2017, at 3:45 AM, Peter Levart > wrote:



    if (result == null) {
result = copy;
    } else {
bufs = new ArrayList<>(8); // <— ?
bufs.add(result);
bufs.add(copy);
    }


I am probably missing something here, but if the do-while loop 
iterates three or more times with nread > 0 each time won’t data be 
lost? Should this not instead be:


                if (result == null) {
    result = copy;
    } else {
                    if (bufs == null) {
                        bufs = new ArrayList<>(8);
                        bufs.add(result);
                    }
                    bufs.add(copy);
    }

Thanks,

Brian


Yes, of course. Good catch. Next time I should try running the code 
before proposing it...


webrev.03 looks good.

Regards, Peter

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[Chris Hegarty]

> On 20 Dec 2017, at 19:21, mandy chung  wrote:
> 
> The native side and hotspot side should also be cleaned up.

Good call. I think the following is probably as far as we want to 
go. Maybe a follow-on issue could be filed if deeper VM clean
up is needed?

http://cr.openjdk.java.net/~chegar/8179424/webrev.01/ 


-Chris.

P.S. jdk and hotspot tests are still running...

Re: [10] RFR 8193856 takeWhile produces incorrect result with elements produced by flatMap

[Paul Sandoz]

Hi Jonathan,

> On 20 Dec 2017, at 14:47, Jonathan Bluett-Duncan  
> wrote:
> 
> Hi Paul,
> 
> It seems that some clever Googler managed to find a workaround for aggressive 
> `flatMap` operations in the form of a so-called `MoreStreams.flatten` 
> operation, implemented in a side project called google/mug.

Thanks. MoreStreams.flatten “lowers” a stream of streams to a spliterator of 
spliterators which in turn is input as a new stream source, enabling a 
cancellation check + tryAdvance for any following short-circuit or terminal ops.

I have a solution in the works for flatMap based on sorted operation (which has 
to buffer all elements, sort ‘em, then push ‘em all downstream).

Paul.

> I'm sharing the javadoc 
> 
>  and GitHub project homepage  with you and the 
> rest of the mailing list in the hope that they prove to be useful.
> 
> Cheers,
> Jonathan
> 
> On 20 December 2017 at 21:28, Paul Sandoz  > wrote:
> Hi,
> 
> Please review this fix for a bug in the stream takeWhile operation:
> 
>   
> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8193856-takeWhile-incorrect-results/webrev/
>  
> 
>  
>   
> >
> 
> The flatMap operation is currently aggressive and does not detect if a 
> downstream operation may or has cancelled processing, and will push all of 
> it’s elements downstream. Short-circuiting operations should be guarded 
> against such behaviour but unfortunately takeWhile was not guarded.
> 
> —
> 
> Separately i plan to figure out a way to ensure flatMap operations become 
> less aggressive if there are short-circuiting downstream operations. This may 
> increase efficiency and also allow support for flat map results that are 
> infinite.
> 
> Paul.
> 

Re: [10] RFR 8193856 takeWhile produces incorrect result with elements produced by flatMap

[Jonathan Bluett-Duncan]

Hi Paul,

It seems that some clever Googler managed to find a workaround for
aggressive `flatMap` operations in the form of a so-called
`MoreStreams.flatten` operation, implemented in a side project called
google/mug. I'm sharing the javadoc

and
GitHub project homepage  with you and the
rest of the mailing list in the hope that they prove to be useful.

Cheers,
Jonathan

On 20 December 2017 at 21:28, Paul Sandoz  wrote:

> Hi,
>
> Please review this fix for a bug in the stream takeWhile operation:
>
>   http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8193856-
> takeWhile-incorrect-results/webrev/  psandoz/jdk10/JDK-8193856-takeWhile-incorrect-results/webrev/>
>
> The flatMap operation is currently aggressive and does not detect if a
> downstream operation may or has cancelled processing, and will push all of
> it’s elements downstream. Short-circuiting operations should be guarded
> against such behaviour but unfortunately takeWhile was not guarded.
>
> —
>
> Separately i plan to figure out a way to ensure flatMap operations become
> less aggressive if there are short-circuiting downstream operations. This
> may increase efficiency and also allow support for flat map results that
> are infinite.
>
> Paul.
>

RFR (JDK10/JAXP Doc-only) 8193568 : @LastModified tag in license header

[Joe Wang]

Hi,

Refer to the previous change for JDK-8191938[1], the @LastModified tag 
needs to be moved to the class description section. This change affects 
341 files and may be hard to look through. But it's done with a script 
that simply moves the tag from the top header to the class description 
section, the majority therefore are two-line changes ( e.g "2 lines 
changed"). If the original description section didn't have any tags, an 
additional comment-line is added to separate the tag with the 
description ( look for "3 lines changed"). If a class didn't have a 
class description at all, a new one is added (therefore "4 lines changed").


Please review:
JBS: https://bugs.openjdk.java.net/browse/JDK-8193568
webrev: http://cr.openjdk.java.net/~joehw/jdk10/8193568/webrev/


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

Thanks,
Joe

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Brian Burkhalter]

On Dec 20, 2017, at 11:52 AM, Paul Sandoz  wrote:

> I was a little lassiaz-faire given that 8K bytes were anyway being allocated 
> upfront. Peter’s changes look good.
> 
> Brian, i would double check the tests to make sure the various code paths are 
> tested.


http://cr.openjdk.java.net/~bpb/8193832/webrev.03/

The patch is updated to:

* use Peter’s approach to avoid allocating an ArrayList when length <= 
DEFAULT_BUFFER_SIZE;
* use the default ArrayList constructor instead of that with a specific initial 
capacity;
* update the test to ensure that lengths which require three buffers are 
covered.

Thanks,

Brian

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Brian Burkhalter]

Hi Peter,

On Dec 20, 2017, at 3:45 AM, Peter Levart  wrote:

> if (result == null) {
> result = copy;
> } else {
> bufs = new ArrayList<>(8); // <— ?
> bufs.add(result);
> bufs.add(copy);
> }

I am probably missing something here, but if the do-while loop iterates three 
or more times with nread > 0 each time won’t data be lost? Should this not 
instead be:

if (result == null) {
result = copy;
} else {
if (bufs == null) {
bufs = new ArrayList<>(8);
bufs.add(result);
}
bufs.add(copy);
}

Thanks,

Brian

[10] RFR 8193856 takeWhile produces incorrect result with elements produced by flatMap

[Paul Sandoz]

Hi,

Please review this fix for a bug in the stream takeWhile operation:

  
http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8193856-takeWhile-incorrect-results/webrev/
 


The flatMap operation is currently aggressive and does not detect if a 
downstream operation may or has cancelled processing, and will push all of it’s 
elements downstream. Short-circuiting operations should be guarded against such 
behaviour but unfortunately takeWhile was not guarded.

—

Separately i plan to figure out a way to ensure flatMap operations become less 
aggressive if there are short-circuiting downstream operations. This may 
increase efficiency and also allow support for flat map results that are 
infinite.

Paul.

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Paul Sandoz]

> On 20 Dec 2017, at 11:04, John Rose  wrote:
> 
> On Dec 20, 2017, at 3:45 AM, Peter Levart  > wrote:
>> 
>> allocation of ArrayList could be avoided. Imagine a use case where lots of 
>> small files are read into byte[] arrays
> 
> +1 I was going to write a similar suggestion; thanks for sending it out.
> 

I was a little lassiaz-faire given that 8K bytes were anyway being allocated 
upfront. Peter’s changes look good.

Brian, i would double check the tests to make sure the various code paths are 
tested.

Paul.

> These sorts of things often need to be designed to perform well at all
> scales, not just large scales.
> 
> The new ArrayList(8) is dead weight if the data fits in the buffer.
> So it's bad for scale < buffer length.
> 
> The earlier new ArrayList(128) is a noticeable overhead if the data
> fits in two buffers.  So it's not so good for scale = M * (buffer length)
> where M is about 2.
> 
> For a large scale result (M > 10), the footprint difference between
> ArrayList(N) for various N is a second-order fraction.
> 
> — John
> 
> P.S. Road not taken:  Instead of new ArrayList(8) you could use
> the default ArrayList constructor, and allocate it unconditionally.
> It has a very low overhead if the ArrayList remains empty.  Using
> that constructor could motivate you to simplify the code to use
> the ArrayList unconditionally, since that would be just a single
> heap node if it is not used to gather multiple results.  But I like
> the "null" formulation despite its complexity.  So I'd probably
> keep it the way Peter wrote it.
> 

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[Daniel Fuchs]

Looks good Chris!

-- daniel

On 20/12/2017 18:42, Chris Hegarty wrote:

In JDK 9 sun.reflect.Reflection.getCallerClass, and its
jdk.internal counterpart, where both deprecated-for-removal
( to give prior warning of the unsupported private API's
intended removal ). The supported replacement, since
Java SE 9, is java.lang.StackWalker.

http://cr.openjdk.java.net/~chegar/8179424/webrev/

-Chris.

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[mandy chung]

The native side and hotspot side should also be cleaned up.

Mandy

On 12/20/17 11:20 AM, mandy chung wrote:



On 12/20/17 10:42 AM, Chris Hegarty wrote:

In JDK 9 sun.reflect.Reflection.getCallerClass, and its
jdk.internal counterpart, where both deprecated-for-removal
( to give prior warning of the unsupported private API's
intended removal ). The supported replacement, since
Java SE 9, is java.lang.StackWalker.

http://cr.openjdk.java.net/~chegar/8179424/webrev/



+1.   Happy to see this removed.

thanks
Mandy

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[mandy chung]

On 12/20/17 10:42 AM, Chris Hegarty wrote:

In JDK 9 sun.reflect.Reflection.getCallerClass, and its
jdk.internal counterpart, where both deprecated-for-removal
( to give prior warning of the unsupported private API's
intended removal ). The supported replacement, since
Java SE 9, is java.lang.StackWalker.

http://cr.openjdk.java.net/~chegar/8179424/webrev/



+1.   Happy to see this removed.

thanks
Mandy

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[Roger Riggs]

+1

On 12/20/2017 1:53 PM, Lance Andersen wrote:

looks fine Chris

Best
Lance

On Dec 20, 2017, at 1:42 PM, Chris Hegarty  wrote:

http://cr.openjdk.java.net/~chegar/8179424/webrev/ 


  
   

  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 8193832: Performance of InputStream.readAllBytes() could be improved

[John Rose]

On Dec 20, 2017, at 3:45 AM, Peter Levart  wrote:
> 
> allocation of ArrayList could be avoided. Imagine a use case where lots of 
> small files are read into byte[] arrays

+1 I was going to write a similar suggestion; thanks for sending it out.

These sorts of things often need to be designed to perform well at all
scales, not just large scales.

The new ArrayList(8) is dead weight if the data fits in the buffer.
So it's bad for scale < buffer length.

The earlier new ArrayList(128) is a noticeable overhead if the data
fits in two buffers.  So it's not so good for scale = M * (buffer length)
where M is about 2.

For a large scale result (M > 10), the footprint difference between
ArrayList(N) for various N is a second-order fraction.

— John

P.S. Road not taken:  Instead of new ArrayList(8) you could use
the default ArrayList constructor, and allocate it unconditionally.
It has a very low overhead if the ArrayList remains empty.  Using
that constructor could motivate you to simplify the code to use
the ArrayList unconditionally, since that would be just a single
heap node if it is not used to gather multiple results.  But I like
the "null" formulation despite its complexity.  So I'd probably
keep it the way Peter wrote it.

Re: RFR [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[Lance Andersen]

looks fine Chris

Best
Lance
> On Dec 20, 2017, at 1:42 PM, Chris Hegarty  wrote:
> 
> http://cr.openjdk.java.net/~chegar/8179424/webrev/ 
> 
 
  

 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 [11] JDK-8179424: Remove terminally deprecated sun.reflect.Reflection.getCallerClass

[Chris Hegarty]

In JDK 9 sun.reflect.Reflection.getCallerClass, and its
jdk.internal counterpart, where both deprecated-for-removal
( to give prior warning of the unsupported private API's
intended removal ). The supported replacement, since
Java SE 9, is java.lang.StackWalker.

http://cr.openjdk.java.net/~chegar/8179424/webrev/

-Chris.

Re: [PATCH] Support for UTC Zones with TimeZone.getTimeZone()

[Naoto Sato]

Hi Naufal,

Thank you for the patch. It does seem to be a bug. However, TimeZone 
class defines CustomID as "GMT+(offset)", so I'd rather fix it in 
GregorianCalendar.from() to recognize those "UTC+offset" zones.


Naoto

On 12/16/17 2:27 AM, Mohamed Naufal wrote:

Hi,

I noticed that with the following data:

LocalDateTime ldt = LocalDateTime.parse("2017-01-01T00:00:00");
ZonedDateTime dt1 = ZonedDateTime.of(ldt, ZoneId.of("GMT+10"));
ZonedDateTime dt2 = ZonedDateTime.of(ldt, ZoneId.of("UTC+10"));

dt1.equals(dt2) returns true as expected, but with:

GregorianCalendar gc1 = GregorianCalendar.from(dt1);
GregorianCalendar gc2 = GregorianCalendar.from(dt2);

gc1.equals(gc2) returns false.

Looking at the code, I see when a GregorianCalendar is being constructed,
TimeZone.getTimeZone() gets called, but it doesn't recognise UTC time-zones
with offsets, and falls back to GMT(+0), whereas ZoneId treats GMT and UTC
based zones equivalently.

PFA a patch to fix this.

Thank you,
Naufal

Re: [11] RFR of 8146656: Wrong Months Array for DateFormatSymbols

[joe darcy]

Hi Rachna,

I think the revised version with the @implSpec tag switch is acceptable, 
but also think providing more text to describe this situation would be 
helpful to programmers unaware of a 13 month possibility.


Cheers,

-Joe


On 12/19/2017 2:08 AM, Rachna Goel wrote:


Hi Joe,

Thanks for the comments.

I have updated the CSR to have @implSpec in place of @implNote.

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

Regarding  "An array with either 12 or 13 elements will be returned 
depending on whether or {@link Calendar.UNDECIMBER} is supported." ,  
I would like to go with existing statement as this method always 
returns 13 elements where the 13th element may be empty string or may 
contain Calendar.UNDECIMBER, depending upon whether its supported by 
the Calendar instance.


kindly suggest whether this looks fine!

Thanks,
Rachna


On 19/12/17 2:55 PM, joe darcy wrote:

Hi Rachna,

On 12/19/2017 1:13 AM, Rachna Goel wrote:


Hello Joe,

Thanks for the review.

Reason I added @implNote is that it's the case for the default 
implementation. Not added as a part of spec, as some implementation 
can just return 12 element array for same methods through the 
"java.text.spi.DateFormatSymbolsProvider" SPI.





That is precisely the sort of situation the @implSpec tag is intended 
for. It allows the specification to say DateFormatSymbols must behave 
this way while allowing subclasses to behave differently.


Perhaps some general text can be added as normal specification, 
something like


"An array with either 12 or 13 elements will be returned depending on 
whether or {@link Calendar.UNDECIMBER} is supported."


paired with

@implSpec This method returns 13 elements since @link 
Calendar.UNDECIMBER} is supported.


HTH,

-Joe



--
Thanks,
Rachna

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Brian Burkhalter]

Hi Peter,

On Dec 20, 2017, at 8:04 AM, Peter Levart  wrote:

>>>  found another improvement. If you are reading from files, there's no 
>>> difference. But if you read from say socket(s), there may be short reads 
>>> (read() returning 0).
>> InputStreams are blocking so if someone creates an InputStream over a socket 
>> configured non-blocking then they have to emulate blocking behavior. So 
>> assuming a non-zero byte array, then read should return a positive value or 
>> -1.
>> 
>> -Alan.
> 
> You are right Alan. I don't know why I assumed that 0 is a valid return value 
> (for non-empty array). So my last suggestion is unnecessary. Each buf will be 
> filled to the top before inner loop exits or the stream will be at EOF.

Thanks for the suggestions. I’ll take a look at the first two later today.

Brian

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Peter Levart]

On 12/20/2017 03:09 PM, Alan Bateman wrote:



On 20/12/2017 12:40, Peter Levart wrote:

Hi Brian,

I found another improvement. If you are reading from files, there's 
no difference. But if you read from say socket(s), there may be short 
reads (read() returning 0).
InputStreams are blocking so if someone creates an InputStream over a 
socket configured non-blocking then they have to emulate blocking 
behavior. So assuming a non-zero byte array, then read should return a 
positive value or -1.


-Alan.


You are right Alan. I don't know why I assumed that 0 is a valid return 
value (for non-empty array). So my last suggestion is unnecessary. Each 
buf will be filled to the top before inner loop exits or the stream will 
be at EOF.


Regards, Peter

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Chris Hegarty]

On 19/12/17 23:22, Brian Burkhalter wrote:

..
Updated version here:

http://cr.openjdk.java.net/~bpb/8193832/webrev.02/


Looks good to me. This is a nice improvement on the
original implementation that I added in 9. Thanks.

-Chris.

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Alan Bateman]

On 20/12/2017 12:40, Peter Levart wrote:

Hi Brian,

I found another improvement. If you are reading from files, there's no 
difference. But if you read from say socket(s), there may be short 
reads (read() returning 0).
InputStreams are blocking so if someone creates an InputStream over a 
socket configured non-blocking then they have to emulate blocking 
behavior. So assuming a non-zero byte array, then read should return a 
positive value or -1.


-Alan.

RE: RFR: JDK8u Backport of 8153955: increase java.util.logging.FileHandler MAX_LOCKS limit

[Ramanand Patil]

Thank you Daniel and Sean for your reviews.

Regards,
Ramanand.

> -Original Message-
> From: Daniel Fuchs
> Sent: Wednesday, December 20, 2017 2:43 PM
> To: Ramanand Patil ; core-libs-dev  d...@openjdk.java.net>
> Subject: Re: RFR: JDK8u Backport of 8153955: increase
> java.util.logging.FileHandler MAX_LOCKS limit
> 
> Hi Ramanand,
> 
> On 18/12/2017 11:41, Ramanand Patil wrote:
> > Hi all,
> > Please review the fix for JDK8u Backport of
> https://bugs.openjdk.java.net/browse/JDK-8153955
> > Backport Bug: https://bugs.openjdk.java.net/browse/JDK-8161266
> > Webrev: http://cr.openjdk.java.net/~rpatil/8161266/webrev.00/
> 
> Looks good to me as well.
> 
> best regards,
> 
> -- daniel
> 
> >
> > Summary(also added to backport bug description):
> > The fix from JDK9 cannot be backported as is into the jdk8u and earlier
> update releases, since it contains JDK API spec changes.
> > In JDK9 the fix is added by altering the FileHandler spec, which introduces 
> > a
> new configurable property "java.util.logging.FileHandler.maxLocks" to
> java.util.logging.FileHandler, defined in .../conf/logging.properties.
> > The solution proposed for update releases is:
> > To introduce an internal JDK implementation specific property-
> "jdk.internal.FileHandlerLogging.maxLocks" which will be used to
> control/override FileHandler's MAX_LOCKS value. The default value of the
> maxLocks (100) will be retained if this new System property is not set. The
> new property is read only once during FileHandler class initialization.
> >
> >
> > Regards,
> > Ramanand.
> >
> 

Re: [11] RFR (XS) 8193838: Update jtreg requiredVersion to 4.2 b11 for JDK 11 and 12 support

[David Holmes]

Thanks Serguei!

David

On 20/12/2017 8:41 PM, serguei.spit...@oracle.com wrote:

David,

It looks good.

Thanks,
Serguei


On 12/20/17 02:22, David Holmes wrote:
Before we can update to JDK version 11, jtreg needs to be updated to 
recognize that release value - which is happening in jtreg 4.2 b11. So 
we then need to ensure that jtreg 4.2 b11 is used by updating the 
requiredVersion in each of the TEST.ROOT files, and in the jib 
configuiration.


bug: https://bugs.openjdk.java.net/browse/JDK-8193838
webrev: http://cr.openjdk.java.net/~dholmes/8193838/webrev/

Testing - TBD once b11 is promoted
 - local build using jib
 - hs/jdk tier1 and tier2

Thanks,
David

Re: [11] RFR (XS) 8193838: Update jtreg requiredVersion to 4.2 b11 for JDK 11 and 12 support

[David Holmes]

Thanks Alan.

David

On 20/12/2017 8:32 PM, Alan Bateman wrote:



On 20/12/2017 10:22, David Holmes wrote:
Before we can update to JDK version 11, jtreg needs to be updated to 
recognize that release value - which is happening in jtreg 4.2 b11. So 
we then need to ensure that jtreg 4.2 b11 is used by updating the 
requiredVersion in each of the TEST.ROOT files, and in the jib 
configuiration.


bug: https://bugs.openjdk.java.net/browse/JDK-8193838
webrev: http://cr.openjdk.java.net/~dholmes/8193838/webrev/

Looks fine.

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Peter Levart]

Hi Brian,

I found another improvement. If you are reading from files, there's no 
difference. But if you read from say socket(s), there may be short reads 
(read() returning 0). Your current code bails out from inner loop when 
this happens and consequently does not fill the buf up to the top when 
it could fill it if it retried the inner loop. This is a variant where 
inner loop guarantees that either buf is filled to the top or stream is 
at EOF:


    public byte[] readAllBytes() throws IOException {
    List bufs = null;
    byte[] result = null;
    byte[] buf = new byte[DEFAULT_BUFFER_SIZE];
    int total = 0;
    int remaining; // # of bytes remaining to fill buf
    do {
    remaining = buf.length;
    int n;
    // read to fill the buf or until EOF. Loop ends when either:
    // - remaining == 0 and there's possibly more to be read 
from stream; or

    // - remaining > 0 and the stream is at EOF
    while (remaining > 0 &&
   (n = read(buf, buf.length - remaining, remaining)) 
>= 0) {

    remaining -= n;
    }

    int nread = buf.length - remaining;
    if (nread > 0) {
    if (MAX_BUFFER_SIZE - total < nread) {
    throw new OutOfMemoryError("Required array size too 
large");

    }
    total += nread;
    byte[] copy;
    if (remaining == 0) { // buf is filled to the top
    copy = buf;
    buf = new byte[DEFAULT_BUFFER_SIZE];
    } else {
    copy = Arrays.copyOf(buf, nread);
    }
    if (result == null) {
    result = copy;
    } else {
    bufs = new ArrayList<>(8);
    bufs.add(result);
    bufs.add(copy);
    }
    }
    } while (remaining == 0); // there may be more bytes in stream

    if (bufs == null) {
    return result == null ? new byte[0] : result;
    }

    result = new byte[total];
    int offset = 0;
    for (byte[] b : bufs) {
    System.arraycopy(b, 0, result, offset, b.length);
    offset += b.length;
    }

    return result;
    }



Regards, Peter

On 12/20/2017 12:59 PM, Peter Levart wrote:

Hi Brian,

There's also a variation of copy-ing fragment possible, that replaces 
copying with allocation:


    byte[] copy;
    if (nread == DEFAULT_BUFFER_SIZE) {
    copy = buf;
    if (n >= 0) {
    buf = new byte[DEFAULT_BUFFER_SIZE];
    }
    } else {
    copy = Arrays.copyOf(buf, nread);
    }

For big FileInputStream(s), the buf will be fully read (nread == 
DEFAULT_BUFFER_SIZE) most of the times. So this might be an 
improvement if allocation (involving pre-zeroing) is faster than 
Arrays.copyOf() which avoids pre-zeroing, but involves copying.


Regards, Peter

On 12/20/2017 12:45 PM, Peter Levart wrote:

Hi Brian,

On 12/20/2017 12:22 AM, Brian Burkhalter wrote:
On Dec 19, 2017, at 2:36 PM, Brian Burkhalter 
 wrote:



You can also simplify the “for(;;) + break" into a do while loop:

do {
  int nread = 0;
  ...
} while (n > 0);

Good suggestion but I think that this needs to be "while (n >= 0)."

Updated version here:

http://cr.openjdk.java.net/~bpb/8193832/webrev.02/

Thanks,

Brian


Looks good. There is one case that could be further optimized. When 
result.length <= DEFAULT_BUFFER_SIZE, the allocation of ArrayList 
could be avoided. Imagine a use case where lots of small files are 
read into byte[] arrays. For exmaple:


    public byte[] readAllBytes() throws IOException {
    List bufs = null;
    byte[] result = null;
    byte[] buf = new byte[DEFAULT_BUFFER_SIZE];
    int total = 0;
    int n;
    do {
    int nread = 0;

    // read to EOF which may read more or less than buffer size
    while ((n = read(buf, nread, buf.length - nread)) > 0) {
    nread += n;
    }

    if (nread > 0) {
    if (MAX_BUFFER_SIZE - total < nread) {
    throw new OutOfMemoryError("Required array size 
too large");

    }
    total += nread;
    byte[] copy = (n < 0 && nread == DEFAULT_BUFFER_SIZE) ?
    buf : Arrays.copyOf(buf, nread);
    if (result == null) {
    result = copy;
    } else {
    bufs = new ArrayList<>(8);
    bufs.add(result);
    bufs.add(copy);
    }
    }
    } while (n >= 0); // if the last call to read returned -1, 
then break


    if (bufs == null) {
    return result == 

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Peter Levart]

Hi Brian,

There's also a variation of copy-ing fragment possible, that replaces 
copying with allocation:


    byte[] copy;
    if (nread == DEFAULT_BUFFER_SIZE) {
    copy = buf;
    if (n >= 0) {
    buf = new byte[DEFAULT_BUFFER_SIZE];
    }
    } else {
    copy = Arrays.copyOf(buf, nread);
    }

For big FileInputStream(s), the buf will be fully read (nread == 
DEFAULT_BUFFER_SIZE) most of the times. So this might be an improvement 
if allocation (involving pre-zeroing) is faster than Arrays.copyOf() 
which avoids pre-zeroing, but involves copying.


Regards, Peter

On 12/20/2017 12:45 PM, Peter Levart wrote:

Hi Brian,

On 12/20/2017 12:22 AM, Brian Burkhalter wrote:
On Dec 19, 2017, at 2:36 PM, Brian Burkhalter 
 wrote:



You can also simplify the “for(;;) + break" into a do while loop:

do {
  int nread = 0;
  ...
} while (n > 0);

Good suggestion but I think that this needs to be "while (n >= 0)."

Updated version here:

http://cr.openjdk.java.net/~bpb/8193832/webrev.02/

Thanks,

Brian


Looks good. There is one case that could be further optimized. When 
result.length <= DEFAULT_BUFFER_SIZE, the allocation of ArrayList 
could be avoided. Imagine a use case where lots of small files are 
read into byte[] arrays. For exmaple:


    public byte[] readAllBytes() throws IOException {
    List bufs = null;
    byte[] result = null;
    byte[] buf = new byte[DEFAULT_BUFFER_SIZE];
    int total = 0;
    int n;
    do {
    int nread = 0;

    // read to EOF which may read more or less than buffer size
    while ((n = read(buf, nread, buf.length - nread)) > 0) {
    nread += n;
    }

    if (nread > 0) {
    if (MAX_BUFFER_SIZE - total < nread) {
    throw new OutOfMemoryError("Required array size 
too large");

    }
    total += nread;
    byte[] copy = (n < 0 && nread == DEFAULT_BUFFER_SIZE) ?
    buf : Arrays.copyOf(buf, nread);
    if (result == null) {
    result = copy;
    } else {
    bufs = new ArrayList<>(8);
    bufs.add(result);
    bufs.add(copy);
    }
    }
    } while (n >= 0); // if the last call to read returned -1, 
then break


    if (bufs == null) {
    return result == null ? new byte[0] : result;
    }

    result = new byte[total];
    int offset = 0;
    for (byte[] b : bufs) {
    System.arraycopy(b, 0, result, offset, b.length);
    offset += b.length;
    }

    return result;
    }


There is a possibility that JIT already avoids allocating ArrayList 
utilizing EA if all involved ArrayList methods inline, so this 
potential optimization should be tested 1st to see if it actually 
helps improve the "small file" case.


Regards, Peter

Re: RFR 8193832: Performance of InputStream.readAllBytes() could be improved

[Peter Levart]

Hi Brian,

On 12/20/2017 12:22 AM, Brian Burkhalter wrote:

On Dec 19, 2017, at 2:36 PM, Brian Burkhalter  
wrote:


You can also simplify the “for(;;) + break" into a do while loop:

do {
  int nread = 0;
  ...
} while (n > 0);

Good suggestion but I think that this needs to be "while (n >= 0)."

Updated version here:

http://cr.openjdk.java.net/~bpb/8193832/webrev.02/

Thanks,

Brian


Looks good. There is one case that could be further optimized. When 
result.length <= DEFAULT_BUFFER_SIZE, the allocation of ArrayList could 
be avoided. Imagine a use case where lots of small files are read into 
byte[] arrays. For exmaple:


    public byte[] readAllBytes() throws IOException {
    List bufs = null;
    byte[] result = null;
    byte[] buf = new byte[DEFAULT_BUFFER_SIZE];
    int total = 0;
    int n;
    do {
    int nread = 0;

    // read to EOF which may read more or less than buffer size
    while ((n = read(buf, nread, buf.length - nread)) > 0) {
    nread += n;
    }

    if (nread > 0) {
    if (MAX_BUFFER_SIZE - total < nread) {
    throw new OutOfMemoryError("Required array size too 
large");

    }
    total += nread;
    byte[] copy = (n < 0 && nread == DEFAULT_BUFFER_SIZE) ?
    buf : Arrays.copyOf(buf, nread);
    if (result == null) {
    result = copy;
    } else {
    bufs = new ArrayList<>(8);
    bufs.add(result);
    bufs.add(copy);
    }
    }
    } while (n >= 0); // if the last call to read returned -1, then 
break


    if (bufs == null) {
    return result == null ? new byte[0] : result;
    }

    result = new byte[total];
    int offset = 0;
    for (byte[] b : bufs) {
    System.arraycopy(b, 0, result, offset, b.length);
    offset += b.length;
    }

    return result;
    }


There is a possibility that JIT already avoids allocating ArrayList 
utilizing EA if all involved ArrayList methods inline, so this potential 
optimization should be tested 1st to see if it actually helps improve 
the "small file" case.


Regards, Peter

Re: [11] RFR (XS) 8193838: Update jtreg requiredVersion to 4.2 b11 for JDK 11 and 12 support

[serguei.spit...@oracle.com]

David,

It looks good.

Thanks,
Serguei


On 12/20/17 02:22, David Holmes wrote:
Before we can update to JDK version 11, jtreg needs to be updated to 
recognize that release value - which is happening in jtreg 4.2 b11. So 
we then need to ensure that jtreg 4.2 b11 is used by updating the 
requiredVersion in each of the TEST.ROOT files, and in the jib 
configuiration.


bug: https://bugs.openjdk.java.net/browse/JDK-8193838
webrev: http://cr.openjdk.java.net/~dholmes/8193838/webrev/

Testing - TBD once b11 is promoted
 - local build using jib
 - hs/jdk tier1 and tier2

Thanks,
David

Re: [11] RFR (XS) 8193838: Update jtreg requiredVersion to 4.2 b11 for JDK 11 and 12 support

[Alan Bateman]

On 20/12/2017 10:22, David Holmes wrote:
Before we can update to JDK version 11, jtreg needs to be updated to 
recognize that release value - which is happening in jtreg 4.2 b11. So 
we then need to ensure that jtreg 4.2 b11 is used by updating the 
requiredVersion in each of the TEST.ROOT files, and in the jib 
configuiration.


bug: https://bugs.openjdk.java.net/browse/JDK-8193838
webrev: http://cr.openjdk.java.net/~dholmes/8193838/webrev/

Looks fine.

Re: RFR [jdk8] : 8193807 : AIX: avoid UnsatisfiedLinkError by providing empty basic implementations of getSystemCpuLoad and getProcessCpuLoad

[Erik Joelsson]

Looks ok to me.

/Erik


On 2017-12-20 10:32, Volker Simonis wrote:

Hi Matthias,

the change looks good!
I can sponsor it once we get the approval.

Also forwarded to buil-dev for the minimal build change.

Thank you and best regards,
Volker


On Wed, Dec 20, 2017 at 9:50 AM, Baesken, Matthias
 wrote:

Hello  , Mark reported  this issue on AIX  with OpenJDK8 :


I'm getting an unsatisfied link error when using logstash on AIX but I suspect 
the issue is with AIX or the JRE, the exception is:

java.lang.UnsatisfiedLinkError: 
sun.management.OperatingSystemImpl.getProcessCpuLoad()D
  getProcessCpuLoad at 
sun/management/OperatingSystemImpl.java:-2
  at 
org/logstash/instrument/monitors/ProcessMonitor.java:40
 detect at 
org/logstash/instrument/monitors/ProcessMonitor.java:79
   generate at 
org/logstash/instrument/reports/ProcessReport.java:15

this is the line in logstash:

this.cpuProcessPercent = scaleLoadToPercent(unixOsBean.getProcessCpuLoad());

https://github.com/elastic/logstash/blob/master/logstash-core/src/main/java/org/logstash/instrument/monitors/ProcessMonitor.java


Could anybody help steer me in the right direction?


This fix addresses  the missing  getSystemCpuLoad and getProcessCpuLoad  on AIX 
 .
JDK8 is only affected , JDK9 and higher is not affected .

Could I get a  review for this change   ?


Change :

http://cr.openjdk.java.net/~mbaesken/webrevs/8193807/

Bug :

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


Thanks, Matthias

[11] RFR (XS) 8193838: Update jtreg requiredVersion to 4.2 b11 for JDK 11 and 12 support

[David Holmes]

Before we can update to JDK version 11, jtreg needs to be updated to 
recognize that release value - which is happening in jtreg 4.2 b11. So 
we then need to ensure that jtreg 4.2 b11 is used by updating the 
requiredVersion in each of the TEST.ROOT files, and in the jib 
configuiration.


bug: https://bugs.openjdk.java.net/browse/JDK-8193838
webrev: http://cr.openjdk.java.net/~dholmes/8193838/webrev/

Testing - TBD once b11 is promoted
 - local build using jib
 - hs/jdk tier1 and tier2

Thanks,
David

Re: RFR [jdk8] : 8193807 : AIX: avoid UnsatisfiedLinkError by providing empty basic implementations of getSystemCpuLoad and getProcessCpuLoad

[Volker Simonis]

Hi Matthias,

the change looks good!
I can sponsor it once we get the approval.

Also forwarded to buil-dev for the minimal build change.

Thank you and best regards,
Volker


On Wed, Dec 20, 2017 at 9:50 AM, Baesken, Matthias
 wrote:
> Hello  , Mark reported  this issue on AIX  with OpenJDK8 :
>
>>
>>I'm getting an unsatisfied link error when using logstash on AIX but I 
>>suspect the issue is with AIX or the JRE, the exception is:
>>
>> java.lang.UnsatisfiedLinkError: 
>> sun.management.OperatingSystemImpl.getProcessCpuLoad()D
>>  getProcessCpuLoad at 
>> sun/management/OperatingSystemImpl.java:-2
>>  at 
>> org/logstash/instrument/monitors/ProcessMonitor.java:40
>> detect at 
>> org/logstash/instrument/monitors/ProcessMonitor.java:79
>>   generate at 
>> org/logstash/instrument/reports/ProcessReport.java:15
>>
>> this is the line in logstash:
>>
>> this.cpuProcessPercent = scaleLoadToPercent(unixOsBean.getProcessCpuLoad());
>>
>> https://github.com/elastic/logstash/blob/master/logstash-core/src/main/java/org/logstash/instrument/monitors/ProcessMonitor.java
>>
>>
>> Could anybody help steer me in the right direction?
>>
>
> This fix addresses  the missing  getSystemCpuLoad and getProcessCpuLoad  on 
> AIX  .
> JDK8 is only affected , JDK9 and higher is not affected .
>
> Could I get a  review for this change   ?
>
>
> Change :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8193807/
>
> Bug :
>
> https://bugs.openjdk.java.net/browse/JDK-8193807
>
>
> Thanks, Matthias

Re: RFR: JDK8u Backport of 8153955: increase java.util.logging.FileHandler MAX_LOCKS limit

[Daniel Fuchs]

Hi Ramanand,

On 18/12/2017 11:41, Ramanand Patil wrote:

Hi all,
Please review the fix for JDK8u Backport of 
https://bugs.openjdk.java.net/browse/JDK-8153955
Backport Bug: https://bugs.openjdk.java.net/browse/JDK-8161266
Webrev: http://cr.openjdk.java.net/~rpatil/8161266/webrev.00/


Looks good to me as well.

best regards,

-- daniel



Summary(also added to backport bug description):
The fix from JDK9 cannot be backported as is into the jdk8u and earlier update 
releases, since it contains JDK API spec changes.
In JDK9 the fix is added by altering the FileHandler spec, which introduces a new 
configurable property "java.util.logging.FileHandler.maxLocks" to 
java.util.logging.FileHandler, defined in .../conf/logging.properties.
The solution proposed for update releases is:
To introduce an internal JDK implementation specific property- 
"jdk.internal.FileHandlerLogging.maxLocks" which will be used to 
control/override FileHandler's MAX_LOCKS value. The default value of the maxLocks (100) 
will be retained if this new System property is not set. The new property is read only 
once during FileHandler class initialization.


Regards,
Ramanand.

RFR [jdk8] : 8193807 : AIX: avoid UnsatisfiedLinkError by providing empty basic implementations of getSystemCpuLoad and getProcessCpuLoad

[Baesken, Matthias]

Hello  , Mark reported  this issue on AIX  with OpenJDK8 :

>
>I'm getting an unsatisfied link error when using logstash on AIX but I suspect 
>the issue is with AIX or the JRE, the exception is:
>
> java.lang.UnsatisfiedLinkError: 
> sun.management.OperatingSystemImpl.getProcessCpuLoad()D
>  getProcessCpuLoad at 
> sun/management/OperatingSystemImpl.java:-2
>  at 
> org/logstash/instrument/monitors/ProcessMonitor.java:40
> detect at 
> org/logstash/instrument/monitors/ProcessMonitor.java:79
>   generate at 
> org/logstash/instrument/reports/ProcessReport.java:15
>
> this is the line in logstash:
>
> this.cpuProcessPercent = scaleLoadToPercent(unixOsBean.getProcessCpuLoad());
>
> https://github.com/elastic/logstash/blob/master/logstash-core/src/main/java/org/logstash/instrument/monitors/ProcessMonitor.java
>
>
> Could anybody help steer me in the right direction?
>

This fix addresses  the missing  getSystemCpuLoad and getProcessCpuLoad  on AIX 
 .
JDK8 is only affected , JDK9 and higher is not affected .

Could I get a  review for this change   ?


Change :

http://cr.openjdk.java.net/~mbaesken/webrevs/8193807/

Bug :

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


Thanks, Matthias

Re: RFR: JDK8u Backport of 8153955: increase java.util.logging.FileHandler MAX_LOCKS limit

[Seán Coffey]

Looks fine to me.

regards,
Sean.


On 18/12/2017 11:41, Ramanand Patil wrote:

Hi all,
Please review the fix for JDK8u Backport of 
https://bugs.openjdk.java.net/browse/JDK-8153955
Backport Bug: https://bugs.openjdk.java.net/browse/JDK-8161266
Webrev: http://cr.openjdk.java.net/~rpatil/8161266/webrev.00/

Summary(also added to backport bug description):
The fix from JDK9 cannot be backported as is into the jdk8u and earlier update 
releases, since it contains JDK API spec changes.
In JDK9 the fix is added by altering the FileHandler spec, which introduces a new 
configurable property "java.util.logging.FileHandler.maxLocks" to 
java.util.logging.FileHandler, defined in .../conf/logging.properties.
The solution proposed for update releases is:
To introduce an internal JDK implementation specific property- 
"jdk.internal.FileHandlerLogging.maxLocks" which will be used to 
control/override FileHandler's MAX_LOCKS value. The default value of the maxLocks (100) 
will be retained if this new System property is not set. The new property is read only 
once during FileHandler class initialization.


Regards,
Ramanand.