RFR (JDK 12/java.base) 8213325: (props) Properties.loadFromXML does not fully comply with the spec

[Joe Wang]

Hi,

Please review a patch to bring the small footprint parser for 
java.util.Properties compliant with the specification.


JBS: https://bugs.openjdk.java.net/browse/JDK-8213325
webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev/

Thanks,
Joe

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[David Holmes]

On 14/11/2018 12:40 am, Brian Goetz wrote:
But don’t they?  We intend to start generating condy in classfiles from 
javac soon; at that point, anyone not using ASM7 will fail when reading 
classfiles generated by javac.


See later email. They need this for nestmates now as well.

Cheers,
David

On Nov 8, 2018, at 4:03 AM, David Holmes > wrote:


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features? If 
so then you could split out the actual update of ASM from the updates 
to the users of ASM (some of which may be quite fine with ASM5).

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi Igor,

Thanks for your comment. I have already applied in my local copy.

Vicente

On 11/13/18 8:30 PM, Igor Ignatyev wrote:

Hi Vicente,

you need to replace "@ignore 8194951: some mlvm tests fail w/ ASMv7" w/ "@ignore 
8194951" in all the occurrences, as we have monitoring tools which expect @ignore to be 
followed by a space-separated list of bug ids. the rest looks good to me.

Thanks,
-- Igor


On Nov 13, 2018, at 5:11 PM, Vicente Romero  wrote:

Hi Alan,

On 11/13/18 9:18 AM, Alan Bateman wrote:

On 13/11/2018 14:00, Vicente Romero wrote:

any other comment after the last iteration? we are in a bit of a hurry to push 
this before the JDK 12 train departs :(

The original patch updated all the use sites (and tests) to specify ASM7 for 
the API version. I just checked the webrev again now and it seems to be just 
the ASM refresh now. Assuming all the tests are passing and you've sorted out 
the mvlm test issues with Igor then I suggest go ahead with this push and we 
can update the sites, as needed, at a later time.

in the last update I sent links to two patches [1] is the ASM7 only changes and 
[2] is the changes to the use sites. My plan is to push both together, but I 
split them to ease the review process. But still I will get your go and push it 
as good ;)


-Alan

Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Igor Ignatyev]

Hi Vicente,

you need to replace "@ignore 8194951: some mlvm tests fail w/ ASMv7" w/ 
"@ignore 8194951" in all the occurrences, as we have monitoring tools which 
expect @ignore to be followed by a space-separated list of bug ids. the rest 
looks good to me.

Thanks,
-- Igor

> On Nov 13, 2018, at 5:11 PM, Vicente Romero  wrote:
> 
> Hi Alan,
> 
> On 11/13/18 9:18 AM, Alan Bateman wrote:
>> On 13/11/2018 14:00, Vicente Romero wrote:
>>> any other comment after the last iteration? we are in a bit of a hurry to 
>>> push this before the JDK 12 train departs :(
>> The original patch updated all the use sites (and tests) to specify ASM7 for 
>> the API version. I just checked the webrev again now and it seems to be just 
>> the ASM refresh now. Assuming all the tests are passing and you've sorted 
>> out the mvlm test issues with Igor then I suggest go ahead with this push 
>> and we can update the sites, as needed, at a later time.
> 
> in the last update I sent links to two patches [1] is the ASM7 only changes 
> and [2] is the changes to the use sites. My plan is to push both together, 
> but I split them to ease the review process. But still I will get your go and 
> push it as good ;)
> 
>> 
>> -Alan
> 
> Vicente
> 
> [1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
> [2] 
> http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/
>  

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Weijun Wang]

Confused. Aren't all Security properties security-related? This is not about 
normal system properties.

And the method name in the latest webrev is "isSecurityProperty" without the 
"JDK" word. I assume this means you don't care about the difference between SE 
properties and JDK properties.

--Max

> On Nov 14, 2018, at 2:53 AM, Sean Mullan  wrote:
> 
> * src/java.base/share/classes/java/security/Security.java
> 
> The isJdkSecurityProperty method could return false positives, for example 
> there may be a non-JDK property starting with "security.". I was thinking it 
> would be better to put all the JDK property names in a HashSet which is 
> populated by the static initialize() method, and only if event logging is 
> enabled. Then setProperty can just check if the property name is in this set.

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi Alan,

On 11/13/18 9:18 AM, Alan Bateman wrote:

On 13/11/2018 14:00, Vicente Romero wrote:
any other comment after the last iteration? we are in a bit of a 
hurry to push this before the JDK 12 train departs :(
The original patch updated all the use sites (and tests) to specify 
ASM7 for the API version. I just checked the webrev again now and it 
seems to be just the ASM refresh now. Assuming all the tests are 
passing and you've sorted out the mvlm test issues with Igor then I 
suggest go ahead with this push and we can update the sites, as 
needed, at a later time.


in the last update I sent links to two patches [1] is the ASM7 only 
changes and [2] is the changes to the use sites. My plan is to push both 
together, but I split them to ease the review process. But still I will 
get your go and push it as good ;)




-Alan


Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/ 

Re: RFR: XXXS fix 1-char typo in doc comment; JDK-8213820: unknown tag: @returns

[Mandy Chung]

+1

Mandy


On 11/13/18 4:12 PM, Jonathan Gibbons wrote:
Please review this trivial fix for an incorrectly spelt tag name in 
public documentation.


$ hg diff -R open
diff -r bbbcd90f0adb 
src/java.base/share/classes/java/lang/ref/Reference.java
--- a/src/java.base/share/classes/java/lang/ref/Reference.java Tue Nov 
13 16:49:58 2018 -0500
+++ b/src/java.base/share/classes/java/lang/ref/Reference.java Tue Nov 
13 16:03:21 2018 -0800

@@ -379,7 +379,7 @@
  * Throws {@link CloneNotSupportedException}. A {@code Reference} 
cannot be

  * meaningfully cloned. Construct a new {@code Reference} instead.
  *
- * @returns never returns normally
+ * @return never returns normally
  * @throws  CloneNotSupportedException always
  *
  * @since 11

(Separately, there are a depressing number of "@params" and even a 
"@returnss" in comments for private members or internal classes!)


-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8213820

RFR: XXXS fix 1-char typo in doc comment; JDK-8213820: unknown tag: @returns

[Jonathan Gibbons]

Please review this trivial fix for an incorrectly spelt tag name in 
public documentation.


$ hg diff -R open
diff -r bbbcd90f0adb 
src/java.base/share/classes/java/lang/ref/Reference.java
--- a/src/java.base/share/classes/java/lang/ref/Reference.java  Tue Nov 
13 16:49:58 2018 -0500
+++ b/src/java.base/share/classes/java/lang/ref/Reference.java  Tue Nov 
13 16:03:21 2018 -0800

@@ -379,7 +379,7 @@
  * Throws {@link CloneNotSupportedException}. A {@code Reference} 
cannot be

  * meaningfully cloned. Construct a new {@code Reference} instead.
  *
- * @returns never returns normally
+ * @return never returns normally
  * @throws  CloneNotSupportedException always
  *
  * @since 11

(Separately, there are a depressing number of "@params" and even a 
"@returnss" in comments for private members or internal classes!)


-- Jon

JBS: https://bugs.openjdk.java.net/browse/JDK-8213820

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Philip Race]

On 11/13/18, 12:52 PM, Roger Riggs wrote:

Hi,

There are enough files unique to each platform to put them in separate 
packages

otherwise you get too many (IMHO) files in a single package/directory and
its harder to tell which go with which.  There isn't much of a problem 
with

classes being public because they are all in a module and not exported.

I would put them all under share/classes/jdk/jpackagers/internal/ and
save a directory level.


That isn't how the rest of the JDK is organised.
Platform specific classes are split where you have "share" in the path 
above.

So whilst the other issues are more arguable I don't think the build team
would like platform specific classes under share. There is already an
objection to that for the native files.

