Re: java.util.stream 'map' method, proposal of method name change

[Krunoslav Magazin]

Thank you all on comments. I have studied referenced sources and now I
will upgrade my initial comment in context of mentioned mutative
update. I find now that first question that need answer is what is the
definition of stream and how pipeline of operations relate to stream.

Method 'map' implies existence of materialized in/out streams. Example
is linear queue. This goes with definition on
https://en.m.wikipedia.org/wiki/Map_(higher-order_function) using list
as map source and output. Same I find in Haskell and Lisp usage of map
or mapcar function. Name of map function is indisputable as defined in
wiki resources. But is usage of that map function justified in java
streams ?
With java streams when implementation use operation fusion on pipeline
of operations there is no intermediary queue between operations.
https://www.ibm.com/developerworks/java/library/j-java-streams-1-brian-goetz/index.html?ca=drs-
chapter 'Stream versus collections', about fusion. This is
implementation specific ("how" to do it)

In common usage word stream represent current of elements. Stream
exist if there is at least one moving element. Without moving element
there is no stream. Stream have no state. Stream is agnostic on: 1)
element type; 2) source state; 3) target state.
This is how I see elements going through pipeline of operations when I
build sequence of operations ("what" to do with an element in
transition). Elements are emitted one by one from backed source
collection by stream() operation (stream source), going through
pipeline composed of filter() and map() operations, intermediate
operations like sorted() (ending one stream and emitting another) and
ending with the sink (collect(), count()...). Map method effectively
replaces element with computed one, stream doesn't care and source
collection is not affected. From this definition of stream goes that
operations are on the element, not on a stream (or collection with
state). This goes with my stand that word replace(With) would be
appropriate name for method, not map. If written description of stream
reflect streams library build up than usage of word map for method in
question is not justified here.

Article 
https://www.ibm.com/developerworks/java/library/j-java-streams-1-brian-goetz/index.html?ca=drs-
describe java streams library. It states that "Streams provide no
storage for the elements..." but in comment @Goetz said that 'replace'
implies mutative update. Am I missing something?


I will appreciate any comments.
Best regards,
Krunoslav Magazin

Re: RFR 8173817: StackOverflowError in "process reaper" thread

[Martin Buchholz]

I suspect this won't save any stack frames in the stack-starved thread...
but seems like small progress nevertheless.

Perhaps lambdas that are likely to show up in stack traces should be given
real static classes with good names to make those stack traces more
readable.

On Fri, Aug 18, 2017 at 7:48 AM, Roger Riggs  wrote:

> Please review a change to the process reaper completion handler's choice
> of an anonymous inner class
> for more consistent stack usage.  Additional background and discussion in
> the issue 8173817 [1].
>
> Webrev:
>http://cr.openjdk.java.net/~rriggs/webrev-stackoverflow-8173817/
>
> Issue:
> [1]   https://bugs.openjdk.java.net/browse/JDK-8173817
>
> Thanks, Roger
>
>

RFR JDK-8186464: ZipFile cannot read some InfoZip ZIP64 zip files

[Xueming Shen]

Hi

Please help review the proposed change for  JDK-8186464.

Issue: https://bugs.openjdk.java.net/browse/JDK-8186464
Webrev: http://cr.openjdk.java.net/~sherman/8186464/webrev

Note:

It appears infozip always adds a ZIP64 format "end" record when there is 
any entry
that needs ZIP64 support, even the ZIP64.end record itself is really not 
necessary. As
the spec suggests, the ZIP64 format record is only needed if any of its 
fields is too

small to hold the required data, which is not the case in this scenario.

  4.4.1.4 If one of the fields in the end of central directory
  record is too small to hold required data, the field should be
  set to -1 (0x or 0x) and the ZIP64 format record
  should be created.

That said, as Martin pointed out, the spec does not prevent the 
implementation
from adding this ZIP64 format record even it's not necessary. Especially 
given the
fact infozip's implementation is actually doing it, it's reasonable to 
be liberal here

to accept this layout in our implementation.

I have manually verified the change does fix the problem (either use the 
jar tf or
java jdk.nio.zipfs.ZipInfo to check the offending zip file). Given the 
nature of the
test case, I'm hesitated to add this test as a unit/regression into the 
repo for now.


thanks,
Sherman

Re: RFR 8173817: StackOverflowError in "process reaper" thread

[David Holmes]

Looks good to me Roger!

Thanks,
David

On 19/08/2017 12:48 AM, Roger Riggs wrote:
Please review a change to the process reaper completion handler's choice 
of an anonymous inner class
for more consistent stack usage.  Additional background and discussion 
in the issue 8173817 [1].


Webrev:
    http://cr.openjdk.java.net/~rriggs/webrev-stackoverflow-8173817/

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

Thanks, Roger

Re: Manifest Add-Exports vs. command line --add-exports

[Tom Hood]

Thanks, Mandy.  I was beginning to think my followup question might have
gotten lost and I was considering a new post.

I'm unable to get --illegal-access=permits to work for our webstart app by
setting it in the Java Control Panel with the 9+181 build.  It appears
webstart crashes when I do this.  I tried enabling tracing & logging from
the Java Control Panel "Advanced" tab, but couldn't find anything left
behind.

Setting a long list of --add-exports in the Java Control Panel *does* work
for our webstart app with the 9+181 build.  However, I don't believe this
is going to work for our customer who has 1000's of geographically
distributed users.  We have next to zero control over how/when system
administrators perform the java installations.  Likewise, asking all those
users to manually add the options themselves is too much to ask of them.  I
predict many complaints and calls to our help desk that our app doesn't
work with Java 9.

Any chance of adding a more webstart-friendly "JEP260 last resort" for Java
9 ?  Our vendors need more time to remove dependencies from their
libraries.  I'm concerned that as we proceed in testing our app with Java 9
that the list of JEP260-override options will grow.  For example, the
--add-opens was added due to an illegal setAccessible(true) reflection call.

BTW: I had a slightly longer than necessary list of java-vm-args in my
original email in this thread.  After correcting that issue, I am now able
to launch our webstart app without editing the Java Control Panel as we are
just under the java command line arg limit.  So maybe we'll get lucky and
further testing won't find any new JEP260-override options that are
needed.  Knock on wood.

-- Tom

On Mon, Aug 21, 2017 at 3:50 PM, mandy chung  wrote:

>
>
> On 8/16/17 2:14 PM, Tom Hood wrote:
>
>> Thanks Richard.  Sorry I didn't see your post before hitting send.
>>
>> So does this mean there is no workaround for a webstart app that requires
>> a
>> lot of the --add-export options?
>>
>
> The only workaround I can think of is to set in Java Control Panel with
> `--add-exports` or `--illegal-access=permits` options.
>
> The long term solution is to request the library maintainer to eliminate
> their dependency on JDK internal APIs.
>
> Mandy
>
>> On Wed, Aug 16, 2017 at 2:12 PM, Tom Hood  wrote:
>>
>> I found Alan's video  (time:
>>> about 27:35) that goes with the slides pdf link and he mentions the
>>> Add-Exports line need to go in the manifest of the jar with the
>>> Main-Class.  I just now tried that and it still didn't take effect.  I'm
>>> still getting the same error:
>>>
>>> java.lang.IllegalAccessError: class com.sun.media.imageioimpl.plug
>>> ins.pnm.PNMImageWriter
>>> (in unamed module @0x52a256fc) cannot access class
>>> sun.security.action.GetPropertyAction
>>> (in module java.base) because module java.base does not export
>>> sun.security.action to unnamed module @0x52a256fc
>>>at com.sun.media.imageioimpl.plugins.pnm.PNMImageWriter.<
>>> init>[PNMImageWriter.java:85]
>>>
>>> The META-INF/MANIFEST.MF looks like this:
>>>
>>> Manifest-Version: 1.0
>>> Trusted-Library: true
>>> Application-Library-Allowable-Codebase: *
>>> Application-Name: TheName
>>> Permissions: all-permissions
>>> *Add-Exports: java.base/sun.security.action*
>>>
>>> Created-By: 1.6.0_24 (Sun Microsystems Inc.)
>>> Caller-Allowable-Codebase: *
>>> Main-Class: path.to.TheMainClass
>>> Codebase: *
>>>
>>> Name: path/to/a/file.class
>>> SHA1-Digest: thedigest
>>>
>>> (many more such class+digest pairs)
>>>
>>> Should this work or am I using it incorrectly?
>>>
>>> Thanks,
>>> -- Tom
>>>
>>> On Wed, Aug 16, 2017 at 1:30 PM, Tom Hood  wrote:
>>>
>>> Hi,

 I need a little help understanding the difference between "Add-Exports:"
 in a jar's manifest vs. the command line arg --add-exports.  I can get
 --add-exports to work, but not Add-Exports.

 JDK Version: 9 build 181 windows 64

 Slide 23 of http://openjdk.java.net/projec
 ts/jigsaw/talks/prepare-for-jd
 k9-j1-2016.pdf seems to suggest Add-Exports in the manifest as an
 alternative to --add-exports

 Our webstart-launched app requires a long list of --add-module and/or
 --add-exports command line options.  The list is long enough that it
 exceeds a limit on number of args and webstart fails to launch the app
 with
 the popup "too many args to run".

 Specifically, I was trying to allow jai_imageio.jar (1.0_01) to access
 java.base/sun.security.action by adding --add-exports=java.base/sun.se
 curity.action=ALL-UNNAMED
 to the java-vm-args in the jnlp.  However, that one additional arg
 pushed
 it over the edge and exceeded the limit.

 This is the full j2se element in our jnlp:

 >>>   java-vm-args="--add-modules=java.corba --add-exports
 

Re: """error: module testng reads package test from both test and testng"""

[mandy chung]

I can reproduce it and javac outputs the following errors.  javac 
compiles successfully if jcommander.jar is removed.   Will look into it.


