Re: RFR: 8165492: Reduce number of lambda forms generated by MethodHandleInlineCopyStrategy

[Vladimir Ivanov]

Webrev: http://cr.openjdk.java.net/~redestad/8165492/webrev.01/


Looks good.

As others, I prefer InternalError over IllegalStateException when buffer 
size mismatch.


Best regards,
Vladimir Ivanov



Testing: RBT, no regressions observed on microbenchmarks[1], while
recuperating ~25-30% of the largest startup regressions introduced in
9+120

It's not the intent of this change to make this new foldArguments a
part of the public API, since it's simply too late for 9. Instead we'd
like to defer the discussion of possibly including this in the public
API until a later time, which also gives us ample opportunity to
examine other options - such as being better at not fully generating
intermediary LFs.

Thanks!

/Claes

Re: RFR: 8165492: Reduce number of lambda forms generated by MethodHandleInlineCopyStrategy

[Claes Redestad]

On 2016-09-09 12:35, Vladimir Ivanov wrote:


Looks good.

As others, I prefer InternalError over IllegalStateException when buffer
size mismatch.


Done: http://cr.openjdk.java.net/~redestad/8165492/webrev.02/

Gave it an extra round of testing since if there was a bug somewhere
it might have gotten more visible with this, but seems we're good. :-)

Thanks!

/Claes

Re: RFR(s): 4285505: deprecate java.lang.Compiler

[Tim Ellison]

On 08/09/16 17:08, Krystal Mok wrote:
> Thanks for your reply. It's good to know at least the known use cases
> from IBM have indeed been discussed.
> 
> On Thu, Sep 8, 2016 at 9:01 AM, Tim Ellison  > wrote:
> 
> On 07/09/16 23:45, Krystal Mok wrote:
> > I see that on the JBS page, your most recent comment says it's been 
> decided
> > that for JDK9 it's okay to deprecate and forRemoval=true, while also
> > mentioning the uses of this class in IBM's implementation.
> >
> > Does that mean IBM has agreed on the deprecation of this class?
> 
> Yep, I've seen those references and am ok with the deprecation in JDK9.
> 
> > I thought J9 had features that allowed Java applications to do
> > fine-grained control over the JIT compiler at runtime, e.g. force
> > compilation of specified methods *at some certain point* in the
> > program.
> 
> Not sure what you are thinking of there Kris.  We do implement the five
> public methods on Compiler to do pretty much what you would expect:
> 
> java.lang.Compiler.command(Object any) - this only does something for a
> couple of custom arguments, most objects passed to it are simply
> ignored.
> 
> java.lang.Compiler.compileClass(Class clazz) - the JIT will compile
> all methods in the given class. These compilations are synchronous, i.e.
> the application thread that called the API will wait until all
> compilations are finished.
> 
> This and the next one are the ones I was talking about: these calls can
> be made at arbitrary points in time during Java program execution, so
> they are "dynamic".
> For instance, I may have a program that wishes to convey phase shift
> properties to the VM, by calling once of these methods right before the
> phase shift happens. That's something a static configuration means (e.g.
> compiler configuration file) won't be able to do. Ditto for the
> "waitOnCompilationQueue" command.

Yes, that is one way in which we can have an application indicate to the
JIT that it is moving from start-up to run phase, and we have played
with that.  But come JDK9 and beyond there are much better opportunities
for start-up optimizations that means these APIs can be removed without
grief.

Regards,
Tim