To the "too many files in one package/directory" point.
I think that might happen at the directory level if Andy went through with
his suggestion but I don't think it will happen with what I proposed and
we probably should get some benefit from being able to make classes + 
methods

package private.

So I think what I proposed is about right ..

phil


$.02, Roger


On 11/13/2018 03:46 PM, Andy Herrick wrote:

I agree with this and would take it further.

1 file is in ./share/classes/jdk/jpackager/internal/builders - why 
not just ./share/classes/jdk/jpackager/internal


2 files are in ./share/classes/jdk/jpackager/internal/bundlers - why 
not just in ./share/classes/jdk/jpackager/internal


1 file is in ./linux/classes/jdk/jpackager/internal/builders/linux - 
why not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - 
why not just ./macosx/classes/jdk/jpackager/internal


1 file is in 
./windows/classes/jdk/jpackager/internal/builders/windows - why not 
just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything 
would be easier to find, and we could make almost all methods 
package-private (the only exception would be the few things called by 
Main, and the ToolProvider.



On 11/13/2018 2:54 PM, Phil Race wrote:
Question .. why is "mac", "linux" and "windows" necessary in the 
package name here ?


 src/jdk.jpackager/macosx/classes/jdk/jpackager/internal/mac/MacAppBundler.java 

 src/jdk.jpackager/windows/classes/jdk/jpackager/internal/builders/windows/WindowsAppImageBuilder.java 

src/jdk.jpackager/linux/classes/jdk/jpackager/internal/linux/LinuxRpmBundler.java 



There's not likely to be a clash, so is there some other reason not 
to want these

in the same package as the shared internals like
src/jdk.jpackager/share/classes/jdk/jpackager/internal/Param.java

?

-phil.


I agree with this and would take it further.

1 file is in ./share/classes/jdk/jpackager/internal/builders - why 
not just ./share/classes/jdk/jpackager/internal


2 files are in ./share/classes/jdk/jpackager/internal/bundlers - why 
not just in ./share/classes/jdk/jpackager/internal


1 file is in ./linux/classes/jdk/jpackager/internal/builders/linux - 
why not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - 
why not just ./macosx/classes/jdk/jpackager/internal


1 file is in 
./windows/classes/jdk/jpackager/internal/builders/windows - why not 
just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything 
would be easier to find, and we could make almost all methods 
package-private (the only exception would be the few things called by 
Main, and the ToolProvider.


/Andy

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Sverre Moe]

Hello,
May I chime in a little on the jpackager.
I have been using it with OpenJDK 11, as backported by Johan Vos from Gluon.
It has worked fine, but I have noticed some flaws.

1)
The control file for DEB package does not set correct description

--name test
--description This is a Test Application

/tmp/jdk.packager607148779833718376/linux/control
Package: test
Description: test

The RPM gets it correctly
Summary : test
Description :
This is a Test Application

2)
Category is not set on either DEB or RPM
  --category
  Category or group of the application.
--category "Some/Category/Application"
Group: Unspecified
Section: unknown

Perhaps I have misunderstood what this category is for, because I see this
it is set in the generated application.desktop, but It should definitely
also be set in the RPM and DEB.

3)
There should be a --release flag to jpackager, at least for RPM. The only
other way to set release would be to supply a custom spec file.
Version: 1.0.0
Release: RC1


/Sverre


Den tir. 13. nov. 2018 kl. 22:30 skrev Andy Herrick :

>
> On 11/13/2018 4:08 PM, Roger Riggs wrote:
> > Hi,
> >
> > A few high level comments:
> >
> > The JDK already has a command option parser (JoptSimple) in the module
> > jdk.internal.opt
> > and the System Logger.  Why not use them for the argument parsing and
> > logging?
>
> We have an RFE to convert argument parsing to joptsimple that I filed
> last July JDK-8208300. 
>
> I spent a few days at the time prototyping it, and concluded it was a
> multi-week project.
>
> The team decided it was not a priority for JDK12 and it is now targeted
> to JDK13.
>
> >
> > Similarly, we've been encouraging developers to use the java.nio.file
> > APIs and
> > get away from java.io.File.  Try-with-resources could be used in a few
> > places
> > to improve the closing of resources.
> >
> > I can also see many places where the Streams functions could be used
> > to make the code easier to read (and write).  That's a missed
> > opportunity.
> >
> > What granularity of comments are you looking for?
>
> We are looking for three types of comments:
>
> 1.) Specific show-stopper issues that would prevent you from approving
> the inclusion of this project in JDK12
>
> 2.) Specific small problems that could be addressed in the limited time
> left for JDK12.
>
> 3.) Any other problems or ideas for improvement that we should consider
> for JDK13 or future releases.
>
> /Andy
>
> >
> > Thanks, Roger
> >
> >
> > On 11/09/2018 05:25 PM, Andy Herrick wrote:
> >> This is an update to the Request For Review of the implementation of
> >> the Java Packager Tool (jpackager) as described in JEP 343: Packaging
> >> Tool 
> >>
> >> This refresh renames the packages used to jdk.jpackager and
> >> jdk.jpackager.runtime, removes the JNLPConverter demo, adds an
> >> initial set of automated tests, and contains fixes to the following
> >> issues:
> >>
> >> JDK-8213324 jpackager deletes existing app directory without warning
> >> JDK-8213166 jpackager --argument arg is broken
> >> JDK-8213163 --app-image arg does not work creating exe installers
> >> JDK-8212089 Prepare packager for localization
> >> JDK-8212537 Create method and class description comments for main
> >> functionality
> >> JDK-8213332 Create minimal automated tests for jpackager
> >> JDK-821 Fix issues found in jpackager with automated tests
> >> JDK-8213394 Stop using Log.info() except for expected output.
> >> JDK-8213345 Secondary Launchers broken on mac.
> >> JDK-8213156 rename packages for jpackager
> >> JDK-8213244 Fix all warnings in jpackager java code
> >> JDK-8212143 Remove native code that supports UserJvmOptionsService
> >> JDK-8213162 Association description in Inno Setup cannot contain
> >> double quotes
> >>
> >> The following additional issues are targeted to be address in the
> >> next few weeks:
> >> JDK-8212936 Makefile and other improvements for jpackager
> >> JDK-8212164 resolve jre.list and jre.module.list
> >> JDK-8213392 Enhance --help and --version
> >> JDK-8208652 File name is not passed to main() via file
> >> association on OS X
> >> JDK-8212538 Determine standard way to determine if a Modular jar
> >> JDK-8213558 Create more unit tests
> >>
> >> Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/
> >>
> >> please send feedback to core-libs-dev@openjdk.java.net
> >>
> >> /Andy Herrick
> >>
> >
>

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

On 11/13/2018 4:08 PM, Roger Riggs wrote:

Hi,

A few high level comments:

The JDK already has a command option parser (JoptSimple) in the module 
jdk.internal.opt
and the System Logger.  Why not use them for the argument parsing and 
logging?


We have an RFE to convert argument parsing to joptsimple that I filed 
last July JDK-8208300. 


I spent a few days at the time prototyping it, and concluded it was a 
multi-week project.


The team decided it was not a priority for JDK12 and it is now targeted 
to JDK13.




Similarly, we've been encouraging developers to use the java.nio.file 
APIs and
get away from java.io.File.  Try-with-resources could be used in a few 
places

to improve the closing of resources.

I can also see many places where the Streams functions could be used
to make the code easier to read (and write).  That's a missed 
opportunity.


What granularity of comments are you looking for?


We are looking for three types of comments:

1.) Specific show-stopper issues that would prevent you from approving 
the inclusion of this project in JDK12


2.) Specific small problems that could be addressed in the limited time 
left for JDK12.


3.) Any other problems or ideas for improvement that we should consider 
for JDK13 or future releases.


/Andy



Thanks, Roger