error: the unnamed module reads package com.beust.jcommander from both 
jcommander and testng
error: the unnamed module reads package com.beust.jcommander.validators 
from both jcommander and testng
error: the unnamed module reads package com.beust.jcommander.internal 
from both jcommander and testng
error: the unnamed module reads package 
com.beust.jcommander.defaultprovider from both jcommander and testng
error: the unnamed module reads package com.beust.jcommander.converters 
from both jcommander and testng
error: module jcommander reads package com.beust.jcommander from both 
testng and jcommander
error: module jcommander reads package com.beust.jcommander.validators 
from both testng and jcommander
error: module jcommander reads package com.beust.jcommander.internal 
from both testng and jcommander
error: module jcommander reads package 
com.beust.jcommander.defaultprovider from both testng and jcommander
error: module jcommander reads package com.beust.jcommander.converters 
from both testng and jcommander
error: module testng reads package com.beust.jcommander from both 
jcommander and testng
error: module testng reads package com.beust.jcommander.validators from 
both jcommander and testng
error: module testng reads package com.beust.jcommander.internal from 
both jcommander and testng
error: module testng reads package com.beust.jcommander.defaultprovider 
from both jcommander and testng
error: module testng reads package com.beust.jcommander.converters from 
both jcommander and testng
/scratch/mchung/ws/jdk10/jdk10-dev/jdk/test/java/lang/ModuleTests/addXXX/test/module-info.java:23: 
error: module test reads package com.beust.jcommander from both testng 
and jcommander

module test {

Mandy

On 8/21/17 2:14 PM, Martin Buchholz wrote:
I just upgraded to 4.2-b08, although without any effect on this 
problem.  jcommander.jar is from jtreg's lib directory


code-tools/jtreg/make/Defs.gmk says:
"""
# TestNG requires jcommander, which may or may not be bundled with 
TESTNG_JAR.

# If it is not, set JCOMMANDER_JAR to an appropriate version
"""

 $ ls -l ./JTwork/modules
total 1620
-rw-r--r-- 1 martin martin   63504 Aug 21 12:27 jcommander.jar
-rw-r--r-- 1 martin martin 1589287 Aug 21 12:27 testng.jar

 $ /home/martin/jtreg-binaries/current/bin/jtreg -noreport -v:fail 
-compilejdk:/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk 
-testjdk:/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk 
java/lang/ModuleTests/addXXX

--
TEST: java/lang/ModuleTests/addXXX/Driver.java
TEST JDK: 
/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk


ACTION: build -- Failed. Compilation failed: Compilation failed
REASON: User specified action: run build test/* m1/* m2/* m3/* m4/*
TIME:   0.943 seconds
messages:
command: build test/* m1/* m2/* m3/* m4/*
reason: User specified action: run build test/* m1/* m2/* m3/* m4/*
Test directory:
  compile: test/module-info, test/test.C, test/test.Service, 
test/test.Main, m1/module-info, m1/p1.C, m2/module-info, m2/p2.C, 
m2/p2.internal.C, m3/module-info, m3/p3.C, m4/module-info, m4/p4.C

elapsed time (seconds): 0.943

ACTION: compile -- Failed. Compilation failed: Compilation failed
REASON: .class file out of date or does not exist
TIME:   0.938 seconds
messages:
command: compile 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/test/module-info.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/test/test/C.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/test/test/Service.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/test/test/Main.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m1/module-info.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m1/p1/C.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m2/module-info.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m2/p2/C.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m2/p2/internal/C.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m3/module-info.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m3/p3/C.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m4/module-info.java 
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m4/p4/C.java

reason: .class file out of date or does not exist
Mode: othervm
elapsed time (seconds): 0.938
configuration:
javac compilation environment
  add modules: testng jcommander
  module path: /home/martin/ws/jdk10/jdk/test/JTwork/modules

rerun:
DISPLAY=localhost:10.0 \
HOME=/home/martin \
LANG=en_US.UTF-8 \
PATH=/bin:/usr/bin \
/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk/bin/javac 
\

Re: Manifest Add-Exports vs. command line --add-exports

[mandy chung]

On 8/16/17 2:14 PM, Tom Hood wrote:

Thanks Richard.  Sorry I didn't see your post before hitting send.

So does this mean there is no workaround for a webstart app that requires a
lot of the --add-export options?


The only workaround I can think of is to set in Java Control Panel with 
`--add-exports` or `--illegal-access=permits` options.


The long term solution is to request the library maintainer to eliminate 
their dependency on JDK internal APIs.


Mandy

On Wed, Aug 16, 2017 at 2:12 PM, Tom Hood  wrote:


I found Alan's video  (time:
about 27:35) that goes with the slides pdf link and he mentions the
Add-Exports line need to go in the manifest of the jar with the
Main-Class.  I just now tried that and it still didn't take effect.  I'm
still getting the same error:

java.lang.IllegalAccessError: class 
com.sun.media.imageioimpl.plugins.pnm.PNMImageWriter
(in unamed module @0x52a256fc) cannot access class 
sun.security.action.GetPropertyAction
(in module java.base) because module java.base does not export
sun.security.action to unnamed module @0x52a256fc
   at com.sun.media.imageioimpl.plugins.pnm.PNMImageWriter.<
init>[PNMImageWriter.java:85]

The META-INF/MANIFEST.MF looks like this:

Manifest-Version: 1.0
Trusted-Library: true
Application-Library-Allowable-Codebase: *
Application-Name: TheName
Permissions: all-permissions
*Add-Exports: java.base/sun.security.action*
Created-By: 1.6.0_24 (Sun Microsystems Inc.)
Caller-Allowable-Codebase: *
Main-Class: path.to.TheMainClass
Codebase: *

Name: path/to/a/file.class
SHA1-Digest: thedigest

(many more such class+digest pairs)

Should this work or am I using it incorrectly?

Thanks,
-- Tom

On Wed, Aug 16, 2017 at 1:30 PM, Tom Hood  wrote:


Hi,

I need a little help understanding the difference between "Add-Exports:"
in a jar's manifest vs. the command line arg --add-exports.  I can get
--add-exports to work, but not Add-Exports.

JDK Version: 9 build 181 windows 64

Slide 23 of http://openjdk.java.net/projects/jigsaw/talks/prepare-for-jd
k9-j1-2016.pdf seems to suggest Add-Exports in the manifest as an
alternative to --add-exports

Our webstart-launched app requires a long list of --add-module and/or
--add-exports command line options.  The list is long enough that it
exceeds a limit on number of args and webstart fails to launch the app with
the popup "too many args to run".

Specifically, I was trying to allow jai_imageio.jar (1.0_01) to access
java.base/sun.security.action by adding 
--add-exports=java.base/sun.security.action=ALL-UNNAMED
to the java-vm-args in the jnlp.  However, that one additional arg pushed
it over the edge and exceeded the limit.

This is the full j2se element in our jnlp:



I then removed the last --add-exports to keep under the arg limit and
instead added an Add-Exports line to the jai_imageio.jar
META-INF/MANIFEST.MF

Add-Exports: java.base/sun.security.action

That doesn't appear to be taking affect.  Am I using it incorrectly?

Thanks,
-- Tom

Re: RFR: JDK-8186466: Fix accessibility and other minor issues in java.base

[Martin Buchholz]

I'm happy.  Thanks for going the extra kilometer.

On Mon, Aug 21, 2017 at 3:31 PM, Jonathan Gibbons <
jonathan.gibb...@oracle.com> wrote:

>
>
> On 08/21/2017 09:38 AM, Jonathan Gibbons wrote:
>
>>
>>
>> On 8/20/17 4:11 PM, Martin Buchholz wrote:
>>
>>> Again, I am happy to take the current state of this change.
>>>
>>> On Sat, Aug 19, 2017 at 2:19 PM, Jonathan Gibbons <
>>> jonathan.gibb...@oracle.com > wrote:
>>>
>>> Actually, thead and tbody have no direct significance for
>>> accessibility. They provide a semantic differentiation of the
>>> content, and provide a hook for different styling, as you have
>>> seen for "striped". Also note, although you can have many ,
>>> you can only have at most one , and at most one .
>>>
>>> Looking at Summary of BlockingDeque methods again, we have what might
>>> logically be a thead in the middle of a table, and the law of "only one
>>> thead, and only at the beginning" might be yet another hint that the html
>>> gods want us to split this table. This could become a nested table with two
>>> rows, one for "first" and one for "last", each of which contains a subtable
>>> with a thead.
>>>
>>
>> I can investigate that.
>>
>
> I investigated.
>
> It won't be a table with two rows; it'll be a table with 3 rows, because
> it would need a header row with column headings :-(  Also, you wouldn't
> have the columns aligned, because of the use of two tables.  And so you
> might as well go with two separate tables, and the "First"/"Last" labels
> moving into captions.
>
> I guess I'd like to declare victory on the BlockingQueue/Deque tables.
> They meet the desired accessibility requirements, which was the primary
> goal.  Even if they don't get the full "striped" approach, they are at
> least visually similar to the original versions in the JDK 8 and JDK 9 API,
> with respect to font, centering, etc.
>
> If we want to continue to enhance the appearance of these tables, we
> should take it offline from this review, and do more experiments on smaller
> API examples that are faster to turn around.
>
> -- Jon
>
>

Re: RFR: JDK-8186466: Fix accessibility and other minor issues in java.base

[Jonathan Gibbons]

On 08/21/2017 09:38 AM, Jonathan Gibbons wrote:



On 8/20/17 4:11 PM, Martin Buchholz wrote:

Again, I am happy to take the current state of this change.

On Sat, Aug 19, 2017 at 2:19 PM, Jonathan Gibbons 
> 
wrote:


Actually, thead and tbody have no direct significance for
accessibility. They provide a semantic differentiation of the
content, and provide a hook for different styling, as you have
seen for "striped". Also note, although you can have many ,
you can only have at most one , and at most one .

Looking at Summary of BlockingDeque methods again, we have what might 
logically be a thead in the middle of a table, and the law of "only 
one thead, and only at the beginning" might be yet another hint that 
the html gods want us to split this table. This could become a nested 
table with two rows, one for "first" and one for "last", each of 
which contains a subtable with a thead.


I can investigate that.


I investigated.

It won't be a table with two rows; it'll be a table with 3 rows, because 
it would need a header row with column headings :-(  Also, you wouldn't 
have the columns aligned, because of the use of two tables.  And so you 
might as well go with two separate tables, and the "First"/"Last" labels 
moving into captions.


I guess I'd like to declare victory on the BlockingQueue/Deque tables. 
They meet the desired accessibility requirements, which was the primary 
goal.  Even if they don't get the full "striped" approach, they are at 
least visually similar to the original versions in the JDK 8 and JDK 9 
API, with respect to font, centering, etc.


If we want to continue to enhance the appearance of these tables, we 
should take it offline from this review, and do more experiments on 
smaller API examples that are faster to turn around.


-- Jon

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Claes Redestad]

Sure, I'll add the following comment:

 /**
+ * As this class is used early during bootstrap, it's motivated to use
+ * Unsafe.compareAndSetObject instead of AtomicReferenceFieldUpdater
+ * (or VarHandles) to reduce dependencies and improve startup time.
+ */
+private static final Unsafe U = Unsafe.getUnsafe();

/Claes

On 2017-08-21 22:57, Bernd Eckenfels wrote:

I would add a comment to Unsafe why it is used (instead of AtomicUpdater) maybe 
pointing to the startup benchmark which shows the improved footprint? After all 
adding Unsafe is might trigger somebody to clean it up in the next release...

// we use Unsafe instead of AtomicReferenceUpdater as it reduces startup 
footprint

Gruss
Bernd
--
http://bernd.eckenfels.net

From: core-libs-dev  on behalf of Claes 
Redestad 
Sent: Monday, August 21, 2017 5:00:37 PM
To: Peter Levart; Aleksey Shipilev; core-libs-dev
Subject: Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from 
BufferedInputStream with Unsafe



On 08/21/2017 04:47 PM, Peter Levart wrote:

Is BufferedInputStream.close() intentionally not synchronized? All
other methods are. If close() was synchronized too, no CAS would be
needed and fields could be normal, not volatile. What is achieved by
close() not being synchronized? Fear of deadlocks?

I don't have the history here, but my gut-feeling is it's intentional to
allow calling close on streams that are blocked or waiting for data from
another thread in a non-blocking (or even deadlocking) fashion

/Claes

Re: RFR: JDK-8133680 add Stream.foldLeft() terminal operation

[Jonathan Bluett-Duncan]

> OK, so I think an indexed stream returning pairs of (long,T) is
the answer.

...which I suspect might be worth waiting on Project Valhalla for, if I've
understood you correctly, since an IndexedElement would essentially be a
`long` and object pair, and having the `long` element stored or retrievable
as a `Long` object would be a bit costly compared to having
stored/retrievable as a generic `long` stored on the stack, like what
Project Valhalla is exploring AFAICT.

Alternatively, perhaps having it implement a theoretical interface like
`LongObjectEntry` might be an option, at the expense of cluttering things a
bit?

Cheers,
Jonathan

On 21 August 2017 at 22:00, John Rose  wrote:

> On Aug 21, 2017, at 9:57 AM, Paul Sandoz  wrote:
> >
> > Hi,
> >
> > My preference, naming-wise, is to stick with the current naming scheme
> and refer to such operations as reduceLeft and reduceRight. We have used
> reduce both for the seed accepting, and optional returning methods, and we
> don’t use different names to distinguish those.
>
> +1
>
> > I don’t think we can rely on the sequential() trick, since as Tagir
> points out it is global.
>
> Yes, that's unfortunate for the present purpose.
>
> The closest thing we have at present is forEachOrdered (as you imply).
> The transform Stream.sorted looks like something close, but it can eagerly
> discard the current ordering.  What I think we want, as an input to
> sequential reduce,
> is something that (a) preserves the encounter order and (b) optionally
> transforms
> it in a predictable way (no-op for reduceLeft, reverse for reduceRight).
>
> So maybe there is something akin to Stream.sorted that wants to exist:
> Stream.ordered(p), where p is some token that expresses at least the
> null permutation (keep encounter order, but allow previous parallelism)
> and the reverse permutation.
>
> A complete, somewhat crazy generalization would be something like
> Stream.ordered(LongBinaryOperator comparator), where the comparator
> would decide where to place the stream elements based on an ordinal
> assigned to encounter order.  I don't know if there are middle grounds
> for less powerful distinctions that are useful, and are more powerful
> than just "forward/reverse".
>
> > My inclination would be to focus on reduceLeft, and spin off reduceRight
> into a separate issue and in addition ponder support for a reverse
> operation, since reduceRight can be built upon that.
>
> Thanks.
>
> > It’s possible to develop a reduceRight or reserve operation that does
> something very similar to forEachOrdered when operating in parallel (the
> last FJ task has to complete before given the ok for it’s direct sibling to
> the left to proceed when ready). For good splitters like ArrayList this
> might work well, for bad splitters like Iterator it will be terrible. It
> may be possible in some cases to fuse reverse().toArray().
>
> Yes, I think so.  Maybe the concept of "ordered()" is too weak; the thing
> we are considering is really turning a stream into a data source with
> two characteristics:  (a) it is ordered, and (b) it is random access.
> If you have a random access data sink (toArray) then, if you can
> determine data source indexes, you can then perform the actual
> reordering when you store the result, instead of earlier.
>
> So, here's a stream transform that gives those characteristics:
>
> Stream Stream.indexed().
>
> (Where IndexedElement implements Map.Entry.
> And can be pattern-matched, etc.)
>
> The semantics is that the elements are associated with a compact
> zero-based set of indexes (which may be either easy or hard to
> compute).  The indexes are compatible with forEachOrdered
> (encounter order).  Those indexes can be used to store the
> original stream's elements into any desired random-access pattern.
>
> From there, we can get buffering into an array (or a consumer
> function), which can redirect the values to a reducing collector
> that takes the values in whatever order the user wants.
>
> (The indexed-element guy is also good for numeric array processing.
> It should eventually be a value type.)
>
> OK, so I think an indexed stream returning pairs of (long,T) is
> the answer.
>
> — John

Re: """error: module testng reads package test from both test and testng"""

[Martin Buchholz]

I just upgraded to 4.2-b08, although without any effect on this problem.
 jcommander.jar is from jtreg's lib directory

code-tools/jtreg/make/Defs.gmk says:
"""
# TestNG requires jcommander, which may or may not be bundled with
TESTNG_JAR.
# If it is not, set JCOMMANDER_JAR to an appropriate version
"""