> java.lang.Compiler.compileClasses(String s) - the JIT will compile all
> methods from classes that match the given string.
> 
> java.lang.Compiler.disable() - disables all future JIT compilations.
> 
> java.lang.Compiler.enable() - resumes JIT compilations.
> 
> IMO dropping these APIs would not be a great loss.
> 
> > What JEP 165 is proposing can only statically configure JIT behaviors 
> for
> > HotSpot. The same approach doesn't seem to cover what J9 used to 
> support.
> >
> > Could you please share more background on the discussions that led to 
> the
> > decision?
> 
> As you would expect, IBM and Oracle talk regularly about all things
> Java, and this topic was raised as a heads-up that it was coming to
> OpenJDK.  So there really isn't any background to the discussion.
> 
> Regards,
> Tim
> (IBM Java team)
> 
> > On Wed, Sep 7, 2016 at 2:50 PM, Stuart Marks
> mailto:stuart.ma...@oracle.com>>
> > wrote:
> >
> >> Tomorrow's headline: Oracle Proposes To Remove Java JIT Compiler
> >>
> >> :-)
> >>
> >>
> >> On 9/7/16 2:44 PM, Remi Forax wrote:
> >>
> >>> Yes, i like this patch :)
> >>>
> >>> Aleksey, It's worst than what you think because for a lot of people
> >>> Compiler == java compiler and not JIT compiler so they try to
> compile a
> >>> .java file with the method compileClasses(String).
> >>>
> >>> I'm glad this class will disappear soon.
> >>>
> >>> Rémi
> >>>
> >>> - Mail original -
> >>>
>  De: "Aleksey Shipilev"  >
>  À: "Stuart Marks"  >, "core-libs-dev" <
>  core-libs-dev@openjdk.java.net
> >
>  Envoyé: Mercredi 7 Septembre 2016 23:34:02
>  Objet: Re: RFR(s): 4285505: deprecate java.lang.Compiler
> 
> >>>
> >>> On 09/07/2016 11:52 PM, Stuart Marks wrote:
> 
> > Please review this small patch to deprecate java.lang.Compiler for
> > removal.
> >
> 
>  Yes, +1.
>  This class is very confusing to have these days.
> 
>  -Aleksey
> 
> >>>
> 
> 

Re: RFR 8165634: Support multiple --add-module options on the command line

[harold seigel]

Hi Gerard,

Thanks for the review!  Please see comments inline.

Harold


On 9/8/2016 12:24 PM, Gerard Ziemski wrote:

hi Harold,

The changes look fine. I have a couple of questions though:

#1 Would the "test/runtime/modules/ModuleOptionsTest.java” be more robust if we 
included a case with a non-error return value that takes more than one module? Ex: 
“java --add-modules=java.base --add-modules=jdk.internal.reflect=ALL-UNNAMED 
-version”
The VM just converts each --add-modules option into a 
jdk.module.add.mods. property that it passes back to the JDK.  The 
fact that the JDK throws an exception for the 
"--add-modules=i_dont_exist --add-modules=java.base" case means that the 
VM is successfully handling both --add-modules and returning both of 
their values to the JDK.   So I don't think an additional test case is 
needed.


Also, the JDK AddModsTest.java test has a multiple --add-modules test 
case that returns successfully.


#2 Looking at the changes for jdk it appears we now also allow duplicate "--add-exports” and 
"--add-reads"? Does it also mean we allow duplicate "--add-modules”, Ex: “java 
--add-modules=java.base --add-modules=java.base -version”?
Thanks for also reviewing the JDK changes.  The core-libs reviewers 
asked that the support for duplicate --add-exports and --add-reads be 
removed from this webrev.  (See next revision.)  Duplicate --add-modules 
are allowed.



cheers



On Sep 8, 2016, at 8:23 AM, harold seigel  wrote:

Hi,

Please review this fix for JDK-8165634.  The fix changes the --add-modules 
option from being a 'last one wins' option to a cumulative one.  With this 
change, if multiple --add-modules options are specified, the VM accumulates all 
the options' values, instead of ignoring all but the last option's value.  The 
--add-modules values are reported back to the JDK as properties using the 
Arguments::create_numbered_property() function.

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

Open webrevs:

   http://cr.openjdk.java.net/~hseigel/bug_8165634.hs/

   http://cr.openjdk.java.net/~hseigel/bug_8165634.jdk/

The fix was tested with the JCK Lang and VM tests, the hotpot, and java/lang, 
java/util and other JTreg tests, the RBT tier2 tests, and the NSK non-colocated 
quick tests.

The JDK changes were done by Mandy Chung (mchung).

Thanks, Harold

Re: RFR 8165634: Support multiple --add-module options on the command line

[harold seigel]

Thanks Lois, for the review.

Harold


On 9/8/2016 1:12 PM, Lois Foltan wrote:


On 9/8/2016 9:23 AM, harold seigel wrote:

Hi,

Please review this fix for JDK-8165634.  The fix changes the 
--add-modules option from being a 'last one wins' option to a 
cumulative one.  With this change, if multiple --add-modules options 
are specified, the VM accumulates all the options' values, instead of 
ignoring all but the last option's value. The --add-modules values 
are reported back to the JDK as properties using the 
Arguments::create_numbered_property() function.


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