On 11/09/2018 05:25 PM, Andy Herrick wrote:
This is an update to the Request For Review of the implementation of 
the Java Packager Tool (jpackager) as described in JEP 343: Packaging 
Tool 


This refresh renames the packages used to jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the following 
issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests

Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

Re: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Thomas Stüfe]

Hi Roger,

I somehow wonder whether that coding could be further simplified and
maybe made faster by just returning a big byte array or String from
the hotspot containing a concatenation of key-value pairs, each
separated by \0, e.g. like this:

"os.name\0linux\0os.arch\0i386\0java.io.tmpdir\0/tmp\0..."

instead of a string array, that is.

That string could be parsed in java, chopped into single string
key-value pairs, which one could feed it into the Properties.

For speedups, one could even return an integer array in addition to
the big string, containing the start indices of the key/value strings.

That way, one would also not have to maintain named indices in java.

Just a thought. I may not seeing the whole picture though.

Best Regards, Thomas
On Tue, Nov 13, 2018 at 5:03 PM Roger Riggs  wrote:
>
> Please review a re-implementation of the initialization of System properties
> moving some functions from native to Java.
>
> The upcalls from native to java for each property are replaced by a
> mechanism
> to gather the platform, VM and command line properties and return them
> from a single JNI call.  The creation of the Properties instance and
> applying
> command line overrides is handled in Java instead of native.
>
> The JVM_initProperties interface in Hotspot is replaced by
> JVM_getProperties.
>
> Webrev:
>http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/
>
> Issue:
>https://bugs.openjdk.java.net/browse/JDK-4947890
>
> Thanks, Roger
>

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Brent Christian]

Along the same lines, if subclasses would typically benefit from 
overriding both skip() and skipNBytes(), how about we call out 
skipNBytes() in the skip() docs?  Either add a @see, or maybe append:


"Subclasses are encouraged to provide a more efficient implementation of 
this method" + " (as well as {@linkplain #skipNBytes()} )".


And do the same for skipNBytes().

-Brent

On 11/12/18 2:48 AM, Daniel Fuchs wrote:

Wouldn't it be better to throw an IOException in this case,
thus forcing subclasses that behave in this way to override
skipFully as well?

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Roger Riggs]

Hi,

A few high level comments:

The JDK already has a command option parser (JoptSimple) in the module 
jdk.internal.opt
and the System Logger.  Why not use them for the argument parsing and 
logging?


Similarly, we've been encouraging developers to use the java.nio.file 
APIs and
get away from java.io.File.  Try-with-resources could be used in a few 
places

to improve the closing of resources.

I can also see many places where the Streams functions could be used
to make the code easier to read (and write).  That's a missed opportunity.

What granularity of comments are you looking for?

Thanks, Roger


On 11/09/2018 05:25 PM, Andy Herrick wrote:
This is an update to the Request For Review of the implementation of 
the Java Packager Tool (jpackager) as described in JEP 343: Packaging 
Tool 


This refresh renames the packages used to jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an initial 
set of automated tests, and contains fixes to the following issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the next 
few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file association 
on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests

Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

I agree with this and would take it further.

1 file is in ./share/classes/jdk/jpackager/internal/builders - why not 
just ./share/classes/jdk/jpackager/internal


2 files are in ./share/classes/jdk/jpackager/internal/bundlers - why not 
just in ./share/classes/jdk/jpackager/internal


1 file is in ./linux/classes/jdk/jpackager/internal/builders/linux - why 
not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - why 
not just ./macosx/classes/jdk/jpackager/internal


1 file is in ./windows/classes/jdk/jpackager/internal/builders/windows - 
why not just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything would 
be easier to find, and we could make almost all methods package-private 
(the only exception would be the few things called by Main, and the 
ToolProvider.



On 11/13/2018 2:54 PM, Phil Race wrote:
Question .. why is "mac", "linux" and "windows" necessary in the 
package name here ?


 src/jdk.jpackager/macosx/classes/jdk/jpackager/internal/mac/MacAppBundler.java 

 src/jdk.jpackager/windows/classes/jdk/jpackager/internal/builders/windows/WindowsAppImageBuilder.java 

src/jdk.jpackager/linux/classes/jdk/jpackager/internal/linux/LinuxRpmBundler.java 



There's not likely to be a clash, so is there some other reason not to 
want these

in the same package as the shared internals like
src/jdk.jpackager/share/classes/jdk/jpackager/internal/Param.java

?

-phil.


I agree with this and would take it further.

1 file is in ./share/classes/jdk/jpackager/internal/builders - why not 
just ./share/classes/jdk/jpackager/internal


2 files are in ./share/classes/jdk/jpackager/internal/bundlers - why not 
just in ./share/classes/jdk/jpackager/internal


1 file is in ./linux/classes/jdk/jpackager/internal/builders/linux - why 
not just ./linux/classes/jdk/jpackager/internal


1 file is in ./macosx/classes/jdk/jpackager/internal/builders/mac - why 
not just ./macosx/classes/jdk/jpackager/internal


1 file is in ./windows/classes/jdk/jpackager/internal/builders/windows - 
why not just ./windows/classes/jdk/jpackager/internal


then just move the associated resources -

Basically put everything except Main in same package - everything would 
be easier to find, and we could make almost all methods package-private 
(the only exception would be the few things called by Main, and the 
ToolProvider.


/Andy

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Phil Race]

Question .. why is "mac", "linux" and "windows" necessary in the package 
name here ?


 src/jdk.jpackager/macosx/classes/jdk/jpackager/internal/mac/MacAppBundler.java
 
src/jdk.jpackager/windows/classes/jdk/jpackager/internal/builders/windows/WindowsAppImageBuilder.java
src/jdk.jpackager/linux/classes/jdk/jpackager/internal/linux/LinuxRpmBundler.java

There's not likely to be a clash, so is there some other reason not to 
want these

in the same package as the shared internals like
src/jdk.jpackager/share/classes/jdk/jpackager/internal/Param.java

?

-phil.

Re: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Roger Riggs]

Hi Andrew,

For the sake of brevity in the source, the PUTPROP and 
PUTPROP_PlatformString
macros prefix those "_xxx" arguments with 
"jdk_internal_util_SystemProps_Raw_"

so they refer to the indexes defined by @native in SystemProps.java.
Possibly more readable (but still shortish) would be to include the 
"Raw_" in source file.


The full symbol names are in the generated source file:
build/xxx/support/headers/java.base/jdk_internal_util_SystemProps_Raw.h

Thanks for reviewing,   Roger


On 11/13/2018 02:25 PM, Andrew Luo wrote:

I'm not a reviewer, but aren't names that begin with _ in the global namespace 
(_awt_headless_NDX for example) reserved for the C/C++ library?

Thanks,

Andrew

-Original Message-
From: core-libs-dev  On Behalf Of Roger 
Riggs
Sent: Tuesday, November 13, 2018 8:00 AM
To: Core-Libs-Dev ; 
hotspot-runtime-...@openjdk.java.net
Subject: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

Please review a re-implementation of the initialization of System properties 
moving some functions from native to Java.

The upcalls from native to java for each property are replaced by a mechanism 
to gather the platform, VM and command line properties and return them from a 
single JNI call.  The creation of the Properties instance and applying command 
line overrides is handled in Java instead of native.

The JVM_initProperties interface in Hotspot is replaced by JVM_getProperties.

Webrev:
    http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/

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

Thanks, Roger

RE: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Andrew Luo]

I'm not a reviewer, but aren't names that begin with _ in the global namespace 
(_awt_headless_NDX for example) reserved for the C/C++ library?

Thanks,

Andrew

-Original Message-
From: core-libs-dev  On Behalf Of Roger 
Riggs
Sent: Tuesday, November 13, 2018 8:00 AM
To: Core-Libs-Dev ; 
hotspot-runtime-...@openjdk.java.net
Subject: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

Please review a re-implementation of the initialization of System properties 
moving some functions from native to Java.