 $ ls -l ./JTwork/modules
total 1620
-rw-r--r-- 1 martin martin   63504 Aug 21 12:27 jcommander.jar
-rw-r--r-- 1 martin martin 1589287 Aug 21 12:27 testng.jar

 $ /home/martin/jtreg-binaries/current/bin/jtreg -noreport -v:fail
-compilejdk:/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk
-testjdk:/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk
java/lang/ModuleTests/addXXX
--
TEST: java/lang/ModuleTests/addXXX/Driver.java
TEST JDK:
/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk

ACTION: build -- Failed. Compilation failed: Compilation failed
REASON: User specified action: run build test/* m1/* m2/* m3/* m4/*
TIME:   0.943 seconds
messages:
command: build test/* m1/* m2/* m3/* m4/*
reason: User specified action: run build test/* m1/* m2/* m3/* m4/*
Test directory:
  compile: test/module-info, test/test.C, test/test.Service,
test/test.Main, m1/module-info, m1/p1.C, m2/module-info, m2/p2.C,
m2/p2.internal.C, m3/module-info, m3/p3.C, m4/module-info, m4/p4.C
elapsed time (seconds): 0.943

ACTION: compile -- Failed. Compilation failed: Compilation failed
REASON: .class file out of date or does not exist
TIME:   0.938 seconds
messages:
command: compile
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/test/module-info.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/test/test/C.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/test/test/Service.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/test/test/Main.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m1/module-info.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m1/p1/C.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m2/module-info.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m2/p2/C.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m2/p2/internal/C.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m3/module-info.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m3/p3/C.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m4/module-info.java
/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX/m4/p4/C.java
reason: .class file out of date or does not exist
Mode: othervm
elapsed time (seconds): 0.938
configuration:
javac compilation environment
  add modules: testng jcommander
  module path: /home/martin/ws/jdk10/jdk/test/JTwork/modules

rerun:
DISPLAY=localhost:10.0 \
HOME=/home/martin \
LANG=en_US.UTF-8 \
PATH=/bin:/usr/bin \

/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk/bin/javac
\

-J-Dtest.src=/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX \

-J-Dtest.src.path=/home/martin/ws/jdk10/jdk/test/java/lang/ModuleTests/addXXX
\

-J-Dtest.classes=/home/martin/ws/jdk10/jdk/test/JTwork/classes/java/lang/ModuleTests/addXXX/Driver.d
\

-J-Dtest.class.path=/home/martin/ws/jdk10/jdk/test/JTwork/classes/java/lang/ModuleTests/addXXX/Driver.d
\
-J-Dtest.vm.opts= \
-J-Dtest.tool.vm.opts= \
-J-Dtest.compiler.opts= \
-J-Dtest.java.opts= \

-J-Dtest.jdk=/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk
\

-J-Dcompile.jdk=/home/martin/ws/jdk10/build/linux-x86_64-normal-server-release/images/jdk
\
-J-Dtest.timeout.factor=1.0 \

@/home/martin/ws/jdk10/jdk/test/JTwork/java/lang/ModuleTests/addXXX/Driver.d/compile.0.jta
STDOUT:
STDERR:
error: module testng reads package test from both test and testng
1 error

TEST RESULT: Failed. Compilation failed: Compilation failed
--
Test results: failed: 1

On Mon, Aug 21, 2017 at 12:12 PM, mandy chung 
wrote:

> The test itself is a module that depends on testng.  So testng.jar is
> added as an automatic module.  This test passes on my setup.
>
> I am puzzling how jcommander.jar is added to the module path. Can you send
> the .jtr file and list what's in JTwork/modules directory?
>
> Mandy
>
>
> On 8/17/17 6:22 PM, Martin Buchholz wrote:
>
>> When I run the jtreg test
>> java/lang/ModuleTests/addXXX
>> I fail with
>>
>> direct:
>> error: module testng reads package test from both test and testng
>>
>> In the javac command line I see:
>>   --add-modules testng,jcommander
>> but ... testng and jcommander aren't modules; they're just garden variety
>> jar files.
>>
>> Running jtreg 4.2-b07.
>>
>
>

RFR 8186539 [testlibrary] : TestSocketFactory should allow triggers before match/replace

[Roger Riggs]

Please review a rmi testlibrary enhancement to allow a trigger byte 
sequence before
the match and replace. It improves the ease with which stream contents 
can be identified
when streams contain IP addresses and sequence numbers that change from 
run to run.


Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-socketfactory-trigger-8186539/

Thanks, Roger

Re: RFR: JDK-8133680 add Stream.foldLeft() terminal operation

[John Rose]

On Aug 21, 2017, at 9:57 AM, Paul Sandoz  wrote:
> 
> Hi,
> 
> My preference, naming-wise, is to stick with the current naming scheme and 
> refer to such operations as reduceLeft and reduceRight. We have used reduce 
> both for the seed accepting, and optional returning methods, and we don’t use 
> different names to distinguish those.

+1

> I don’t think we can rely on the sequential() trick, since as Tagir points 
> out it is global.

Yes, that's unfortunate for the present purpose.

The closest thing we have at present is forEachOrdered (as you imply).
The transform Stream.sorted looks like something close, but it can eagerly
discard the current ordering.  What I think we want, as an input to sequential 
reduce,
is something that (a) preserves the encounter order and (b) optionally 
transforms
it in a predictable way (no-op for reduceLeft, reverse for reduceRight).

So maybe there is something akin to Stream.sorted that wants to exist:
Stream.ordered(p), where p is some token that expresses at least the
null permutation (keep encounter order, but allow previous parallelism)
and the reverse permutation.

A complete, somewhat crazy generalization would be something like
Stream.ordered(LongBinaryOperator comparator), where the comparator
would decide where to place the stream elements based on an ordinal
assigned to encounter order.  I don't know if there are middle grounds
for less powerful distinctions that are useful, and are more powerful
than just "forward/reverse".

> My inclination would be to focus on reduceLeft, and spin off reduceRight into 
> a separate issue and in addition ponder support for a reverse operation, 
> since reduceRight can be built upon that.

Thanks.

> It’s possible to develop a reduceRight or reserve operation that does 
> something very similar to forEachOrdered when operating in parallel (the last 
> FJ task has to complete before given the ok for it’s direct sibling to the 
> left to proceed when ready). For good splitters like ArrayList this might 
> work well, for bad splitters like Iterator it will be terrible. It may be 
> possible in some cases to fuse reverse().toArray().

Yes, I think so.  Maybe the concept of "ordered()" is too weak; the thing
we are considering is really turning a stream into a data source with
two characteristics:  (a) it is ordered, and (b) it is random access.
If you have a random access data sink (toArray) then, if you can
determine data source indexes, you can then perform the actual
reordering when you store the result, instead of earlier.

So, here's a stream transform that gives those characteristics:

Stream Stream.indexed().

(Where IndexedElement implements Map.Entry.
And can be pattern-matched, etc.)

The semantics is that the elements are associated with a compact
zero-based set of indexes (which may be either easy or hard to
compute).  The indexes are compatible with forEachOrdered
(encounter order).  Those indexes can be used to store the
original stream's elements into any desired random-access pattern.

From there, we can get buffering into an array (or a consumer
function), which can redirect the values to a reducing collector
that takes the values in whatever order the user wants.

(The indexed-element guy is also good for numeric array processing.
It should eventually be a value type.)

OK, so I think an indexed stream returning pairs of (long,T) is
the answer.

— John

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Bernd Eckenfels]

I would add a comment to Unsafe why it is used (instead of AtomicUpdater) maybe 
pointing to the startup benchmark which shows the improved footprint? After all 
adding Unsafe is might trigger somebody to clean it up in the next release...

// we use Unsafe instead of AtomicReferenceUpdater as it reduces startup 
footprint

Gruss
Bernd
--
http://bernd.eckenfels.net

From: core-libs-dev  on behalf of Claes 
Redestad 
Sent: Monday, August 21, 2017 5:00:37 PM
To: Peter Levart; Aleksey Shipilev; core-libs-dev
Subject: Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from 
BufferedInputStream with Unsafe



On 08/21/2017 04:47 PM, Peter Levart wrote:
>
> Is BufferedInputStream.close() intentionally not synchronized? All
> other methods are. If close() was synchronized too, no CAS would be
> needed and fields could be normal, not volatile. What is achieved by
> close() not being synchronized? Fear of deadlocks?

I don't have the history here, but my gut-feeling is it's intentional to
allow calling close on streams that are blocked or waiting for data from
another thread in a non-blocking (or even deadlocking) fashion

/Claes

Re: [10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean

[mandy chung]

cc'ing serviceability-dev which is the right mailing list for platform 
management discussion.


JVMCI is currently named as `jdk.internal.vm.ci` (a JDK internal 
module).   I suppose this new module is intended to be kept as an 
internal module?


  30  * @since 9

should be @since 10.

JVMCIMBeans.java

  55 @Override
  56 public Set mbeanInterfaces() {
  57 return Collections.singleton(mbean.getClass());
  58 }

This mbeanInterfaces method should return the MBean interface class but 
not the class of the mbean implementation.  This allows the platform 
mbean to be looked up from ManagementFactory.getPlatformMXBean.  If this 
is a dynamic mbean, then this method simply returns an empty list (see 
DiagnosticCommandMBean).To support standard mbean, 
HotSpotJVMCICompilerFactory::mbeans method would need to include the 
mbean interface type in addition to the name and the mbean 
implementation object, i.e. may need to define a specific type for it.


Mandy


On 8/18/17 11:49 AM, Vladimir Kozlov wrote:
Updated changes in all repos: 
http://cr.openjdk.java.net/~kvn/8182701/webrev.01/


On 8/18/17 7:12 AM, Jaroslav Tulach wrote:

Thanks for pushing me forward, Vladimir. Yes, the changes are still 
needed if
we want the Graal compiler to expose its MBean in a lazy way. I am 
offering

new webrev for review. It contains following changes:

Per Mandy's suggestion I created new module jdk.vm.ci.management to 
bridge between
JVMCI and jdk.management. Adding new module was a bit tricky, but with 
great help of Jan

Lahoda I even managed to register it as a boot module.

I renamed the JVMCIMXBean class and dropped X per Vladimir's advice. I 
fixed

the non-standard location of JVMCIMXBean class.

I changed the interface to use Map, so the compiler is able to expose 
more

than a single bean.

That's it. I am looking forward to your review comments.
-jt

Here are original changes for reference:

http://cr.openjdk.java.net/~kvn/8182701/webrev.jdk/
http://cr.openjdk.java.net/~kvn/8182701/webrev.hs/

On 8/17/17 11:54 AM, Vladimir Kozlov wrote:

Hi Jaroslav,

What we should do with 8182701? Do you still need JVMCI changes?

Note, your changes to Graal [GR-5435] were integrated recently into 
JDK (jdk10/hs):

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

Thanks,
Vladimir

On 8/14/17 10:06 AM, Jaroslav Tulach wrote:

On čtvrtek 3. srpna 2017 17:03:39 CEST Jaroslav Tulach wrote:

On čtvrtek 27. července 2017 15:01:17 CEST Alan Bateman wrote:

On 27/07/2017 10:07, Jaroslav Tulach wrote:
Yes, it seems like a desirable goal to let Graal compiler work 
with just

java.base. Is there a description how to build JDK9/10 with just
java.base
that I could follow and test against?


You can use jlink to create a run-time image that only contains
java.base (jlink --module-path $JAVA_HOME/jmods --add-modules 
java.base

--output smalljre).


Status update: I've just tried to run Graal compiler against JDK9 
with only
java.base and jdk.internal.vm.ci modules, and there are some 
problems. I
need to resolve them first before I provide updated version of my 
patch.


FYI: As of
https://github.com/graalvm/graal/commit/ca9071941a1be7f1a3725529ecc231ff621d5ed0 

the Graal compiler can run with java.base, jdk.unsupported and 
jdk.vm.ci only
modules. But it wasn't easy, especially the 
http://wiki.apidesign.org/wiki/PropertyChangeListener required a bit 
of work.


-jt

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

[Claes Redestad]

I think it makes a lot of sense to get this into a 9.x update, and I'll 
work through the details as soon as there's a 9 update forest available 
to commit backports to.


/Claes

On 2017-08-21 20:05, Jonathan Bluett-Duncan wrote:
Would it not be viable to merge this into Java 9 in the foreseeable 
future for one of its inevitable bug fix releases? I admit I'm 
unfamiliar with the whole process, so I realise my question may be 
naive or that the answer is considered "common knowledge".


Cheers,
Jonathan

On 21 August 2017 at 18:41, Claes Redestad > wrote:




On 2017-08-21 19:15, Paul Sandoz wrote:

On 21 Aug 2017, at 02:53, Claes Redestad
> wrote:

Hi,

this patch addresses an unfortunate regression where
backtick characters
in a manifest can cause an AIOOBE.

Webrev:
http://cr.openjdk.java.net/~redestad/8186334/jdk.00/

Bug: https://bugs.openjdk.java.net/browse/JDK-8186334


Basically an off-by-one issue during certain steps in the
search algorithm,
meaning it is context dependent whether a backtick will
trip on this issue
or not.

Ooops, looks good.


Thanks for reviewing!


I see a workaround has been pushed to Jackson, which reduced
my urge to suggest a nine respin. But… being slightly paranoid
testing JarFile on a local maven central mirror would give us
a better sense of the impact.


I think the 9 train has left the station even for issues of this
severity. And seeing as there are somewhat straightforward
workarounds I guess we'll have to live with my mistakes. Sorry!

/Claes

Re: """error: module testng reads package test from both test and testng"""

[mandy chung]

The test itself is a module that depends on testng.  So testng.jar is 
added as an automatic module.  This test passes on my setup.


I am puzzling how jcommander.jar is added to the module path. Can you 
send the .jtr file and list what's in JTwork/modules directory?


Mandy

On 8/17/17 6:22 PM, Martin Buchholz wrote:

When I run the jtreg test
java/lang/ModuleTests/addXXX
I fail with

direct:
error: module testng reads package test from both test and testng

In the javac command line I see:
  --add-modules testng,jcommander
but ... testng and jcommander aren't modules; they're just garden variety
jar files.

Running jtreg 4.2-b07.

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

[Brent Christian]

Hi, Claes

Fix looks good, test looks good.

Thanks,
-Brent
On 08/21/2017 02:53 AM, Claes Redestad wrote:

Hi,

this patch addresses an unfortunate regression where backtick characters
in a manifest can cause an AIOOBE.

Webrev: http://cr.openjdk.java.net/~redestad/8186334/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8186334

Basically an off-by-one issue during certain steps in the search algorithm,
meaning it is context dependent whether a backtick will trip on this issue
or not.

/Claes

Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants

[Aleksey Shipilev]

On 08/21/2017 08:17 PM, Aleksey Shipilev wrote:
> On 08/21/2017 08:06 PM, Paul Sandoz wrote:
>>> On 21 Aug 2017, at 07:48, Claes Redestad  wrote:
>>> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants 
>>> fails
>>> when providing an Integer as a constant, which appears to be due to failure 
>>> to
>>> coerce boxed types to the corresponding primitive types when looking up 
>>> various
>>> methods in StringConcatHelper:
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186500
>>>
>>> Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive
>>> counterpart, and is a (semantical) no-op for other types, e.g., String.
>>
>> Looks good. Perhaps a token test would be useful (maybe hard to test all 
>> code paths here without some combinator test).
> 
> Please add tests. We have to at least assert that passing "null" works fine 
> there -- I am not sure
> it does right now. Since we are talking non-String constants, passing 
> java/lang/Object is a good
> test too.

I remembered why we haven't covered this before:

 *   \2 (Unicode point 0002): a constant. This input passed
 *   through static bootstrap argument. This constant can be any value
 *   representable in constant pool. If necessary, the factory would call
 *   {@code toString} to perform a one-time String conversion.

Neither Integer, nor Object are representable in constant pool, and so this bug 
cannot surface via
JDK 9 bytecode, but only via direct SCF call. So, from some point of view, SCF 
may exhibit undefined
behavior, including throwing up on non-CP-representable value.

Using "should be the value representable in constant pool" instead of "can..." 
would make it
stronger, but alas. Once other constant flavors are added (/me looks at Paul, 
Brian and John), it
would become a possibility. This is why I think we need to add Integer and 
Object constant tests for
future proofing SCF, while this is still a gray area.

...and maybe add {Class, MethodHandle, MethodType} as the tested arguments too, 
as per:
https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-4.html#jvms-4.4

Thanks for taking care of this!

-Aleksey

Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants

[Aleksey Shipilev]

On 08/21/2017 08:06 PM, Paul Sandoz wrote:
>> On 21 Aug 2017, at 07:48, Claes Redestad  wrote:
>> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants 
>> fails
>> when providing an Integer as a constant, which appears to be due to failure 
>> to
>> coerce boxed types to the corresponding primitive types when looking up 
>> various
>> methods in StringConcatHelper:
>>
>> Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186500
>>
>> Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive
>> counterpart, and is a (semantical) no-op for other types, e.g., String.
> 
> Looks good. Perhaps a token test would be useful (maybe hard to test all code 
> paths here without some combinator test).

Please add tests. We have to at least assert that passing "null" works fine 
there -- I am not sure
it does right now. Since we are talking non-String constants, passing 
java/lang/Object is a good
test too.

-Aleksey

Re: RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants

[Paul Sandoz]

> On 21 Aug 2017, at 07:48, Claes Redestad  wrote:
> 
> Hi,
> 
> a trivial test[1] invoking the StringConcatFactory.makeConcatWithConstants 
> fails
> when providing an Integer as a constant, which appears to be due to failure to
> coerce boxed types to the corresponding primitive types when looking up 
> various
> methods in StringConcatHelper:
> 
> Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8186500
> 
> Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive
> counterpart, and is a (semantical) no-op for other types, e.g., String.
> 

Looks good. Perhaps a token test would be useful (maybe hard to test all code 
paths here without some combinator test).

Paul.

> Thanks!
> 
> /Claes
> 
> [1]
> 
> import java.lang.invoke.*;
> 
> public class Test {
>public static void main(String ... args) throws Exception {
> StringConcatFactory.makeConcatWithConstants(MethodHandles.lookup(), "name",
>MethodType.methodType(String.class, String.class, String.class), 
> "\1\2\1", (Integer)1);
>}
> }
> 

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

[Jonathan Bluett-Duncan]

Would it not be viable to merge this into Java 9 in the foreseeable future
for one of its inevitable bug fix releases? I admit I'm unfamiliar with the
whole process, so I realise my question may be naive or that the answer is
considered "common knowledge".

Cheers,
Jonathan

On 21 August 2017 at 18:41, Claes Redestad 
wrote:

>
>
> On 2017-08-21 19:15, Paul Sandoz wrote:
>
>> On 21 Aug 2017, at 02:53, Claes Redestad 
>>> wrote:
>>>
>>> Hi,
>>>
>>> this patch addresses an unfortunate regression where backtick characters
>>> in a manifest can cause an AIOOBE.
>>>
>>> Webrev: http://cr.openjdk.java.net/~redestad/8186334/jdk.00/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8186334
>>>
>>> Basically an off-by-one issue during certain steps in the search
>>> algorithm,
>>> meaning it is context dependent whether a backtick will trip on this
>>> issue
>>> or not.
>>>
>>> Ooops, looks good.
>>
>
> Thanks for reviewing!
>
>
>> I see a workaround has been pushed to Jackson, which reduced my urge to
>> suggest a nine respin. But… being slightly paranoid testing JarFile on a
>> local maven central mirror would give us a better sense of the impact.
>>
>
> I think the 9 train has left the station even for issues of this severity.
> And seeing as there are somewhat straightforward workarounds I guess we'll
> have to live with my mistakes. Sorry!
>
> /Claes
>

Re: RFR: JDK-8186466: Fix accessibility and other minor issues in java.base

[Jonathan Gibbons]

On 8/21/17 9:46 AM, Martin Buchholz wrote:



On Mon, Aug 21, 2017 at 9:38 AM, Jonathan Gibbons 
> wrote:




On 8/20/17 4:11 PM, Martin Buchholz wrote:

Again, I am happy to take the current state of this change.

On Sat, Aug 19, 2017 at 2:19 PM, Jonathan Gibbons
> wrote:

Actually, thead and tbody have no direct significance for
accessibility. They provide a semantic differentiation of the
content, and provide a hook for different styling, as you
have seen for "striped". Also note, although you can have
many , you can only have at most one , and at
most one .

Looking at Summary of BlockingDeque methods again, we have what
might logically be a thead in the middle of a table, and the law
of "only one thead, and only at the beginning" might be yet
another hint that the html gods want us to split this table. 
This could become a nested table with two rows, one for "first"

and one for "last", each of which contains a subtable with a thead.


I can investigate that.

I would ask, why is this materially different from a new left-most
column in a single table, but I guess one response would be that
the subtables could be striped, which would give visual
consistency with similar tables.


Right - the new theads could get that fashionable #DDD background like 
the other tables.


Yes, that's better than encoding colors in the doc comments, in case we 
want to do style changes in the stylesheet.


-- Jon

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

[Claes Redestad]

On 2017-08-21 19:15, Paul Sandoz wrote:

On 21 Aug 2017, at 02:53, Claes Redestad  wrote:

Hi,

this patch addresses an unfortunate regression where backtick characters
in a manifest can cause an AIOOBE.

Webrev: http://cr.openjdk.java.net/~redestad/8186334/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8186334

Basically an off-by-one issue during certain steps in the search algorithm,
meaning it is context dependent whether a backtick will trip on this issue
or not.


Ooops, looks good.


Thanks for reviewing!



I see a workaround has been pushed to Jackson, which reduced my urge to suggest 
a nine respin. But… being slightly paranoid testing JarFile on a local maven 
central mirror would give us a better sense of the impact.


I think the 9 train has left the station even for issues of this 
severity. And seeing as there are somewhat straightforward workarounds I 
guess we'll have to live with my mistakes. Sorry!


/Claes

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

[Paul Sandoz]

> On 21 Aug 2017, at 02:53, Claes Redestad  wrote:
> 
> Hi,
> 
> this patch addresses an unfortunate regression where backtick characters
> in a manifest can cause an AIOOBE.
> 
> Webrev: http://cr.openjdk.java.net/~redestad/8186334/jdk.00/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8186334
> 
> Basically an off-by-one issue during certain steps in the search algorithm,
> meaning it is context dependent whether a backtick will trip on this issue
> or not.
> 

Ooops, looks good.

I see a workaround has been pushed to Jackson, which reduced my urge to suggest 
a nine respin. But… being slightly paranoid testing JarFile on a local maven 
central mirror would give us a better sense of the impact.

Paul.

> /Claes

Re: RFR: JDK-8133680 add Stream.foldLeft() terminal operation

[Paul Sandoz]

Hi,

My preference, naming-wise, is to stick with the current naming scheme and 
refer to such operations as reduceLeft and reduceRight. We have used reduce 
both for the seed accepting, and optional returning methods, and we don’t use 
different names to distinguish those.

I don’t think we can rely on the sequential() trick, since as Tagir points out 
it is global.

My inclination would be to focus on reduceLeft, and spin off reduceRight into a 
separate issue and in addition ponder support for a reverse operation, since 
reduceRight can be built upon that.

It’s possible to develop a reduceRight or reserve operation that does something 
very similar to forEachOrdered when operating in parallel (the last FJ task has 
to complete before given the ok for it’s direct sibling to the left to proceed 
when ready). For good splitters like ArrayList this might work well, for bad 
splitters like Iterator it will be terrible. It may be possible in some cases 
to fuse reverse().toArray(). IMO pushing reserve further down into a 
characteristic of a Spliterator is going too far at this moment, as that is 
potentially a lot more work. 

(Note naming-wise one could consider reduceOrdered, and then add reverse with 
no need for reduceRight, but i think the names and operations reduceRight/Left 
are likely familiar and i think makes it clear in the source the intent of the 
terminal operation).

So i am supportive of a reduceLeft, and may be supportive of a 
reduceRight+reverse.

In hindsight when we added forEachOrdered i wish i recognised that this was a 
void-returning reduceLeft.

Paul.


> On 19 Aug 2017, at 22:36, Tagir Valeev  wrote:
> 
> Hello, John!
> 
>> I think s.foldLeft(op) is the same as s.sequential().reduce(op).
>> If that's the case, there's no need for it, except in documentation.
> 
> This implementation could be worse for parallel stream. First, as
> Brian already mentioned, forEachOrdered (thus my proposed
> implementation) could still benefit from parallelization, e.g. if
> upstream contains expensive mapping operation. Second, upstream may
> contain full stop operation (e.g. sorted()). In this case everything
> before sorting including sorting itself would be parallelized nicely.
> When you use sequential(), you force all the stream to be sequential,
> killing the parallelism on all the stages.
> 
> Also note that we are speaking about extending the interface, not the
> implementation. It's not specified how exactly reduce(op) must behave
> and third-party implementations in wild may reduce in another order,
> actually relying on op assotiativity. E.g. for stream of four elements
> the implementation is free to reduce like op(op(e1, e2), op(e3, e4)).
> As long as associativity holds, the result will be indistinguishable
> from op(op(op(e1, e2), e3), e4). Using forEachOrdered is much more
> reliable as its behavior is more constrained by documentation.
> 
> Finally this does not solve the problem with more popular version of
> foldLeft which has seed element (possibly of another type). In this
> case you cannot delegate the call to existing reduce without providing
> a bogus combiner (and hoping that it's never called for sequential
> stream, even though it's not specified).
> 
>> A quick, inexpert read of package-info.java suggests to me that
>> the encounter order of a sequential ordered stream is stable,
>> and therefore the result of executing a reduce on such a stream
>> should always produce the same order of operations.  Which
>> order?  Well of course, the one given in the javadoc.  Which
>> is an implementation of your foldLeft, minus the unnecessary
>> buffering.
> 
> My implementation has no unnecessary buffering. It just stores the
> accumulator inside the forEachOrdered consumer. Practically the same
> is made inside reduce implementation. The result of executing a reduce
> could be the same even if operation order is different, because spec
> requires that op must be associative, so een sequential reduce may
> rely on associativity.
> 
> As for foldRight, some considerations about reverse() operation are
> mentioned in Stuart's comment in the issue:
> https://bugs.openjdk.java.net/browse/JDK-8133680?focusedCommentId=13861795=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13861795
> Indeed creating reverse() operation would be nice, but it will require
> much more effort to support in existing spliterators. Also we should
> provide default implementation for Stream interface as well as more
> optimal specific implementation for JDK Stream. This is doable, but of
> course I cannot write it by myself without support from other OpenJDK
> developers (or at least without strong evidence that such feature
> could be accepted).
> 
> With best regards,
> Tagir Valeev.

Re: RFR: JDK-8186466: Fix accessibility and other minor issues in java.base

[Martin Buchholz]

On Mon, Aug 21, 2017 at 9:38 AM, Jonathan Gibbons <
jonathan.gibb...@oracle.com> wrote:

>
>
> On 8/20/17 4:11 PM, Martin Buchholz wrote:
>
> Again, I am happy to take the current state of this change.
>
> On Sat, Aug 19, 2017 at 2:19 PM, Jonathan Gibbons <
> jonathan.gibb...@oracle.com> wrote:
>
>> Actually, thead and tbody have no direct significance for accessibility.
>> They provide a semantic differentiation of the content, and provide a hook
>> for different styling, as you have seen for "striped". Also note, although
>> you can have many , you can only have at most one , and at
>> most one .
>>
> Looking at  Summary of BlockingDeque methods again, we have what might
> logically be a thead in the middle of a table, and the law of "only one
> thead, and only at the beginning" might be yet another hint that the html
> gods want us to split this table.  This could become a nested table with
> two rows, one for "first" and one for "last", each of which contains a
> subtable with a thead.
>
>
> I can investigate that.
>
> I would ask, why is this materially different from a new left-most column
> in a single table, but I guess one response would be that the subtables
> could be striped, which would give visual consistency with similar tables.
>

Right - the new theads could get that fashionable #DDD background like the
other tables.

Re: RFR: JDK-8186466: Fix accessibility and other minor issues in java.base

[Jonathan Gibbons]

On 8/20/17 4:11 PM, Martin Buchholz wrote:

Again, I am happy to take the current state of this change.

On Sat, Aug 19, 2017 at 2:19 PM, Jonathan Gibbons 
> wrote:


Actually, thead and tbody have no direct significance for
accessibility. They provide a semantic differentiation of the
content, and provide a hook for different styling, as you have
seen for "striped". Also note, although you can have many ,
you can only have at most one , and at most one .

Looking at Summary of BlockingDeque methods again, we have what might 
logically be a thead in the middle of a table, and the law of "only 
one thead, and only at the beginning" might be yet another hint that 
the html gods want us to split this table.  This could become a nested 
table with two rows, one for "first" and one for "last", each of which 
contains a subtable with a thead.


I can investigate that.

I would ask, why is this materially different from a new left-most 
column in a single table, but I guess one response would be that the 
subtables could be striped, which would give visual consistency with 
similar tables.


-- Jon

Re: RFR: JDK-8186466: Fix accessibility and other minor issues in java.base

[Naoto Sato]

Thanks for the explanation, Jon. I am fine with the current one.

Naoto

On 8/18/17 5:37 PM, Jonathan Gibbons wrote:

Hi Naoto,

Thanks for checking out the changes.

On 08/18/2017 05:19 PM, Naoto Sato wrote:

Hi Jon,

Thank you for the cleanup! I looked at j.l.String, 
j.t.DateTimeFormatter, and j.u.ResourceBundle and all look good.


BTW, I just noticed that in ResourceBundle, line 1330 and so on, there 
is a new column "Index" introduced. Is this intentional?


Yes. It is intentional, although I'm open to better suggestions for the 
column header, or for other presentations.


Conceptually, the information is a list of pairs.   The text that 
precedes the info describes it as
"the sequence of locale-format combinations to be used to call 
|control.newBundle|. ".
HTML can't do a list of pairs as such, and so a table is used. But for 
accessibility, there must
be some subset of each row which together uniquely define the row, and 
so can be used
as row headers for the data cells in that row.  Typically, the first 
element is the
the unique element.  But that is not so in this case ... worse: there is 
no such column whose
cells define the row. Both columns contain duplicates.  That means the 
only combination
that uniquely defines the row is the entire row, which would mean the 
table would be

all header cells and no data cells!

I went with a solution to add a new column which identifies the position 
in the sequence.
If you don't like the extra column, the alternative would be a bulleted 
or numbered list,
with the two items of the pair separated by commas or some other 
punctuation, as in:


  *   Locale("de", "DE"),     java.class
  *   Locale("de", "DE") ,    java.properties
  *   Locale("de"),     java.class
  *   Locale("de"),     java.properties
  *   Locale(""),     java.class
  *   Locale(""),     java.properties

(In my mail client, that looks like a bulleted list; I don't know how 
well it will travel

through the mail system.)

-- Jon



Naoto

On 8/18/17 5:03 PM, Jonathan Gibbons wrote:
Please review these fixes for various minor documentation issues in 
the java.base module.
The changes are mostly in java.util and its subpackages, but there is 
some minor cleanup

in previously updated packages as well.

The primary focus is on addressing accessibility issues. In addition, 
some doc files have
been converted to HTML5. As with all recent fixes like this, there 
should be no change to

the underlying specifications.

JBS:https://bugs.openjdk.java.net/browse/JDK-8186466
Webrev:http://cr.openjdk.java.net/~jjg/8186466/webrev.00/index.html
API:http://cr.openjdk.java.net/~jjg/8186466/api.00/overview-summary.html

-- Jon, the Javadoc Janitor


Here are more detailed notes on the changes:

src/java.base/share/classes/java/lang/String.java
 Some greek text that previously used discrete image files for 
the characters
 has been updated to use Unicode characters, specified with HTML 
entities.
 All related image files in the doc-files subdirectory have now 
been removed.


src/java.base/share/classes/java/lang/doc-files/ValueBased.html
 The file is trivially updated to HTML 5.

src/java.base/share/classes/java/lang/doc-files/threadPrimitiveDeprecation.html 


 The file is updated to HTML 5.

src/java.base/share/classes/java/time/format/DateTimeFormatter.java
 Two missing quote marks are added.
 The quotes are regrettably necessary: some of the examples 
contain spaces,

 and some cells have more than one example,

src/java.base/share/classes/java/util/Deque.java
 The tables are made accessible.
 Where reasonable, the tables are converted to the de-facto standard
 "striped" style.

src/java.base/share/classes/java/util/Queue.java
 A table is made accessible, and converted to the de-facto standard
 "striped" style.

src/java.base/share/classes/java/util/ResourceBundle.java
 A preformatted list is converted to a semantic list.
 The tables are made accessible.
 Where reasonable, the tables are converted to the de-facto standard
 "striped" style.
 Line 3458 and following. Previously, the comment did not display 
correctly
 because javadoc incorrectly treated the dot in this string 
"'.'"
 as the end of the first sentence! The minimal fix would be to 
change that

 string to "{@code .}", but that would be stylistically inconsistent
 with the rest of the comment. There are too many occurrences of
 ... in the file as a whole to change them all at 
this time,
 so the compromise is just to replace the occurrences in this 
comment.

 Introducing more cleanup for existing uses of ... is a
 topic for another day.

src/java.base/share/classes/java/util/concurrent/BlockingDeque.java
 The tables are made accessible.
 In the second table, the "Insert"/"Remove"/"Examine" headings
 are moved from embedded rows to a new left-most column
 for consistency with other similar 

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Martin Buchholz]

Thanks.  Approved.

I missed that buf was protected and so the javadoc here actually matters.
Having proper javadoc here for the meaning of null buf (including for
subclasses) "should" be done, but out of scope for this change.

On Mon, Aug 21, 2017 at 8:09 AM, Claes Redestad 
wrote:

> Hi Martin,
>
>
> On 08/21/2017 04:33 PM, Martin Buchholz wrote:
>
>> This comment still seems applicable. Can we move it into the docstring
>> for buf?
>>
>> We use nullness
>> - * of buf[] as primary indicator that this stream is closed. (The
>> - * "in" field is also nulled out on close.)
>>
>>
> this is a reasonable request, the catch is that the field is protected and
> thus visible
> in javadoc, so it needs to be updated as a CSR(?). Perhaps we can move it
> to a regular
> comment associated with the field to avoid this implementation detail
> showing up in
> javadocs?
>
> http://cr.openjdk.java.net/~redestad/8185362/jdk.02/
>
> Thanks!
>
> /Claes
>

Re: RFR(s) #2: 6344935: (spec) clarify specifications for Object.wait overloads

[Martin Buchholz]

On Sun, Aug 20, 2017 at 9:53 PM, David Holmes 
wrote:

> On 21/08/2017 2:07 PM, Martin Buchholz wrote:
>
>> On Sun, Aug 20, 2017 at 6:36 PM, David Holmes > > wrote:
>>
>> On 20/08/2017 6:37 AM, Martin Buchholz wrote:
>>
>> Future projects:
>>
>> 377 * The specified amount of real time has elapsed, more or
>> less.
>>
>> Replace with
>>
>> * At least the specified amount of real time has elapsed.
>>
>> (I think I failed to persuade David last time ...)
>>
>>
>> And you will continue to do so. :) In the presence of spurious
>> wakeups it is completely untestable to say "at least the specified
>> time has elapsed"
>>
>>
>> My view is that spurious wakeup is a separate item in this list. "The
>> specified amount of real time has elapsed" should only be about "normal"
>> timeout.
>>
>
> And if you could perform whitebox testing that peeks inside to be able to
> make the distinction that would be fine. But in terms of the API and any
> conformance test, it is impossible to distinguish between an "early return"
> and a "spurious return". So specifying "at least ..." is meaningless so
> long as spurious wakeups are allowed.
>

The testability difference is that regular timeout is "normal" whereas
spurious wakeup must be "rare" (although of course "rare" is hard to pin
down).
Before we fixed
8065372: Object.wait(ms, ns) timeout returns early

returning early was unacceptably common

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Xueming Shen]

+1

On 8/21/17, 8:09 AM, Claes Redestad wrote:

Hi Martin,

On 08/21/2017 04:33 PM, Martin Buchholz wrote:
This comment still seems applicable. Can we move it into the 
docstring for buf?


We use nullness
- * of buf[] as primary indicator that this stream is closed. (The
- * "in" field is also nulled out on close.)



this is a reasonable request, the catch is that the field is protected 
and thus visible
in javadoc, so it needs to be updated as a CSR(?). Perhaps we can move 
it to a regular
comment associated with the field to avoid this implementation detail 
showing up in

javadocs?

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

Thanks!

/Claes

Re: RFR(s) #2: 6344935: (spec) clarify specifications for Object.wait overloads

[Martin Buchholz]

My attempt to write a good wait loop led me to want only to use a
nanosecond wait method.  And in fact we have existing method
Condition.awaitNanos for inspiration.  For a moment I considered adding
Object#waitNanos but then I came to my senses.  But we could add a static
method Objects#waitNanos and that could be the recommended method to call
inside all wait loops?  Maybe it's time to replace JVM_MonitorWait (that
takes millis) with a version that takes nanos?  Object.c has not been
touched since 2003!

On Sun, Aug 20, 2017 at 8:05 PM, Martin Buchholz 
wrote:

>
>
> On Sat, Aug 19, 2017 at 1:53 PM, Martin Buchholz 
> wrote:
>
>> Now I see that the code snippet in TimeUnit.timedWait is also in need of 
>> fixing. H 
>>
>>  public synchronized Object poll(long timeout, TimeUnit unit)
>>  throws InterruptedException {
>>while (empty) {
>>  unit.timedWait(this, timeout);
>>  ...
>>}
>>  }
>>
>> It's surprisingly hard to write a high quality wait loop, and we've been
> doing our users a disservice by punting on providing good models in the
> javadoc.  You probably want to work entirely in nanoseconds, to avoid
> rounding errors, and because you'll end up calling nanoTime anyways.  You
> don't want to call nanoTime unless you are sure to wait.  OTOH you want to
> throw NPE on null TimeUnit even if there's no need to wait.  You must
> beware of overflow when timeout is Long.MIN_VALUE.
>
> Below is my attempt to fix the sample loop in timedWait.  Calling
> Object.wait directly is obviously even harder.  How about just not
> providing a sample loop that calls Object.wait directly (effectively,
> gently deprecate timed Object wait in favor of the loop below)  (I'm also
> pretending that the platform has better than millisecond granularity).
>
>  public E poll(long timeout, TimeUnit unit)
>  throws InterruptedException {
>long nanosTimeout = unit.toNanos(timeout);
>synchronized (lock) {
>  for (long deadline = 0L; isEmpty(); ) {
>long remaining = (deadline == 0L) ? nanosTimeout
>  : deadline - System.nanoTime();
>if (remaining <= 0)
>  return null;
>if (deadline == 0L) { // first wait
>  deadline = System.nanoTime() + nanosTimeout;
>  if (deadline == 0L) // unlikely corner case
>deadline = 1L;
>}
>NANOSECONDS.timedWait(lock, remaining);
>  }
>  return dequeueElement();
>}
>  }
>
>

Re: RFR(s) #2: 6344935: (spec) clarify specifications for Object.wait overloads

[Martin Buchholz]

Using a boolean flag is reasonable, but there are micro-defects here as
well:
- There's a dead initial store to deadline just to appease the definite
assignment algorithm.
- the scope of deadline and remaining leaks into the post-wait-loop code,
and it would be ugly to introduce a new scope explicitly
- if you wait, you will call System.nanoTime() one more time than
necessary.  A high quality wait loop only ever calls nanoTime just before
waiting.

On Mon, Aug 21, 2017 at 12:34 AM, Peter Levart 
wrote:

> Hi Martin,
>
>
> On 08/21/2017 05:05 AM, Martin Buchholz wrote:
>
>> On Sat, Aug 19, 2017 at 1:53 PM, Martin Buchholz 
>> wrote:
>>
>> Now I see that the code snippet in TimeUnit.timedWait is also in need of
>>> fixing. H 
>>>
>>>   public synchronized Object poll(long timeout, TimeUnit unit)
>>>   throws InterruptedException {
>>> while (empty) {
>>>   unit.timedWait(this, timeout);
>>>   ...
>>> }
>>>   }
>>>
>>> It's surprisingly hard to write a high quality wait loop, and we've been
>>>
>> doing our users a disservice by punting on providing good models in the
>> javadoc.  You probably want to work entirely in nanoseconds, to avoid
>> rounding errors, and because you'll end up calling nanoTime anyways.  You
>> don't want to call nanoTime unless you are sure to wait.  OTOH you want to
>> throw NPE on null TimeUnit even if there's no need to wait.  You must
>> beware of overflow when timeout is Long.MIN_VALUE.
>>
>> Below is my attempt to fix the sample loop in timedWait.  Calling
>> Object.wait directly is obviously even harder.  How about just not
>> providing a sample loop that calls Object.wait directly (effectively,
>> gently deprecate timed Object wait in favor of the loop below)  (I'm also
>> pretending that the platform has better than millisecond granularity).
>>
>>   public E poll(long timeout, TimeUnit unit)
>>   throws InterruptedException {
>> long nanosTimeout = unit.toNanos(timeout);
>> synchronized (lock) {
>>   for (long deadline = 0L; isEmpty(); ) {
>> long remaining = (deadline == 0L) ? nanosTimeout
>>   : deadline - System.nanoTime();
>> if (remaining <= 0)
>>   return null;
>> if (deadline == 0L) { // first wait
>>   deadline = System.nanoTime() + nanosTimeout;
>>   if (deadline == 0L) // unlikely corner case
>> deadline = 1L;
>> }
>> NANOSECONDS.timedWait(lock, remaining);
>>   }
>>   return dequeueElement();
>> }
>>   }
>>
>
> You could split the state of "first wait" vs. "deadline" into separate
> locals. It would be easier to understand and no corner case to watch for.
> For example:
>
> public E poll(long timeout, TimeUnit unit)
> throws InterruptedException {
> long nanosTimeout = unit.toNanos(timeout);
> long deadline = 0;
> long remaining = nanosTimeout;
> synchronized (lock) {
> for (boolean first = true; isEmpty();) {
> if (remaining <= 0)
> return null;
> if (first) { // before first wait
> deadline = System.nanoTime() + nanosTimeout;
> first = false;
> }
> NANOSECONDS.timedWait(lock, remaining);
> remaining = deadline - System.nanoTime();
> }
> return dequeueElement();
> }
> }
>
>
> Regards, Peter
>
>

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Claes Redestad]

Hi Martin,

On 08/21/2017 04:33 PM, Martin Buchholz wrote:
This comment still seems applicable. Can we move it into the docstring 
for buf?


We use nullness
- * of buf[] as primary indicator that this stream is closed. (The
- * "in" field is also nulled out on close.)



this is a reasonable request, the catch is that the field is protected 
and thus visible
in javadoc, so it needs to be updated as a CSR(?). Perhaps we can move 
it to a regular
comment associated with the field to avoid this implementation detail 
showing up in

javadocs?

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

Thanks!

/Claes

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Claes Redestad]

On 08/21/2017 04:47 PM, Peter Levart wrote:


Is BufferedInputStream.close() intentionally not synchronized? All 
other methods are. If close() was synchronized too, no CAS would be 
needed and fields could be normal, not volatile. What is achieved by 
close() not being synchronized? Fear of deadlocks? 


I don't have the history here, but my gut-feeling is it's intentional to 
allow calling close on streams that are blocked or waiting for data from 
another thread in a non-blocking (or even deadlocking) fashion


/Claes

RFR: 8186500: StringConcatFactory.makeConcatWithConstants throws AssertionError when recipe contains non-String constants

[Claes Redestad]

Hi,

a trivial test[1] invoking the 
StringConcatFactory.makeConcatWithConstants fails
when providing an Integer as a constant, which appears to be due to 
failure to
coerce boxed types to the corresponding primitive types when looking up 
various

methods in StringConcatHelper:

Webrev: http://cr.openjdk.java.net/~redestad/8186500/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8186500

Simply using Wrapper.asPrimitiveType coerces boxed types to their primitive
counterpart, and is a (semantical) no-op for other types, e.g., String.

Thanks!

/Claes

[1]

import java.lang.invoke.*;

public class Test {
public static void main(String ... args) throws Exception {
StringConcatFactory.makeConcatWithConstants(MethodHandles.lookup(), "name",
MethodType.methodType(String.class, String.class, 
String.class), "\1\2\1", (Integer)1);

}
}

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Peter Levart]

Hi Claes,

On 08/21/2017 01:15 PM, Claes Redestad wrote:

On 08/21/2017 12:29 PM, Aleksey Shipilev wrote:

On 08/21/2017 12:20 PM, Claes Redestad wrote:

Webrev: http://cr.openjdk.java.net/~redestad/8185362/jdk.00/
*) Should be static *final*, otherwise you miss constant folding for 
Unsafe accesses:


   66 private static long BUF_OFFSET = 
U.objectFieldOffset(BufferedInputStream.class, "buf");


*) While you are at it, maybe switch to proper Java style here, e.g. 
"volatile byte[] buf"?


   73 protected volatile byte buf[];


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

/Claes



Just a side question...

Is BufferedInputStream.close() intentionally not synchronized? All other 
methods are. If close() was synchronized too, no CAS would be needed and 
fields could be normal, not volatile. What is achieved by close() not 
being synchronized? Fear of deadlocks?


Regards, Peter

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Martin Buchholz]

This comment still seems applicable.  Can we move it into the docstring for
buf?

We use nullness
- * of buf[] as primary indicator that this stream is closed. (The
- * "in" field is also nulled out on close.)

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Aleksey Shipilev]

On 08/21/2017 01:15 PM, Claes Redestad wrote:
> Done: http://cr.openjdk.java.net/~redestad/8185362/jdk.01/

Looks good.

-Aleksey

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Claes Redestad]

On 08/21/2017 12:29 PM, Aleksey Shipilev wrote:

On 08/21/2017 12:20 PM, Claes Redestad wrote:

Webrev: http://cr.openjdk.java.net/~redestad/8185362/jdk.00/

*) Should be static *final*, otherwise you miss constant folding for Unsafe 
accesses:

   66 private static long BUF_OFFSET = U.objectFieldOffset(BufferedInputStream.class, 
"buf");

*) While you are at it, maybe switch to proper Java style here, e.g. "volatile 
byte[] buf"?

   73 protected volatile byte buf[];


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

/Claes

Re: RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Aleksey Shipilev]

On 08/21/2017 12:20 PM, Claes Redestad wrote:
> Webrev: http://cr.openjdk.java.net/~redestad/8185362/jdk.00/

*) Should be static *final*, otherwise you miss constant folding for Unsafe 
accesses:

  66 private static long BUF_OFFSET = 
U.objectFieldOffset(BufferedInputStream.class, "buf");

*) While you are at it, maybe switch to proper Java style here, e.g. "volatile 
byte[] buf"?

  73 protected volatile byte buf[];

Thanks,
-Aleksey

RFR [10]: 8185362: Replace use of AtomicReferenceFieldUpdater from BufferedInputStream with Unsafe

[Claes Redestad]

Hi,

by using Unsafe.compareAndSetObject instead of AtomicReferenceFieldUpdater
in BufferedInputStream we can avoid some classloading and initialization 
work

during System.initPhase1.

Webrev: http://cr.openjdk.java.net/~redestad/8185362/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8185362

/Claes

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

[Claes Redestad]

Hi,

this patch addresses an unfortunate regression where backtick characters
in a manifest can cause an AIOOBE.

Webrev: http://cr.openjdk.java.net/~redestad/8186334/jdk.00/
Bug: https://bugs.openjdk.java.net/browse/JDK-8186334

Basically an off-by-one issue during certain steps in the search algorithm,
meaning it is context dependent whether a backtick will trip on this issue
or not.

/Claes

Re: [10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean

[Doug Simon]

> On 21 Aug 2017, at 10:48, Jaroslav Tulach  wrote:
> 
> Hello Doug & co.,
> I think your comments are about the old webrev. I hope most of them have been 
> (by a chance) addressed in the newest webrev 01:

Indeed they are. Sorry again for missing the latest link when do the review ;-)

-Doug

Re: [10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean

[Jaroslav Tulach]

Hello Doug & co.,
I think your comments are about the old webrev. I hope most of them have been 
(by a chance) addressed in the newest webrev 01:

http://cr.openjdk.java.net/~kvn/8182701/webrev.01/

On pátek 18. srpna 2017 21:11:28 CEST Doug Simon wrote:
> I don't think JVMCIMXBeans should be in the hotspot-agnostic
> jdk.vm.ci.services.internal package since it has a direct reference to
> HotSpotJVMCIRuntime. I would suggest moving it to jdk.vm.ci.hotspot.

Mandy already suggested to put the JVMCIMBeans class into its own module with 
dependencies on jdk.management and jdk.vm.internal.ci modules. I think it is 
the cleanest solution from the point of modular architecture. The suggested 
module can be found in the webrev.01 under the name jdk.vm.ci.management.

> In HotSpotJVMCRuntime.java:
> 
>  558 public String mbeanName() {
>  568 public Object mbean() {
> 
> Any reason these methods don't follow the conventions of other getter
> methods in this class (i.e. getMBeanName() and getMBean())?

These two methods have been replaced by 

/**
 * Provides compiler specific platform MBeans. These MBeans will be 
automatically
 * exposed once the management system gets initialized.
 *
 * @return map from MBean names to their instances
 */
public Map mbeans() {
return Collections.emptyMap();
}

in the webrev.01. You are right, the name of the method could be getMBeans(). 
Let me know if I should rename that and also ...

> 
>   95 /** Name of the {@link MBeanInfo MBean} representing the internals
> 
> This should be:
> 
> /**
>  * Gets the name of the ...

...please give me a better version of Javadoc, in case the current 
"Provides..." isn't sufficient.

> Same comment for mbean() method. Also {@code null} is used in JVMCI instead
> of null.

The webrev.01 version never returns null. Instead Collections.emptyMap() is 
returned by default. Thus this comment no longer applies.

Thanks for your comments.
-jt

> > On 18 Aug 2017, at 20:49, Vladimir Kozlov 
> > wrote:
> > 
> > Updated changes in all repos:
> > http://cr.openjdk.java.net/~kvn/8182701/webrev.01/
> > 
> > On 8/18/17 7:12 AM, Jaroslav Tulach wrote:
> > 
> > Thanks for pushing me forward, Vladimir. Yes, the changes are still needed
> > if we want the Graal compiler to expose its MBean in a lazy way. I am
> > offering new webrev for review. It contains following changes:
> > 
> > Per Mandy's suggestion I created new module jdk.vm.ci.management to bridge
> > between JVMCI and jdk.management. Adding new module was a bit tricky, but
> > with great help of Jan Lahoda I even managed to register it as a boot
> > module.
> > 
> > I renamed the JVMCIMXBean class and dropped X per Vladimir's advice. I
> > fixed the non-standard location of JVMCIMXBean class.
> > 
> > I changed the interface to use Map, so the compiler is able to expose more
> > than a single bean.
> > 
> > That's it. I am looking forward to your review comments.
> > -jt
> > 
> > Here are original changes for reference:
> > 
> > http://cr.openjdk.java.net/~kvn/8182701/webrev.jdk/
> > http://cr.openjdk.java.net/~kvn/8182701/webrev.hs/
> > 
> > On 8/17/17 11:54 AM, Vladimir Kozlov wrote:
> >> Hi Jaroslav,
> >> What we should do with 8182701? Do you still need JVMCI changes?
> >> Note, your changes to Graal [GR-5435] were integrated recently into JDK
> >> (jdk10/hs): https://bugs.openjdk.java.net/browse/JDK-8186158
> >> Thanks,
> >> Vladimir
> >> 
> >> On 8/14/17 10:06 AM, Jaroslav Tulach wrote:
> >>> On čtvrtek 3. srpna 2017 17:03:39 CEST Jaroslav Tulach wrote:
>  On čtvrtek 27. července 2017 15:01:17 CEST Alan Bateman wrote:
> > On 27/07/2017 10:07, Jaroslav Tulach wrote:
> >> Yes, it seems like a desirable goal to let Graal compiler work with
> >> just
> >> java.base. Is there a description how to build JDK9/10 with just
> >> java.base
> >> that I could follow and test against?
> > 
> > You can use jlink to create a run-time image that only contains
> > java.base (jlink --module-path $JAVA_HOME/jmods --add-modules
> > java.base
> > --output smalljre).
>  
>  Status update: I've just tried to run Graal compiler against JDK9 with
>  only
>  java.base and jdk.internal.vm.ci modules, and there are some problems.
>  I
>  need to resolve them first before I provide updated version of my
>  patch.
> >>> 
> >>> FYI: As of
> >>> https://github.com/graalvm/graal/commit/ca9071941a1be7f1a3725529ecc231ff
> >>> 621d5ed0 the Graal compiler can run with java.base, jdk.unsupported and
> >>> jdk.vm.ci only modules. But it wasn't easy, especially the
> >>> http://wiki.apidesign.org/wiki/PropertyChangeListener required a bit of
> >>> work.
> >>> 
> >>> -jt

Re: RFR(s) #2: 6344935: (spec) clarify specifications for Object.wait overloads

[Peter Levart]

Hi Martin,

On 08/21/2017 05:05 AM, Martin Buchholz wrote:

On Sat, Aug 19, 2017 at 1:53 PM, Martin Buchholz 
wrote:


Now I see that the code snippet in TimeUnit.timedWait is also in need of 
fixing. H 

  public synchronized Object poll(long timeout, TimeUnit unit)
  throws InterruptedException {
while (empty) {
  unit.timedWait(this, timeout);
  ...
}
  }

It's surprisingly hard to write a high quality wait loop, and we've been

doing our users a disservice by punting on providing good models in the
javadoc.  You probably want to work entirely in nanoseconds, to avoid
rounding errors, and because you'll end up calling nanoTime anyways.  You
don't want to call nanoTime unless you are sure to wait.  OTOH you want to
throw NPE on null TimeUnit even if there's no need to wait.  You must
beware of overflow when timeout is Long.MIN_VALUE.

Below is my attempt to fix the sample loop in timedWait.  Calling
Object.wait directly is obviously even harder.  How about just not
providing a sample loop that calls Object.wait directly (effectively,
gently deprecate timed Object wait in favor of the loop below)  (I'm also
pretending that the platform has better than millisecond granularity).

  public E poll(long timeout, TimeUnit unit)
  throws InterruptedException {
long nanosTimeout = unit.toNanos(timeout);
synchronized (lock) {
  for (long deadline = 0L; isEmpty(); ) {
long remaining = (deadline == 0L) ? nanosTimeout
  : deadline - System.nanoTime();
if (remaining <= 0)
  return null;
if (deadline == 0L) { // first wait
  deadline = System.nanoTime() + nanosTimeout;
  if (deadline == 0L) // unlikely corner case
deadline = 1L;
}
NANOSECONDS.timedWait(lock, remaining);
  }
  return dequeueElement();
}
  }


You could split the state of "first wait" vs. "deadline" into separate 
locals. It would be easier to understand and no corner case to watch 
for. For example:


public E poll(long timeout, TimeUnit unit)
throws InterruptedException {
long nanosTimeout = unit.toNanos(timeout);
long deadline = 0;
long remaining = nanosTimeout;
synchronized (lock) {
for (boolean first = true; isEmpty();) {
if (remaining <= 0)
return null;
if (first) { // before first wait
deadline = System.nanoTime() + nanosTimeout;
first = false;
}
NANOSECONDS.timedWait(lock, remaining);
remaining = deadline - System.nanoTime();
}
return dequeueElement();
}
}


Regards, Peter