Open webrevs:

   http://cr.openjdk.java.net/~hseigel/bug_8165634.hs/

   http://cr.openjdk.java.net/~hseigel/bug_8165634.jdk/

The fix was tested with the JCK Lang and VM tests, the hotpot, and 
java/lang, java/util and other JTreg tests, the RBT tier2 tests, and 
the NSK non-colocated quick tests.


The JDK changes were done by Mandy Chung (mchung).


Hi Harold,
Looks good, thanks for removing 
Arguments::append_to_addmods_property!  One minor comment, shouldn't 
--patch-modules also be in the unsupported_options list for 
Arguments::check_unsupported_dumping_properties?

Thanks,
Lois



Thanks, Harold

Re: Review Request: JDK-8165346 j.l.ClassLoader.getDefinedPackage(String) throws NPE

[Alan Bateman]

On 08/09/2016 23:02, Mandy Chung wrote:


Spec bug: missing @throws NPE if the specified name is null in the relevant 
getPackage methods:
   ClassLoader.getDefinedPackage, ClassLoader::getPackage, Package::getPackage

Webrev at:
   http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8165346/webrev.00/index.html


This looks okay to me.

-Alan

Re: RFR(s): 4285505: deprecate java.lang.Compiler

[Roger Riggs]

Hi Stuart,

Related to java.lang.Complier there is the System property 
"java.compiler" [1].
Is there some notation needed on the property or will it just go poof 
when the compiler is removed?


Thanks, Roger

[1] JDK-8041676  
java.compiler property is uninitialized



On 9/7/2016 4:52 PM, Stuart Marks wrote:

Hi all,

Please review this small patch to deprecate java.lang.Compiler for 
removal.


Thanks,

s'marks

# HG changeset patch
# User smarks
# Date 1473281459 25200
#  Wed Sep 07 13:50:59 2016 -0700
# Node ID e520c4e6970c079573bad20c6b1eba8d5794b34d
# Parent  76ba1b74f268f1acc4847e242a2cfcd29880418c
4285505: deprecate java.lang.Compiler
Reviewed-by: XXX

diff -r 76ba1b74f268 -r e520c4e6970c 
src/java.base/share/classes/java/lang/Compiler.java
--- a/src/java.base/share/classes/java/lang/Compiler.javaTue Sep 
06 16:08:54 2016 -0700
+++ b/src/java.base/share/classes/java/lang/Compiler.javaWed Sep 
07 13:50:59 2016 -0700

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1995, 2014, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 1995, 2016, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -29,21 +29,18 @@
  * The {@code Compiler} class is provided to support Java-to-native-code
  * compilers and related services. By design, the {@code Compiler} 
class does
  * nothing; it serves as a placeholder for a JIT compiler 
implementation.

+ * If no compiler is available, these methods do nothing.
  *