The upcalls from native to java for each property are replaced by a mechanism 
to gather the platform, VM and command line properties and return them from a 
single JNI call.  The creation of the Properties instance and applying command 
line overrides is handled in Java instead of native.

The JVM_initProperties interface in Hotspot is replaced by JVM_getProperties.

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/

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

Thanks, Roger

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Sean Mullan]

Looking good, a couple of comments/questions:

* src/java.base/share/classes/java/security/Security.java

The isJdkSecurityProperty method could return false positives, for 
example there may be a non-JDK property starting with "security.". I was 
thinking it would be better to put all the JDK property names in a 
HashSet which is populated by the static initialize() method, and only 
if event logging is enabled. Then setProperty can just check if the 
property name is in this set.


* src/java.base/share/classes/sun/security/x509/X509CertImpl.java

 158 // Event recording cache list
 159 private List recordedCerts;

Shouldn't this be static? Otherwise each certificate would have it's own 
instance and duplicates would not be detected.


The rest of my comments are mostly minor stuff, and nits:

* 
src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java


245 if(xve.shouldCommit() || EventHelper.isLoggingSecurity()) {

add space before "(xve"

* src/java.base/share/classes/sun/security/ssl/Finished.java

1097 }

indent

1098 if(event.shouldCommit()) {

add space before "(event"

* src/java.base/share/classes/sun/security/x509/X509CertImpl.java

update copyright

* src/java.base/share/classes/jdk/internal/event/EventHelper.java

35  * A helper class to have events logged to a JDK Event Logger

Add '.' at end of sentence.

* src/java.base/share/classes/jdk/internal/event/TLSHandshakeEvent.java

38: remove blank line

* src/java.base/share/classes/jdk/internal/event/X509ValidationEvent.java

30  * used in X509 cert path validation

Add '.' at end of sentence.

* src/jdk.jfr/share/classes/jdk/jfr/events/X509ValidationEvent.java

47: remove blank line

* test/jdk/jdk/security/logging/TestTLSHandshakeLog.java

45 l.addExpected("SunJSSE Test Serivce");

Is that a typo? "Serivce"

* test/jdk/jdk/security/logging/TestX509ValidationLog.java

54: remove blank line

* test/lib/jdk/test/lib/security/SSLSocketTest.java

Why is this file included? It looks like a duplicate of SSLSocketTemplate.

--Sean


On 11/6/18 3:46 AM, Seán Coffey wrote:

With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk code.

Some modifications also made based on off-thread suggestions :

src/java.base/share/classes/java/security/Security.java

* Only record JDK security properties for now (i.e. those in 
java.security conf file)
   - we can consider a new event to record non-JDK security events if 
demand comes about

   - new event renamed to *Jdk*SecurityPropertyModificationEvent

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

* Use hashCode() equality test for storing certs in List.

Tests updated to capture these changes

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java 



* Verify that self signed certs exercise the modified code paths

I've added new test functionality to test use of self signed cert.

webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/

Regards,
Sean.

On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off 
thread who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the 
@Relational JFR annotation.
* Keep calls for JFR event operations local to call site to optimize 
jvm compilation


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:

Erik,

After some trial edits, I'm not so sure if moving the event & logger 
commit code into the class where it's used works too well after all.


In the code you suggested, there's an assumption that calls such as 
EventHelper.certificateChain(..) are low cost. While that might be 
the case here, it's something we can't always assume. It's called 
once for the JFR operation and once for the Logger operation. That 
pattern multiplies itself further in other Events. i.e. set up and 
assign the variables for a JFR event without knowing whether they'll 
be needed again for the Logger call. We could work around it by 
setting up some local variables and re-using them in the Logger code 
but then, we're back to where we were. The current EventHelper design 
eliminates this problem and also helps to abstract the 
recording/logging functionality away from the functionality of the 
class itself.


It also becomes a problem where we record events in multiple 
different classes (e.g. SecurityPropertyEvent). While we could leave 
the code in EventHelper for this case, we then have a mixed design 
pattern.


Are you ok with leaving as is for now? A future wish might be one 
where JFR would handle 

RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Roger Riggs]

Please review a re-implementation of the initialization of System properties
moving some functions from native to Java.

The upcalls from native to java for each property are replaced by a 
mechanism

to gather the platform, VM and command line properties and return them
from a single JNI call.  The creation of the Properties instance and 
applying

command line overrides is handled in Java instead of native.

The JVM_initProperties interface in Hotspot is replaced by 
JVM_getProperties.


Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/

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

Thanks, Roger

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

On 11/13/2018 3:39 AM, Alan Bateman wrote:

On 12/11/2018 21:40, Philip Race wrote:

  74
  75 static String getTmpDir() {
  76 String os = System.getProperty("os.name").toLowerCase();
  77 if (os.contains("win")) {
  78 return System.getProperty("user.home")
  79 + 
"\\AppData\\LocalLow\\Sun\\Java\\JPackager\\tmp";

  80 } else if (os.contains("mac") || os.contains("os x")) {
  81 return System.getProperty("user.home")
  82 + "/Library/Application 
Support/Oracle/Java/JPackager/tmp";

  83 } else if (os.contains("nix") || os.contains("nux")
  84 || os.contains("aix")) {
  85 return System.getProperty("user.home") + 
"/.java/jpackager/tmp";

  86 }
  87
  88 return System.getProperty("java.io.tmpdir");


This seems unduly complex, and I don't understand the implication of
supporting AIX .. or some unknown "Unix", when packager is targeted
only at mac, linux + windows.
user.home is specified to be the user's home directory so I would 
think it should use that consistently everywhere. I assume "Sun" and 
"Oracle" can be dropped from the file location too.


Agreed - the resulting paths will all start with 
System.getProperty("user.home") and the "Sun" and "Oracle" 
sub-directories will be removed both here and in the matching native 
launcher code.  Added that to JDK-8213756 



/Andy




-Alan

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

On 11/12/2018 6:36 PM, Alan Snyder wrote:

Can someone say whether the two-step package creation feature is implemented 
and explain how to use it?


Yes -

1.) "jpackager create-image -o output -n app-name ..."

2.) "jpackager create-installer -o installer-output -n installer-name 
--app-image 'output\app-name' ..." (windows)


"jpackager create-installer -o installer-output -n installer-name 
--app-image 'output/app-name.app' ..." (for mac)


"jpackager create-installer -o installer-output -n installer-name 
--app-image 'output\app' ..." (for linux)




What is the plan for documentation? The command line help is inadequate.


Yes - there will be further documentation, it's content and form are 
still under discussion.


/Andy




On Oct 26, 2018, at 1:09 PM, Alan Snyder wrote:

I noticed the following statement in the JEP:

  In this latter case, the tool can either create a native package from a 
previously created application image, or it can create a native package 
directly.

I cannot tell from the command summary whether this option has been implemented 
or not.

Without this feature, the ability to create a signed DMG is not very useful, 
except in the case where the tool creates exactly the right application image.

  Alan

Both Collector.of() are not correctly typed

[Remi Forax]

Last year,
a student of mine as remarked that the two variants of Collector.of() are not 
correctly typed (the wildcards are missing)
and obviously, this year it's one of my teaching assistant that has found 
exactly the same issue.
So let's fix this issue.

Adding the wildcards is both source and binary backward compatible in this case
- the methods of are static (so no overriding possible)
- the wildcards types are super types of the types of current version, so it 
will not break at use sites and
- the erased signature is the same so it's a binary backward compatible change.

The right code is
@SuppressWarnings("unchecked")
public static Collector of(Supplier supplier,
  BiConsumer 
accumulator,
  BinaryOperator combiner,
  Characteristics... 
characteristics) {
Objects.requireNonNull(supplier);
Objects.requireNonNull(accumulator);
Objects.requireNonNull(combiner);
Objects.requireNonNull(characteristics);
Set cs = (characteristics.length == 0)
  ? Collectors.CH_ID
  : 
Collections.unmodifiableSet(EnumSet.of(Collector.Characteristics.IDENTITY_FINISH,
   
characteristics));
return new Collectors.CollectorImpl<>((Supplier)supplier, 
(BiConsumer)accumulator, combiner, cs);
}
and
@SuppressWarnings("unchecked")
public static Collector of(Supplier supplier,
 BiConsumer accumulator,
 BinaryOperator combiner,
 Function finisher,
 Characteristics... 
characteristics) {
Objects.requireNonNull(supplier);
Objects.requireNonNull(accumulator);
Objects.requireNonNull(combiner);
Objects.requireNonNull(finisher);
Objects.requireNonNull(characteristics);
Set cs = Collectors.CH_NOID;
if (characteristics.length > 0) {
cs = EnumSet.noneOf(Characteristics.class);
Collections.addAll(cs, characteristics);
cs = Collections.unmodifiableSet(cs);
}
return new Collectors.CollectorImpl<>((Supplier)supplier, 
(BiConsumer)accumulator, combiner, (Function)finisher, cs);
}

You may ask why the code is safe given there are now some 
@SuppressWarnings("unchecked"), it's because for a functional interface,
the parameter types are contra-variant and the return type is covariant.

At one point in the future, perhaps JEP 300 [1] will be implemented, in that 
case we will be able to remove the @SuppressWarnings.

You may also notice that, we may want at the same time  replace the 
unmodifiableSet(EnumSet.of() + add) with Set.of(),
i've not done that change given it's not related to the typing issue.

cheers,
Rémi

[1] https://openjdk.java.net/jeps/300
 

Re: Stream Method Proposal: long count(Predicate predicate)

[Brian Goetz]

Exactly right.

Note, though, that count() is _already_ a convenience method!  You could call 
.collect(counting()) instead.  So, why were we willing to add count() and 
similar methods, when we’re not willing to add this one?  Several reasons:

 - Discoverability.  Collect() is complicated, and until you understand the 
whole API (and the power of collect()) a new user would be stymied by how to do 
something simple like count or sum a list.  So sometimes, discoverability is a 
good reason to violate the preference against gratuitous convenience methods.

 - Performance.  For a large class of stream sources, there’s an optimization 
for counting that turns it into an O(1) operation (ask the spliterator if it 
knows its size), which collect() can’t do.  So sometimes, turning an O(n) 
operation into an O(1) operation is a good enough reason to violate the 
preference against gratuitous convenience methods.  

There are others too, but this one doesn’t clear the bar.  



> On Nov 12, 2018, at 12:50 AM, James Roper  wrote:
> 
> Another reason to prefer a smaller API is that it can aid the ability to
> comprehend, since there's less concepts to understand. The idea behind
> design patterns is that APIs constrain themselves to just using the well
> established and understood patterns, which means when someone who has never
> seen the API before comes and sees code that uses it, they can understand
> the code. But that only works if APIs do constrain themselves to the design
> patterns - constraint is key to the advantageous application of design
> patterns to APIs. java.util.stream does not sit by itself, it is one of
> hundreds of APIs on the JVM alone, including other languages it's among
> thousands of APIs that offer a functional API with filter and count
> abstractions. All of these APIs share filter/count as well established,
> well understood, instantly readable and understandable concepts. You don't
> have to know anything about java.util.stream to be able to understand
> exactly what filter(predicate).count() is doing. And this bootstrapping off
> this large ecosystem of APIs is one of the things that make
> java.util.stream a good API. But if it were to add count/findFirst variants
> that add predicates? It's not unprecedented, but it is by far less common,
> which makes it less understandable. That doesn't mean we never add
> convenience methods, but they do have to add a high degree of convenience
> to diverge from the well established patterns, and in this case, the
> convenience added is only small.
> 
> On Fri, 9 Nov 2018 at 04:39, Roger Riggs  wrote:
> 
>> Hi Jacob,
>> 
>> Its hard to resist the urge to add convenience methods, they look nice
>> and help a few developers.
>> However, they accumulate rapidly and end up obscuring the core
>> functionality.
>> They can hurt comprehension since they fold different functions together
>> and the collective API surface area ends up impinging on every
>> developers learning curve.
>> 
>> $.02, Roger
>> 
>> 
>> On 11/07/2018 08:00 PM, Jacob Glickman wrote:
>>>  Hello!
>>> 
>>> I see myself having to often call count() as a terminal operation on a
>>> Stream immediately after performing a filter operation. How feasible
>> would
>>> it be to add an overloaded count() method that accepts a Predicate, which
>>> it uses as a filter before returning the count of elements in the Stream?
>>> If this is supported, I'd gladly create the webrev & tests for it!
>>> 
>>> I suppose the method signature can be something along the lines of:
>>> 
>>> long count(Predicate predicate)
>>> 
>>> It would also seem reasonable to give this method to IntStream,
>>> DoubleStream, and LongStream, but allowing them to use IntPredicate,
>>> DoublePredicate, and LongPredicate respectively.
>>> 
>>> Thanks,
>>> 
>>> Jacob Glickman
>> 
>> 
> 
> -- 
> James Roper
> Architect, Office of the CTO, Lightbend, Inc.
> @jroper 
> 
> 

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Brian Goetz]

But don’t they?  We intend to start generating condy in classfiles from javac 
soon; at that point, anyone not using ASM7 will fail when reading classfiles 
generated by javac.  

> On Nov 8, 2018, at 4:03 AM, David Holmes  wrote:
> 
> Is it that case that the code the uses the ASM library, like the JFR code and 
> jlink code, and the tests, doesn't actually _have to_ change to specifying 
> Opcodes.ASM7 unless they plan on using ASM7 features? If so then you could 
> split out the actual update of ASM from the updates to the users of ASM (some 
> of which may be quite fine with ASM5).

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Andy Herrick]

Yes - The intent of getTmpDir() here is to match the GetTempDirectory() 
in launcher, which this does for the three supported platforms, but 
there is no need to check for the unsupported platforms.


I will clean this up as you suggest as part ofJDK-8213756 



/Andy