- *  When the Java Virtual Machine first starts, it determines if 
the system
- * property {@code java.compiler} exists. (System properties are 
accessible

- * through {@link System#getProperty(String)} and {@link
- * System#getProperty(String, String)}.  If so, it is assumed to be 
the name of

- * a library (with a platform-dependent exact location and type); {@link
- * System#loadLibrary} is called to load that library. If this loading
- * succeeds, the function named {@code java_lang_Compiler_start()} in 
that

- * library is called.
- *
- *  If no compiler is available, these methods do nothing.
+ * @deprecated JIT compilers and their technologies vary too widely to
+ * be controlled effectively by a standardized interface. As such, many
+ * JIT compiler implementations ignore this interface, and are instead
+ * controllable by implementation-specific mechanisms such as 
command-line
+ * options. This class is subject to removal in a future version of 
Java SE.

  *
  * @author  Frank Yellin
  * @since   1.0
  */
+@Deprecated(since="9", forRemoval=true)
 public final class Compiler  {
 private Compiler() {}   // don't make instances

Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base

[Erik Joelsson]

Build change looks ok.

/Erik


On 2016-09-08 17:37, Naoto Sato wrote:

Updated the webrev wrt the latter comment:

http://cr.openjdk.java.net/~naoto/8165605/webrev.02/

Naoto

On 9/7/16 6:37 PM, Mandy Chung wrote:



On Sep 7, 2016, at 6:29 PM, Naoto Sato  wrote:

Hi Mandy,

Although avoiding the hardcoded pathname is good, it is specific to 
the BreakIterator implementation of the COMPAT provider. So I am not 
sure making a generic SPI would be needed here.


I was thinking of one of the internal services that jdk.localedata 
currently provides.




Anyway, this split package issue is blocking Alan's push, so I'd 
like to push the change as it is. We can get back to this later.


I agree this can be cleaned up as a separate issue.

 152 InputStream is = module.getResourceAsStream(
 153 ("jdk.localedata".equals(module.getName()) ?
 154  "sun/text/resources/ext/" : 
"sun/text/resources/") + dictionaryName);


It may be easier to read if line 153-154 are moved and assign to a 
separate variable.  Otherwise, looks fine.


Mandy



Naoto

On 9/7/16 5:17 PM, Mandy Chung wrote:

Hi Naoto,

Is there an alternative to get back the pathname of the resource 
e.g. adding a method in existing internal SPI to avoid hardcoding 
the module name and the resource pathname.


Mandy


On Sep 7, 2016, at 3:56 PM, Naoto Sato  wrote:

Forgot to include jlink plugin changes. Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8165605/webrev.01/

Naoto

On 9/7/16 3:03 PM, Naoto Sato wrote:

Please review the changes to the subject bug:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8165605/webrev.00/

The change is simply to move those 3 resources under
sun.text.resources.ext package so that it won't cause the split 
package

issue.

Naoto

Re: RFR 9: 8155760 Implement Serialization Filtering

[Roger Riggs]

Hi Andrej,

Thanks for the review and comments..

On 9/9/2016 2:32 AM, Andrej Golovnin wrote:

Hi Roger,

src/java.base/share/classes/java/io/ObjectInputStream.java

259 private static class Logging {

The class can be final.
But there is no advantage or limitation since it is an private 
implementation class.


1265 ?  Logger.Level.DEBUG

There is one space too much before "Logger".

removed


2611 /** total bytes read from the stream */
2612 private int totalBytesRead = 0;

I think the type of the field totalBytesRead must be long. In the
#skip(long)-method you update it with a long value.

ok

I'll fix these before pushing.

Thanks, Roger



Best regards,
Andrej Golovnin

[8u-dev] Request for Review and Approval to backport: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Ivan Gerasimov]

Hello!

I'd like to backport this fix to jdk8u.

The unshuffled fix applies almost clearly: I only had to slightly modify 
the regression test.


Bug: https://bugs.openjdk.java.net/browse/JDK-8165243
Jdk9 change: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/a81f30fb7d8c
Jdk9 review: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-September/043266.html

Jdk8u webrev: http://cr.openjdk.java.net/~igerasim/8165243/03/webrev/

Would you please help review the change and approve the backport?

Thanks in advance,
Ivan

Re: [8u-dev] Request for Review and Approval to backport: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Seán Coffey]

Looks fine Ivan. Reviewed.

Approved for jdk8u-dev.

Regards,
Sean.

On 09/09/16 16:43, Ivan Gerasimov wrote:

Hello!

I'd like to backport this fix to jdk8u.

The unshuffled fix applies almost clearly: I only had to slightly 
modify the regression test.


Bug: https://bugs.openjdk.java.net/browse/JDK-8165243
Jdk9 change: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/a81f30fb7d8c
Jdk9 review: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-September/043266.html

Jdk8u webrev: http://cr.openjdk.java.net/~igerasim/8165243/03/webrev/

Would you please help review the change and approve the backport?

Thanks in advance,
Ivan

Re: [8u-dev] Request for Review and Approval to backport: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Ivan Gerasimov]

Thank you very much Seán!


On 09.09.2016 18:50, Seán Coffey wrote:

Looks fine Ivan. Reviewed.

Approved for jdk8u-dev.

Regards,
Sean.

On 09/09/16 16:43, Ivan Gerasimov wrote:

Hello!

I'd like to backport this fix to jdk8u.

The unshuffled fix applies almost clearly: I only had to slightly 
modify the regression test.


Bug: https://bugs.openjdk.java.net/browse/JDK-8165243
Jdk9 change: http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/a81f30fb7d8c
Jdk9 review: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-September/043266.html

Jdk8u webrev: http://cr.openjdk.java.net/~igerasim/8165243/03/webrev/

Would you please help review the change and approve the backport?

Thanks in advance,
Ivan

Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base

[Naoto Sato]

The build tool is getting the output directory as a parameter and that 
also need to be tweaked. That output directory path is specified in the 
makefile which I wanted to avoid.


Naoto

On 9/8/16 4:31 PM, Masayoshi Okutsu wrote:

Is it just a matter of an extra step, new File(path).getName()?

Masayoshi


On 9/9/2016 12:00 AM, Naoto Sato wrote:

Well, actually I tried this approach first, then found out that the
data files are also used by the GenerateBreakIteratorData build tool
which has some implicit assumption of the value being the file name
w/o path. So I ended up with the fix.

Naoto

On 9/8/16 5:46 AM, Alan Bateman wrote:



On 08/09/2016 03:51, Masayoshi Okutsu wrote:

I thought Mandy suggested that the dictionary names in a
ResourceBundle contain path names rather than base names, something
like this:

In BreakIteratorInfo_th.java:

{"WordDictionary", "thai_dict"},
to
{"WordDictionary", "sun/text/resources/ext/thai_dict"},

I haven't checked any side effects of this change, though.

That would be cleaner, assuming it doesn't cause issues.

-Alan

RFR 8165726: fix for 8165595 revealed a bug in pack200 tool's handling of main class attribute of module-info classes

[Sundararajan Athijegannathan]

Please review http://cr.openjdk.java.net/~sundar/8165726/webrev.01/ for
https://bugs.openjdk.java.net/browse/JDK-8165726

Thanks,

-Sundar

Re: RFR 8165726: fix for 8165595 revealed a bug in pack200 tool's handling of main class attribute of module-info classes

[Kumar Srinivasan]

Looks good, thanks for taking care of it.

We need to double check these again, I will take a closer look
later.

Kumar

On 9/9/2016 9:36 AM, Sundararajan Athijegannathan wrote:

Please review http://cr.openjdk.java.net/~sundar/8165726/webrev.01/ for
https://bugs.openjdk.java.net/browse/JDK-8165726

Thanks,

-Sundar

Re: RFR(s): 4285505: deprecate java.lang.Compiler

[Stuart Marks]

Hi Roger,

Thanks for mentioning bug JDK-8041676.

As far as I understand, the java.compiler property is intended to be set by the 
user in order to tell the JVM which JIT compiler to use. This is hinted at in 
the text I'm removing from the java.lang.Compiler spec in this changeset.


The java.compiler property is also mentioned in j.l.System.getProperties(), 
where it simply says


java.compiler Name of JIT compiler to use

which is pretty inconclusive.

Bug JDK-8041676 is strange in that it implies that something in the JDK should 
be setting this property. But I think that's wrong.


I did a quick survey, and there are bunch of places that set 
-Djava.compiler=none in test code, demos, and in various checked-in NetBeans 
projects, but the only place that seems to read it is 
hotspot/src/share/vm/runtime/arguments.cpp where the specific value of "none" 
for this property is treated as a synonym for -Xint (run in interpretive mode). 
This still seems to be active.


I'll update JDK-8041676 with these findings. I don't think java.lang.Compiler 
deprecation has any impact on the java.compiler property, though, except to 
remove some long-obsolete text.


s'marks



On 9/9/16 6:55 AM, Roger Riggs wrote:

Hi Stuart,

Related to java.lang.Complier there is the System property "java.compiler" [1].
Is there some notation needed on the property or will it just go poof when the
compiler is removed?

Thanks, Roger

[1] JDK-8041676  java.compiler
property is uninitialized


On 9/7/2016 4:52 PM, Stuart Marks wrote:

Hi all,

Please review this small patch to deprecate java.lang.Compiler for removal.

Thanks,

s'marks

# HG changeset patch
# User smarks
# Date 1473281459 25200
#  Wed Sep 07 13:50:59 2016 -0700
# Node ID e520c4e6970c079573bad20c6b1eba8d5794b34d
# Parent  76ba1b74f268f1acc4847e242a2cfcd29880418c
4285505: deprecate java.lang.Compiler
Reviewed-by: XXX

diff -r 76ba1b74f268 -r e520c4e6970c
src/java.base/share/classes/java/lang/Compiler.java
--- a/src/java.base/share/classes/java/lang/Compiler.javaTue Sep 06
16:08:54 2016 -0700
+++ b/src/java.base/share/classes/java/lang/Compiler.javaWed Sep 07
13:50:59 2016 -0700
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1995, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2016, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -29,21 +29,18 @@
  * The {@code Compiler} class is provided to support Java-to-native-code
  * compilers and related services. By design, the {@code Compiler} class does
  * nothing; it serves as a placeholder for a JIT compiler implementation.
+ * If no compiler is available, these methods do nothing.
  *
- *  When the Java Virtual Machine first starts, it determines if the system
- * property {@code java.compiler} exists. (System properties are accessible
- * through {@link System#getProperty(String)} and {@link
- * System#getProperty(String, String)}.  If so, it is assumed to be the name of
- * a library (with a platform-dependent exact location and type); {@link
- * System#loadLibrary} is called to load that library. If this loading
- * succeeds, the function named {@code java_lang_Compiler_start()} in that
- * library is called.
- *
- *  If no compiler is available, these methods do nothing.
+ * @deprecated JIT compilers and their technologies vary too widely to
+ * be controlled effectively by a standardized interface. As such, many
+ * JIT compiler implementations ignore this interface, and are instead
+ * controllable by implementation-specific mechanisms such as command-line
+ * options. This class is subject to removal in a future version of Java SE.
  *
  * @author  Frank Yellin
  * @since   1.0
  */
+@Deprecated(since="9", forRemoval=true)
 public final class Compiler  {
 private Compiler() {}   // don't make instances

RFR(XS): 8165393: bad merge in java/lang/ref/package-info.java

[Kim Barrett]

Please review this fix of a merge error in the package-info for
java.lang.ref.

This is a doc-only change; per the RDP1 process the CR has been
labeled "noreg-doc".

CR:
https://bugs.openjdk.java.net/browse/JDK-8165393

Webrev:
http://cr.openjdk.java.net/~kbarrett/8165393/webrev/

Re: RFR 8165726: fix for 8165595 revealed a bug in pack200 tool's handling of main class attribute of module-info classes

[Alan Bateman]

On 09/09/2016 19:15, Kumar Srinivasan wrote:

Looks good, thanks for taking care of it.

We need to double check these again, I will take a closer look
later.
It's a CONSTANT_Class_info so I think it's right. It would be good to 
check the others as it's easy to get these wrong, maybe additional 
comments at the topic would help.


-Alan

Re: RFR(XS): 8165393: bad merge in java/lang/ref/package-info.java

[Roger Riggs]

Hi Kim,

Looks good, thanks for the correction.

Roger


On 9/9/2016 4:23 PM, Kim Barrett wrote:

Please review this fix of a merge error in the package-info for
java.lang.ref.

This is a doc-only change; per the RDP1 process the CR has been
labeled "noreg-doc".

CR:
https://bugs.openjdk.java.net/browse/JDK-8165393

Webrev:
http://cr.openjdk.java.net/~kbarrett/8165393/webrev/

Re: RFR(XS): 8165393: bad merge in java/lang/ref/package-info.java

[Kim Barrett]

> On Sep 9, 2016, at 4:27 PM, Roger Riggs  wrote:
> 
> Hi Kim,
> 
> Looks good, thanks for the correction.

Thanks.

> 
> Roger
> 
> 
> On 9/9/2016 4:23 PM, Kim Barrett wrote:
>> Please review this fix of a merge error in the package-info for
>> java.lang.ref.
>> 
>> This is a doc-only change; per the RDP1 process the CR has been
>> labeled "noreg-doc".
>> 
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8165393
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8165393/webrev/

Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base

[Naoto Sato]

While at it, I noticed two build issues and updated the webrev. They are 
not directly related to the split package issue per se, but related to 
Thai breakiterator:


1) BreakIteratorRules_th.class slipped into the product image, which 
shouldn't.


2) Removed BreakIteratorRulesProvider.java which is not needed, as the 
class above is only used at the build time and not through module.


Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8165605/webrev.03/

Naoto

On 9/9/16 7:15 AM, Erik Joelsson wrote:

Build change looks ok.

/Erik


On 2016-09-08 17:37, Naoto Sato wrote:

Updated the webrev wrt the latter comment:

http://cr.openjdk.java.net/~naoto/8165605/webrev.02/

Naoto

On 9/7/16 6:37 PM, Mandy Chung wrote:



On Sep 7, 2016, at 6:29 PM, Naoto Sato  wrote:

Hi Mandy,

Although avoiding the hardcoded pathname is good, it is specific to
the BreakIterator implementation of the COMPAT provider. So I am not
sure making a generic SPI would be needed here.


I was thinking of one of the internal services that jdk.localedata
currently provides.



Anyway, this split package issue is blocking Alan's push, so I'd
like to push the change as it is. We can get back to this later.


I agree this can be cleaned up as a separate issue.

 152 InputStream is = module.getResourceAsStream(
 153 ("jdk.localedata".equals(module.getName()) ?
 154  "sun/text/resources/ext/" :
"sun/text/resources/") + dictionaryName);

It may be easier to read if line 153-154 are moved and assign to a
separate variable.  Otherwise, looks fine.

Mandy



Naoto

On 9/7/16 5:17 PM, Mandy Chung wrote:

Hi Naoto,

Is there an alternative to get back the pathname of the resource
e.g. adding a method in existing internal SPI to avoid hardcoding
the module name and the resource pathname.

Mandy


On Sep 7, 2016, at 3:56 PM, Naoto Sato  wrote:

Forgot to include jlink plugin changes. Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8165605/webrev.01/

Naoto

On 9/7/16 3:03 PM, Naoto Sato wrote:

Please review the changes to the subject bug:

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

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8165605/webrev.00/

The change is simply to move those 3 resources under
sun.text.resources.ext package so that it won't cause the split
package
issue.

Naoto

Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base

[Mandy Chung]

> On Sep 9, 2016, at 2:58 PM, Naoto Sato  wrote:
> 
> While at it, I noticed two build issues and updated the webrev. They are not 
> directly related to the split package issue per se, but related to Thai 
> breakiterator:
> 
> 1) BreakIteratorRules_th.class slipped into the product image, which 
> shouldn't.
> 
> 2) Removed BreakIteratorRulesProvider.java which is not needed, as the class 
> above is only used at the build time and not through module.
> 
> Here is the updated webrev:
> 
> http://cr.openjdk.java.net/~naoto/8165605/webrev.03/

Looks fine.  

It’d be good to file a follow-on issue and investigate what it takes to 
implement Masayoshi’s suggestion - have BreakIterators::getObject to return 
dictionary data instead of a resource name.

Mandy

RFR: 8163798: Create a JarFile versionedStream method

[Steve Drach]

Hi,

Please review this changeset that adds a VersionedStream class to the 
jdk.internal.util.jar package.  Some may recall that I submitted a similar RFR 
a few weeks ago; this is a redesign from that one.  We decided not to make a 
public JarFile::versionedStream method at this time.  Once we get sufficient 
experience with this and find a few more use cases, we will revisit the idea of 
making this a public method in JarFile.

issue: https://bugs.openjdk.java.net/browse/JDK-8163798 

webrev: http://cr.openjdk.java.net/~sdrach/8163798/webrev.01/index.html 


Thanks,
Steve

Re: [9] RFR: 8165605: Thai resources in jdk.localedata cause split package issue with java.base

[Naoto Sato]

Thanks. Created an issue for the follow-on issue:

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

Naoto

On 9/9/16 3:34 PM, Mandy Chung wrote:



On Sep 9, 2016, at 2:58 PM, Naoto Sato  wrote:

While at it, I noticed two build issues and updated the webrev. They are not 
directly related to the split package issue per se, but related to Thai 
breakiterator:

1) BreakIteratorRules_th.class slipped into the product image, which shouldn't.

2) Removed BreakIteratorRulesProvider.java which is not needed, as the class 
above is only used at the build time and not through module.

Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8165605/webrev.03/


Looks fine.

It’d be good to file a follow-on issue and investigate what it takes to 
implement Masayoshi’s suggestion - have BreakIterators::getObject to return 
dictionary data instead of a resource name.

Mandy

RFR: 8165723: JarFile::isMultiRelease() method returns false when it should return true

[Claes Redestad]

Hi,

JDK-8152733 introduced a corner-case where isMultiRelease returns
false when it should return true due to an erroneously hand-crafted
Boyer-Moore search.

Webrev: http://cr.openjdk.java.net/~redestad/8165723/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8165723

Testing: created a test reproducer using randomly constructed
manifests and verified the supplied jar is properly detected as being
multi-release.

Thanks!

/Claes