On 11/12/2018 4:40 PM, Philip Race wrote:

   74
   75 static String getTmpDir() {
   76 String os = System.getProperty("os.name").toLowerCase();
   77 if (os.contains("win")) {
   78 return System.getProperty("user.home")
   79 + "\\AppData\\LocalLow\\Sun\\Java\\JPackager\\tmp";
   80 } else if (os.contains("mac") || os.contains("os x")) {
   81 return System.getProperty("user.home")
   82 + "/Library/Application 
Support/Oracle/Java/JPackager/tmp";
   83 } else if (os.contains("nix") || os.contains("nux")
   84 || os.contains("aix")) {
   85 return System.getProperty("user.home") + 
"/.java/jpackager/tmp";
   86 }
   87
   88 return System.getProperty("java.io.tmpdir");

This seems unduly complex, and I don't understand the implication of
supporting AIX .. or some unknown "Unix", when packager is targeted
only at mac, linux + windows.

I think its sufficient to look for the strings windows, macos and linux.
And if you want a persistent storage location then default to the "unix"
location if there's no match .. although I am not sure it makes sense
on platforms that aren't targeted by jpackager.

-phil.


-phil.

On 11/12/18, 12:22 PM, Philip Race wrote:

Adding build-dev back ..

I noticed that module jdk.jpackager.runtime requires java.desktop on 
all platforms


So far as I can tell this is for a Mac-only support for receiving and
handling file open events. Probably it only makes sense or gets used
when the API is used from a running desktop application.

I might ask if we need this at all, but I definitely think it should
not be required on all platforms if needed only for Mac even if
we might want it on windows in a future version.

Do we not envisage cases where you need the runtime API for
some kind of daemon service for which there should be a singleton ?
Do you really want to force the desktop module to be dragged along
in such a case ?

I think we should remove this dependency even if it means losing a
MacOS feature at least for now.

-phil.

On 11/11/18, 2:40 PM, Andy Herrick wrote:

On 11/9/2018 5:25 PM, Andy Herrick wrote:
This is an update to the Request For Review of the implementation 
of the Java Packager Tool (jpackager) as described in JEP 343: 
Packaging Tool 


This refresh renames the packages used to jdk.jpackager and 
jdk.jpackager.runtime, removes the JNLPConverter demo, adds an 
initial set of automated tests, and contains fixes to the 
following issues:


JDK-8213324 jpackager deletes existing app directory without warning
JDK-8213166 jpackager --argument arg is broken
JDK-8213163 --app-image arg does not work creating exe installers
JDK-8212089 Prepare packager for localization
JDK-8212537 Create method and class description comments for main 
functionality

JDK-8213332 Create minimal automated tests for jpackager
JDK-821 Fix issues found in jpackager with automated tests
JDK-8213394 Stop using Log.info() except for expected output.
JDK-8213345 Secondary Launchers broken on mac.
JDK-8213156 rename packages for jpackager
JDK-8213244 Fix all warnings in jpackager java code
JDK-8212143 Remove native code that supports UserJvmOptionsService
JDK-8213162 Association description in Inno Setup cannot contain 
double quotes


The following additional issues are targeted to be address in the 
next few weeks:

JDK-8212936 Makefile and other improvements for jpackager
JDK-8212164 resolve jre.list and jre.module.list
JDK-8213392 Enhance --help and --version
JDK-8208652 File name is not passed to main() via file 
association on OS X

JDK-8212538 Determine standard way to determine if a Modular jar
JDK-8213558 Create more unit tests
Note: The above issues are targeted to 'internal' - meaning we 
expect to resolve them in the sandbox before the initial push to 
JDK12.
Issues targeted to '12' are expected to be fixed after the initial 
push.


/Andy


Webrev: http://cr.openjdk.java.net/~herrick/8212780/webrev.2/

please send feedback to core-libs-dev@openjdk.java.net

/Andy Herrick

Re: RFR - JDK-8203442 String::transform (Code Review)

[Brian Goetz]

> An argument against re-using the name map() for this String method is that 
> Stream.map() and Optional.map() act on the element(s) of the "container" the 
> method is invoked upon, and return the same raw part of type with type 
> parameter adjusted, while String.map() would be passing the target object 
> itself to the function and returning an arbitrary type.

This is exactly why we did not choose the name map() when this feature was 
proposed.  (Such is life when a platform is accrete-only; you have to live with 
your past decisions, good and bad.)  

(I didn’t see the proposal to rename transform() to map() fly by; I would have 
made the same comments as Peter.)  

> Other languages have introduced an operator to solve that issue (function 
> call syntax is not fluent) like by example the operator '|>' as in "foo" |> 
> Utils.capitalizeFirstLetter.

Yes, we know :)  But we don’t have any current plans to do that, nor use-site 
extension methods, nor does it seem likely to come to the top of the language 
priority list very soon.  So its not a choice between |> and .transform(); it’s 
a choice between transform() and nothing.  Picking transform() seems pretty 
good here.  

Stephen raised the issue of a “broader context”; indeed, the intention of 
adding a transform() method here was that it would be feasible to add to other 
types for which it was suitable.  String is an obvious candidate for “do it 
first”, but this is within a broader context.

Re: Stream Method Proposal: long count(Predicate predicate)

[Brian Goetz]

It is entirely feasible.  But, that’s not the criteria for adding it.  

There are frequent calls for fusing methods that are commonly used together, 
such as filter+map.  Our position has been that the stream API is better served 
by providing mostly orthogonal primitives, and letting developers compose them 
as they see fit.  This keeps the API simpler, and reduces the maintenance 
burden, allowing us to focus on more significant improvements.  (Imagine how 
this goes; we add this one, which is inevitably used as justification for a 
thousand others, each of which is desired by at least one person.  And the 
result is an unusable API.)  



> On Nov 8, 2018, at 1:57 AM, Jacob Glickman  wrote:
> 
> Hello!
> 
> I see myself having to often call count() as a terminal operation on a
> Stream immediately after performing a filter operation. How feasible would
> it be to add an overloaded count() method that accepts a Predicate, which
> it uses as a filter before returning the count of elements in the Stream?
> If this is supported, I'd gladly create the webrev & tests for it!
> 
> I suppose the method signature can be something along the lines of:
> 
>long count(Predicate predicate)
> 
> It would also seem reasonable to give this method to IntStream,
> DoubleStream, and LongStream, but allowing them to use IntPredicate,
> DoublePredicate, and LongPredicate respectively.
> 
> Thanks,
> 
> Jacob Glickman

Re: Method references in annotations

[Brian Goetz]

The story here is: we’ve completed our analysis on the feature, and concluded 
that the feature is sound (many such features don’t survive this cut.)  But, 
there is actually quite a bit of work needed in the underlying infrastructure 
to implement it, largely having to do with annotation processing in the VM, and 
reflection.  As a result, the amount of work went up, which pushed its 
cost/benefit ratio down.  It is still on the list, but hasn’t been prioritized. 
 

If someone with VM and reflection skills wanted to step up and help implement 
it, that might move the balance back in the other direction.  

> On Nov 7, 2018, at 8:17 PM, Julian Ruppel  wrote:
> 
> Hi,
> 
> being aware that there has been a discussion quite some time ago (
> http://mail.openjdk.java.net/pipermail/lambda-dev/2011-August/003768.html)
> I wanted to come up again with this topic as this feature would be
> beneficial especially for framework developers who wants to appoint
> behaviour to annotations.
> 
> The following snippet shows what I want to vote for.
> 
> @MyAnnotation(method = MyClass::myMethod)private String myVariable;
> 
> This would be far more type-safe compared to using string literals to find
> the method via reflection.
> 
> 
> - Julian

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Alan Bateman]

On 13/11/2018 14:00, Vicente Romero wrote:
any other comment after the last iteration? we are in a bit of a hurry 
to push this before the JDK 12 train departs :(
The original patch updated all the use sites (and tests) to specify ASM7 
for the API version. I just checked the webrev again now and it seems to 
be just the ASM refresh now. Assuming all the tests are passing and 
you've sorted out the mvlm test issues with Igor then I suggest go ahead 
with this push and we can update the sites, as needed, at a later time.


-Alan

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

any other comment after the last iteration? we are in a bit of a hurry 
to push this before the JDK 12 train departs :(


Thanks,
Vicente

On 11/8/18 11:39 AM, Vicente Romero wrote:

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features?


I changed only the tests for which the new ASM was complaining about a 
particular API available only for ASM7


If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with ASM5).


I have made two webrevs to make the review easier [1], contain only 
the changes to the internal asm and [2] contains the changes to the 
clients plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/


On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal 
version in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Stephen Colebourne]

I'm very uncomfortable about adding this method as is.

Firstly, as Peter says (in a different thread), naming this method
`map()` is inconsistent with `Stream` and `Optional`. Adding `map()`
to `String` would be:
  public String map(Function);
Not that such a method would be particularly useful!

Secondly, the premise of the method is effectively an alternative to
the language feature "extension methods". While the motivation in
https://bugs.openjdk.java.net/browse/JDK-8203703 and
https://dzone.com/articles/making-java-fluent-api-more-flexible makes
some sense, it is clearly equally applicable to all types - `String`
is not special here. An isolated change here just looks odd and ill
thought through - a big mistake for the core `String` class.

My preference is for the change to not proceed without consideration
of the broader context. The change should not be added without a
commitment to also add the same method to Stream and Optional as a
minimum, something that would make the addition part of a more
consistent broader design. Doing this would also mandate a different
method name.

FWIW, were such a method added to java.time.* as the OP suggested, it
would enable pluggable conversions:

  java.util.Date ju = LocalDate.now()
.plusDays(2)
.apply(MyUtility::convertToOldDate);

Clearly there is some value to the above in terms of abstraction.
However, there would also be problems making the best choice of types.
Does it go on the concrete LocalDate or the interface Temporal. Its a
problem just like String vs CharSequence.

TLDR, this change needs to be part of a broader context to be
acceptable (something that would inform the best name). Without that
broader context, I'm against this change.

Stephen


On Mon, 12 Nov 2018 at 14:05, Jim Laskey  wrote:
>
> updated webrev: 
> http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html 
> 
> > On Sep 21, 2018, at 7:42 AM, Remi Forax  wrote:
> >
> > - Mail original -
> >> De: "Alan Bateman" 
> >> À: "Jim Laskey" , "core-libs-dev" 
> >> 
> >> Envoyé: Vendredi 21 Septembre 2018 12:22:42
> >> Objet: Re: RFR - JDK-8203442 String::transform (Code Review)
> >
> >> On 18/09/2018 18:52, Jim Laskey wrote:
> >>> Please review the code for String::transform. The goal is to provide a 
> >>> String
> >>> instance method to allow function application of custom transformations 
> >>> applied
> >>> to an instance of String.
> >>>
> >>> webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html
> >>> jbs: https://bugs.openjdk.java.net/browse/JDK-8203442
> >>> csr: https://bugs.openjdk.java.net/browse/JDK-8203703
> >> I hate to bring up naming but if I have a Stream or
> >> Optional then I'll use the "map" method to apply the mapping
> >> function. Has this already been through the naming bike shed and it
> >> settled on "transform"? Maybe a different name was chosen after looking
> >> at code that would use it in stream operations? Maybe "transform" was
> >> used as the argument to the function is always text? I'm not interested
> >> in re-opening any discussions, just trying to see if options are
> >> captured in a mail somewhere.
> >>
> >> I'm also wondering if this is general enough to be defined by
> >> CharSequence. Retrofitting CharSequence is always problematic and never
> >> done lightly but I'm just wondering if it has been discussed and
> >> dismissed. I don't think it could be done at a later time, at least not
> >> without changing the input type parameter so that the mapping function
> >> is Function rather than Function.
> >
> > the main issue is that a lot of utility methods take a String as parameter, 
> > not a CharSequence :(
> > and Function is a supertype not a subtype of Function > super CharSequence, R>.
> >
> >>
> >> -Alan
> >
> > Rémi
>

Re: RFR - JDK-8203442 String::transform (Code Review)

[Peter Levart]

On 9/21/18 12:22 PM, Alan Bateman wrote:

On 18/09/2018 18:52, Jim Laskey wrote:
Please review the code for String::transform. The goal is to provide 
a String instance method to allow function application of custom 
transformations applied to an instance of String.


webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html
jbs: https://bugs.openjdk.java.net/browse/JDK-8203442
csr: https://bugs.openjdk.java.net/browse/JDK-8203703
I hate to bring up naming but if I have a Stream or 
Optional then I'll use the "map" method to apply the mapping 
function.


An argument against re-using the name map() for this String method is 
that Stream.map() and Optional.map() act on the element(s) of the 
"container" the method is invoked upon, and return the same raw part of 
type with type parameter adjusted, while String.map() would be passing 
the target object itself to the function and returning an arbitrary 
type. So in this regard, it is a different operation. The same method as 
suggested for String would be usable on Stream too, but it would have to 
be called differently on Stream. Imagine defining this method on Object. 
It would clash with Stream.map() and Optional.map() if it was called 
map(). So I don't think .map() is the best name for this method.


Regards, Peter

Re: The new optimized version of Dual-Pivot Quicksort

[Zheka Kozlov]

Thanks, Tagir.

The benchmark was indeed incorrect. There was also another mistake:
dualPivot() and radix() sorted different arrays. I believe I fixed it now:
https://gist.github.com/orionll/595d10ff37ffe0d8c3824e734055cf00

Benchmark   (seed)(size)  Mode  Cnt   Score   Error  Units
Sort.dualPivot   110  avgt5   0,066 ? 0,008  us/op
Sort.dualPivot   1  1000  avgt5  15,611 ? 0,778  us/op
Sort.dualPivot   110  avgt55968,684 ?   883,238  us/op
Sort.dualPivot   1  1000  avgt5  873228,528 ? 93489,438  us/op
Sort.dualPivot   210  avgt5   0,053 ? 0,013  us/op
Sort.dualPivot   2  1000  avgt5  18,489 ? 0,874  us/op
Sort.dualPivot   210  avgt56821,103 ?  1367,378  us/op
Sort.dualPivot   2  1000  avgt5  873083,780 ? 55668,302  us/op
Sort.dualPivot   310  avgt5   0,049 ? 0,001  us/op
Sort.dualPivot   3  1000  avgt5  17,539 ? 2,661  us/op
Sort.dualPivot   310  avgt55959,760 ?   288,375  us/op
Sort.dualPivot   3  1000  avgt5  904949,768 ? 95230,256  us/op
Sort.radix   110  avgt5   1,252 ? 0,133  us/op
Sort.radix   1  1000  avgt5   8,403 ? 1,465  us/op
Sort.radix   110  avgt52106,251 ?   310,883  us/op
Sort.radix   1  1000  avgt5  377041,746 ? 42450,236  us/op
Sort.radix   210  avgt5   1,244 ? 0,100  us/op
Sort.radix   2  1000  avgt5   8,685 ? 1,726  us/op
Sort.radix   210  avgt52201,534 ?   204,659  us/op
Sort.radix   2  1000  avgt5  381438,308 ? 47735,981  us/op
Sort.radix   310  avgt5   1,259 ? 0,090  us/op
Sort.radix   3  1000  avgt5   8,338 ? 1,127  us/op
Sort.radix   310  avgt52081,945 ?   225,230  us/op
Sort.radix   3  1000  avgt5  376115,679 ? 47896,963  us/op

Now the benchmark shows that radix() wins significantly except for small
arrays.

вт, 13 нояб. 2018 г. в 15:32, Tagir Valeev :

> Hello, Zheka!
>
> Seems that your benchmark is wrong: after the first iteration (which
> is part of warmup) you're always sorting an array which is already
> sorted, so in fact you are testing how algorithms behave on already
> sorted arrays.
>
> With best regards,
> Tagir Valeev.
> On Mon, Nov 12, 2018 at 10:08 AM Zheka Kozlov 
> wrote:
> >
> > Hi Tagir!
> >
> > I wrote a simple benchmark comparing Arrays.sort() and your
> implementation:
> https://gist.github.com/orionll/595d10ff37ffe0d8c3824e734055cf00
> >
> > Here are the results on my computer (JDK 11):
> >
> > REMEMBER: The numbers below are just data. To gain reusable insights,
> you need to follow up on
> > why the numbers are the way they are. Use profilers (see -prof, -lprof),
> design factorial
> > experiments, perform baseline and negative tests that provide
> experimental control, make sure
> > the benchmarking environment is safe on JVM/OS/HW level, ask for reviews
> from the domain experts.
> > Do not assume the numbers tell you what you want them to tell.
> >
> > Benchmark (size)  Mode  Cnt  Score  Error  Units
> > Sort.dualPivot10  avgt4  0,012 ?0,002  us/op
> > Sort.dualPivot  1000  avgt4  0,282 ?0,014  us/op
> > Sort.dualPivot10  avgt4 31,625 ?   10,063  us/op
> > Sort.dualPivot  1000  avgt4  10077,948 ?  601,204  us/op
> > Sort.radix10  avgt4  1,310 ?0,151  us/op
> > Sort.radix  1000  avgt4  1,297 ?0,063  us/op
> > Sort.radix10  avgt4 33,009 ?2,303  us/op
> > Sort.radix  1000  avgt4  10150,036 ? 1095,015  us/op
> >
> > I don't want to make any conclusions. These are just numbers. Probably
> your implementation can be optimized so it beats the platform sort at least
> on large arrays.
> >
> > вс, 11 нояб. 2018 г. в 11:48, Tagir Valeev :
> >>
> >> Hello!
> >>
> >> By the way why not using radix sort, at least for int[] arrays? The
> >> implementation is much simpler, it uses constant-size additional
> >> buffer (1024 ints) and performance should be better than DPQS, at
> >> least for large arrays. Here's sample implementation written by me
> >> several years ago (reusing merge sort part from JDK) which degrades to
> >> merge sort if array is nearly sorted.
> >> http://cr.openjdk.java.net/~tvaleev/patches/radix/RadixSort.java
> >>
> >> With best regards,
> >> Tagir Valeev.
> >>
> >> On Fri, Jan 19, 2018 at 8:38 PM Vladimir Yaroslavskiy
> >>  wrote:
> >> >
> >> > Hi team,
> >> >
> >> > In Sept 2009 Josh Bloch, Jon Bentley and I introduced new sorting
> >> > algorithm, Dual-Pivot Quicksort, for primitives in JDK 7 and later
> >> > I suggested several improvements of Dual-Pivot Quicksort, which
> >> > 

Re: RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Andrej Golovnin]

Hi Jim,

> updated webrev: 
> http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html 
> 

src/java.base/share/classes/java/lang/String.java

2983  * @param   class of the result

Maybe "the type of the result" would be better.

Best regards,
Andrej Golovnin

Re: RFR: JDK-8212780: JEP 343: Packaging Tool Implementation

[Alan Bateman]

On 12/11/2018 21:40, Philip Race wrote:

  74
  75 static String getTmpDir() {
  76 String os = System.getProperty("os.name").toLowerCase();
  77 if (os.contains("win")) {
  78 return System.getProperty("user.home")
  79 + 
"\\AppData\\LocalLow\\Sun\\Java\\JPackager\\tmp";

  80 } else if (os.contains("mac") || os.contains("os x")) {
  81 return System.getProperty("user.home")
  82 + "/Library/Application 
Support/Oracle/Java/JPackager/tmp";

  83 } else if (os.contains("nix") || os.contains("nux")
  84 || os.contains("aix")) {
  85 return System.getProperty("user.home") + 
"/.java/jpackager/tmp";

  86 }
  87
  88 return System.getProperty("java.io.tmpdir");


This seems unduly complex, and I don't understand the implication of
supporting AIX .. or some unknown "Unix", when packager is targeted
only at mac, linux + windows.
user.home is specified to be the user's home directory so I would think 
it should use that consistently everywhere. I assume "Sun" and "Oracle" 
can be dropped from the file location too.


-Alan

Re: The new optimized version of Dual-Pivot Quicksort

[Tagir Valeev]

Hello, Zheka!

Seems that your benchmark is wrong: after the first iteration (which
is part of warmup) you're always sorting an array which is already
sorted, so in fact you are testing how algorithms behave on already
sorted arrays.

With best regards,
Tagir Valeev.
On Mon, Nov 12, 2018 at 10:08 AM Zheka Kozlov  wrote:
>
> Hi Tagir!
>
> I wrote a simple benchmark comparing Arrays.sort() and your implementation: 
> https://gist.github.com/orionll/595d10ff37ffe0d8c3824e734055cf00
>
> Here are the results on my computer (JDK 11):
>
> REMEMBER: The numbers below are just data. To gain reusable insights, you 
> need to follow up on
> why the numbers are the way they are. Use profilers (see -prof, -lprof), 
> design factorial
> experiments, perform baseline and negative tests that provide experimental 
> control, make sure
> the benchmarking environment is safe on JVM/OS/HW level, ask for reviews from 
> the domain experts.
> Do not assume the numbers tell you what you want them to tell.
>
> Benchmark (size)  Mode  Cnt  Score  Error  Units
> Sort.dualPivot10  avgt4  0,012 ?0,002  us/op
> Sort.dualPivot  1000  avgt4  0,282 ?0,014  us/op
> Sort.dualPivot10  avgt4 31,625 ?   10,063  us/op
> Sort.dualPivot  1000  avgt4  10077,948 ?  601,204  us/op
> Sort.radix10  avgt4  1,310 ?0,151  us/op
> Sort.radix  1000  avgt4  1,297 ?0,063  us/op
> Sort.radix10  avgt4 33,009 ?2,303  us/op
> Sort.radix  1000  avgt4  10150,036 ? 1095,015  us/op
>
> I don't want to make any conclusions. These are just numbers. Probably your 
> implementation can be optimized so it beats the platform sort at least on 
> large arrays.
>
> вс, 11 нояб. 2018 г. в 11:48, Tagir Valeev :
>>
>> Hello!
>>
>> By the way why not using radix sort, at least for int[] arrays? The
>> implementation is much simpler, it uses constant-size additional
>> buffer (1024 ints) and performance should be better than DPQS, at
>> least for large arrays. Here's sample implementation written by me
>> several years ago (reusing merge sort part from JDK) which degrades to
>> merge sort if array is nearly sorted.
>> http://cr.openjdk.java.net/~tvaleev/patches/radix/RadixSort.java
>>
>> With best regards,
>> Tagir Valeev.
>>
>> On Fri, Jan 19, 2018 at 8:38 PM Vladimir Yaroslavskiy
>>  wrote:
>> >
>> > Hi team,
>> >
>> > In Sept 2009 Josh Bloch, Jon Bentley and I introduced new sorting
>> > algorithm, Dual-Pivot Quicksort, for primitives in JDK 7 and later
>> > I suggested several improvements of Dual-Pivot Quicksort, which
>> > were integrated into JDK 8.
>> >
>> > Now I have more optimized and faster version of Dual-Pivot Quicksort.
>> > I have been working on it for the last 5 years. Please, find the
>> > summary of changes below and sources / diff at webrev [1].
>> >
>> > All tests and benchmarking were run on the most recent build of JDK 10,
>> > jdk-10-ea+39. The new version shows the better performance on different
>> > inputs and guarantees n*log(n) on any data.
>> >
>> > The new implementation of Dual-Pivot Quicksort is 'all-in-one' version:
>> > it contains one code for both parallel and sequential sorting algorithms.
>> >
>> > Suggested version is 10-20% faster on random data, 1.5-4 times faster
>> > on nearly structured arrays, 1.5-2 times faster on period inputs.
>> >
>> > Parallel Dual-Pivot Quicksort is 1.5-3 times faster than current
>> > algorithm based on merge sort.
>> >
>> > Benchmarking on the test suite, suggested by Jon Bentley, shows the
>> > boost of performance in 1.4 times. This test suite contains several
>> > types of inputs, such as random data, nearly structured arrays, data
>> > with period and so on. Also there are several modifications of inputs
>> > and parameters which are used in data building. We run sorting on all
>> > combinations to compare two algorithms.
>> >
>> > Please let me know if you have any questions / comments.
>> >
>> > Summary of changes:
>> >
>> > DualPivotQuicksort class
>> > 
>> > * Pivots are chosen with another step, the 1-st and 5-th candidates
>> >are taken as pivots instead of 2-nd and 4-th.
>> > * Splitting into parts is related to the golden ratio
>> > * Pivot candidates are sorted by combination of 5-element
>> >network sorting + insertion sort
>> > * New backwards partitioning is simpler and more efficient
>> > * Quicksort tuning parameters were updated
>> > * Merging sort is invoked on each iteration from Quicksort
>> > * Additional steps for float/double were fully rewritten
>> > * Heap sort is invoked on the leftmost part
>> > * Heap sort is used as a guard against quadratic time
>> > * Parallel sorting is based on Dual-Pivot Quicksort
>> >instead of merge sort
>> >
>> > SortingAlgorithms class
>> > ---
>> > * Merging sort and pair insertion sort were moved from
>> >DualPivotQuicksort class
>> > *