Re: A bit of sugar for j.u.c.locks with try-with-resources?

[Krystal Mok]

Hi Vitaly,

Thanks for your comments!

On Fri, Sep 2, 2016 at 7:55 PM, Vitaly Davidovich  wrote:

> Hi Kris,
>
>
> On Friday, September 2, 2016, Krystal Mok  wrote:
>
>> Hi core-libs developers,
>>
>> I mostly live down in the VM world, but recently I've been playing with
>> j.u.c.locks a bit, and saw that there's an opportunity to retrofit the API
>> with the try-with-resources syntax. I wonder if anybody has brought this
>> topic up before; apologies if there had been.
>>
>> From the JavaDoc of j.u.c.l.ReentrantLock, the following is a typical
>> usage:
>>
>>  class X {
>>private final ReentrantLock lock = new ReentrantLock();
>>// ...
>>
>>public void m() {
>>  lock.lock();  // block until condition holds
>>  try {
>>// ... method body
>>  } finally {
>>lock.unlock()
>>  }
>>}
>>  }
>>
>> The try...finally construction really pops out as a try-with-resources
>> candidate.
>>
>> So what if we retrofit that with something like:
>>
>>  class X {
>>private final ReentrantLock lock = new ReentrantLock();
>>// ...
>>
>>public void m() {
>>  try (lock.lock()) { // block until condition holds
>>// ... method body
>>  }   // automatic unlock at the end
>>}
>>  }
>
> This is Java 9 syntax right? Java 7-8 require an assignment to a local in
> the TWR block.
>

Yes, this is Java 9 syntax, although I was actually intending to use the
old Java 7/8 syntax as the example...got too used to the new goodies.


>
>> Assuming lock.lock() returns a temporary wrapper object (let's call it a
>> "Locker" for this discussion), where Locker implements AutoCloseable, and
>> its close() method calls lock.unlock().
>> That'll make the API look and feel quite similar to the built-in
>> "synchronized () { ... }" syntax. With escape analysis and scalar
>> replacement implemented correctly in the VM, this temporary Locker object
>> wouldn't incur much (or any) runtime cost after optimized JIT'ing, so it
>> feels like a pure win to me.
>>
>> What do you think?
>
> So using TWR with scoped objects, such as this, is used quite a bit; I've
> seen this idiom many times, and have used it myself.
>
> Now, what's the value add to have this in the JDK? One can write such a
> Locker themselves quite easily.
>
> Having the Locker integrated into the API makes the experience smoother.
If we write our own wrapper, it might look like:

try (locker = new Locker(lock)) {
  // ... method body
}

Which is okay-ish. Or maybe with the help of some statically imported
helper method and the Java 9 syntax:

try (locker(lock)) {
  // ...
}

Hmm...I can live with that.

I also don't hold as much confidence in EA as you, apparently - it's too
> brittle and unpredictable in its current form, IMHO.  Of course when the
> allocation doesn't matter, that part isn't a big deal.
>
> Haha, I know how C2's EA isn't giving everybody the kind of confidence
they want. But for simple cases like this it should work nicely (and if
not, let's fix it :-)

And let's say when HotSpot gets a new JIT compiler, it'll have a more
effective EA. All the better.

Thanks,
Kris


> My $.02.
>
>>
>> Best regards,
>> Kris (OpenJDK username: kmo)
>>
>
>
> --
> Sent from my phone
>

Re: A bit of sugar for j.u.c.locks with try-with-resources?

[Vitaly Davidovich]

Hi Kris,

On Friday, September 2, 2016, Krystal Mok  wrote:

> Hi core-libs developers,
>
> I mostly live down in the VM world, but recently I've been playing with
> j.u.c.locks a bit, and saw that there's an opportunity to retrofit the API
> with the try-with-resources syntax. I wonder if anybody has brought this
> topic up before; apologies if there had been.
>
> From the JavaDoc of j.u.c.l.ReentrantLock, the following is a typical
> usage:
>
>  class X {
>private final ReentrantLock lock = new ReentrantLock();
>// ...
>
>public void m() {
>  lock.lock();  // block until condition holds
>  try {
>// ... method body
>  } finally {
>lock.unlock()
>  }
>}
>  }
>
> The try...finally construction really pops out as a try-with-resources
> candidate.
>
> So what if we retrofit that with something like:
>
>  class X {
>private final ReentrantLock lock = new ReentrantLock();
>// ...
>
>public void m() {
>  try (lock.lock()) { // block until condition holds
>// ... method body
>  }   // automatic unlock at the end
>}
>  }

This is Java 9 syntax right? Java 7-8 require an assignment to a local in
the TWR block.

>
> Assuming lock.lock() returns a temporary wrapper object (let's call it a
> "Locker" for this discussion), where Locker implements AutoCloseable, and
> its close() method calls lock.unlock().
> That'll make the API look and feel quite similar to the built-in
> "synchronized () { ... }" syntax. With escape analysis and scalar
> replacement implemented correctly in the VM, this temporary Locker object
> wouldn't incur much (or any) runtime cost after optimized JIT'ing, so it
> feels like a pure win to me.
>
> What do you think?

So using TWR with scoped objects, such as this, is used quite a bit; I've
seen this idiom many times, and have used it myself.

Now, what's the value add to have this in the JDK? One can write such a
Locker themselves quite easily.

I also don't hold as much confidence in EA as you, apparently - it's too
brittle and unpredictable in its current form, IMHO.  Of course when the
allocation doesn't matter, that part isn't a big deal.

My $.02.

>
> Best regards,
> Kris (OpenJDK username: kmo)
>


-- 
Sent from my phone

A bit of sugar for j.u.c.locks with try-with-resources?

[Krystal Mok]

Hi core-libs developers,

I mostly live down in the VM world, but recently I've been playing with
j.u.c.locks a bit, and saw that there's an opportunity to retrofit the API
with the try-with-resources syntax. I wonder if anybody has brought this
topic up before; apologies if there had been.

>From the JavaDoc of j.u.c.l.ReentrantLock, the following is a typical usage:

 class X {
   private final ReentrantLock lock = new ReentrantLock();
   // ...

   public void m() {
 lock.lock();  // block until condition holds
 try {
   // ... method body
 } finally {
   lock.unlock()
 }
   }
 }

The try...finally construction really pops out as a try-with-resources
candidate.

So what if we retrofit that with something like:

 class X {
   private final ReentrantLock lock = new ReentrantLock();
   // ...

   public void m() {
 try (lock.lock()) { // block until condition holds
   // ... method body
 }   // automatic unlock at the end
   }
 }

Assuming lock.lock() returns a temporary wrapper object (let's call it a
"Locker" for this discussion), where Locker implements AutoCloseable, and
its close() method calls lock.unlock().
That'll make the API look and feel quite similar to the built-in
"synchronized () { ... }" syntax. With escape analysis and scalar
replacement implemented correctly in the VM, this temporary Locker object
wouldn't incur much (or any) runtime cost after optimized JIT'ing, so it
feels like a pure win to me.

What do you think?

Best regards,
Kris (OpenJDK username: kmo)

Re: RFR(m): 8159404: immutable collections should throw UOE unconditionally

[Stuart Marks]

On 9/1/16 8:41 PM, Martin Buchholz wrote:

Looks good to me!

Thanks!
Another idea for another day: I would like the immutable collections to be 
more optimal than they currently are, even if we have to write more code.  It 
bugs me is that all of these collections have a modCount, despite never being 
modified!  (Even for mutable lists, I'm not sure the added safety of modCount 
was worth the price)


java -jar jol-cli-0.5-full.jar internals java.util.ImmutableCollections\$List1
...
java.util.ImmutableCollections$List1 object internals:
  012(object header)N/A
 12 4int AbstractList.modCount  N/A
 16 4 Object List1.e0   N/A

Yes, good idea. I've filed JDK-8165396 to track this.

s'marks

Re: Participating on https://bugs.openjdk.java.net/browse/JDK-8156070

[Stuart Marks]

Hi Jonathan,

Welcome to OpenJDK, and thanks for your interest in JEP 269!

I see you found a the subtasks of JDK-8156070, which is basically a container 
for a bunch of ideas for work on the JEP 269 collections. I took a quick look 
through them and this one seemed promising:


JDK-8134373 explore potential uses of convenience factories within the JDK

This isn't necessarily what you're looking for, as it's not working on the 
collections themselves, but instead it's attempting to put them to use within 
the JDK. I've added some notes to this bug about the dynamics of retrofitting 
usage into the JDK. Although it's not work directly on the collections, finding 
out where they can and cannot be applied would definitely be useful. It would 
also be useful, of course, if space savings could be realized by using them 
instead of conventional collections (where appropriate, of course).


I'd also suggest looking beyond the JEP 269 tasks and at broader collections 
issues. To query for unresolved collections issues, go to bugs.openjdk.java.net; 
click Issues > Search for Issues; click Advanced; and paste the following query 
into the search box:


project = jdk and component = core-libs and subcomponent = java.util:collections 
and resolution = unresolved


(You should be able to run this query without having a JIRA account.)

There's a wider variety of things here, which might turn up something more 
appropriate. (Then again, the choice might be overwhelming.)


As Mark Reinhold mentioned, we're now in the "ramp-down phase 1" part of the JDK 
9 schedule, so the proposal is to start restricting the kind of work that can be 
done. The low-priority bugs are probably off the table until JDK 10 (I'm not 
sure when that tree opens up), but some things like docs and tests are still 
fair game for the time being. (Also note that "the javadoc" is a mixture of 
specifications and informative documentation, and the differences between them 
aren't always apparent. Specification changes require a good deal more rigor, 
and they go through extra rounds of review.)


Anyway, I hope you find something of interest. I'm going heads-down to prepare 
for JavaOne (Sep 18-22) but I'll try to keep an eye out for followup messages.


s'marks

[1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-August/004777.html



On 9/2/16 5:59 AM, Jonathan Bluett-Duncan wrote:

Hi Stuart Marks,

I've been told by Martijn Verburg on the discuss mailing list that you're
the lead person for the Immutable Collections enhancements piece.

Is there anything I can do to help you out with https://bugs.openjdk.
java.net/browse/JDK-8156070 or any other part of JEP 269
?

Kind regards,
Jonathan

On 28 August 2016 at 17:25, Jonathan Bluett-Duncan 
wrote:


Hi all,

My name is Jonathan, and it's my first time posting to this mailing list.

I'm writing to you all as I'm rather interested in contributing to this
area of the OpenJDK. Particularly, I'm thinking I'd quite like to help out
with one of the subtasks for the Immutable Collections enhancements
. However I don't have
specific ideas and I don't know which of the subtasks would most closely
match my skills and knowledge, so I wondered if I could have some guidance
as to what I could do and how I can get started.

FYI, I've partially read the contribution instructions
, and I just submitted an OCA
earlier today, so I don't expect to be actually ready to participate until
I hear back from Oracle, but nonetheless I'd like to try to get the ball
rolling as soon as possible.

Kind regards,
Jonathan Bluett-Duncan

Re: RFR: 8151901: test/tools/pack200/Pack200Test fails on verifying native unpacked JAR

[John Rose]

> On Sep 2, 2016, at 10:14 AM, Kumar Srinivasan  
> wrote:
> 
> Hi John,
> 
> Please review the amended patch, based on our discussions.
> http://cr.openjdk.java.net/~ksrini/8151901/webrev.01/ 
> 
> 
> Highlights:
> * adjust the ordering of the InnerClass and BootStrapMethods computations
> * enabled the test, and disabled memory leak check, which causes intermittent
>   failures, because of GC's ergonomics etc.
> * added the two test files to golden.jar, just in case.
> 
> Thanks
> Kumar

Much better; I'm glad we figured it out.

This comment is wrong:
+// remove the attr previously set, otherwise add the bsm and
+// references as required
Should be merely:
+// add the bsm and references as required

Reviewed!

— John

Re: RFR: JDK-8161230 ClassLoader: add resource methods returning java.util.stream.Stream

[Mandy Chung]

> On Sep 2, 2016, at 2:50 AM, Patrick Reinhart  wrote:
> 
> Updated the existing 
> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.01

ClassLoader::resources returning Stream is a good addition. 
 
1386  * {@code IOException} occur getting the next resource element, it 
must be 
1387  * wrapped into an {@code UncheckedIOException}.

This has been mentioned in the paragraph above. This can be dropped from 
@apiNote. And add:

@throws UncheckedIOException if I/O errors occur

1392  * @return  An stream of resource {@link java.net.URL {@code URL}}

{@code…} is not needed as @link generates the text in .. format.  
You can simply write:
   {@link java.net.URL URL}

typo s/An/A

I’m unsure if “resource URL” clearly describes it.  What about
   @return A stream of {@code URL} representing the location of the resources 
with the given name.

1399  * @since  1.9

should be @since 9.

I have reservation in adding ClassLoader::systemResources static method.

ClassLoader::getSystemResources is a convenient method that is equivalent to 
calling ClassLoader::getSystemClassLoader().getResources(), since the system 
class loader is never null.  This method includes the resources in the 
application’s classpath.

With the new ClassLoader::getPlatformClassLoader() method, a better way to get 
the “system” resources from JDK (not the ones from the classpath), it can call:
   ClassLoader::getPlatformClassLoader().resources();

To get a Stream of URL of the resources visible to system class loader, it can 
call the new resources method:
   ClassLoader::getSystemClassLoader().resources();

So it doesn’t seem that the proposed systemResources static method is necessary.

I suggest to combine the two tests in a single test, ResourcesStreamTest and 
add @bug JBS-id

ResourcesSuccessCase

  46 List resources = stream.collect(Collectors.toList());
  52 URL url = resources.get(0);

You can check the size first.  This can be simplified to use filter/findFirst 
to file the expected URL (yes need to call cl.resources again that is not an 
issue).

Mandy
P.S. process wide - RDP1 starts.  This enhancement would need a sponsor and 
also request for approval [1].  I’m off for the long weekend and will follow up 
next Tuesday on this.

[1] http://openjdk.java.net/projects/jdk9/fc-extension-process

Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class

[harold seigel]

On 9/2/2016 4:17 PM, fo...@univ-mlv.fr wrote:

- Mail original -

De: "harold seigel" 
À: "Remi Forax" 
Cc: "Alan Bateman" , "Hotspot dev runtime" 
,
core-libs-dev@openjdk.java.net
Envoyé: Vendredi 2 Septembre 2016 20:32:55
Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private 
class from VM anonymous class
Hi Rémi,

Thank you for looking at this change.

Not allowing host classes to be array classes is not completely
unrelated to this bug because it affects the implementation of the code
that prepends the host class's package to the anonymous class.

yes, right.
but i've always believed that the name was more for debugging purpose,
i.e. because a VM anonymous class name is not registered in a Classloader,
so the VM will never find an anonymous class by it's name.
Having a package name that matches its host class's ensures that the 
anonymous class is in the same module as its host class in cases where 
the VM determines a class's module from its Klass's package entry structure.


Harold



We decided to not allow array host classes in JDK-9 because it makes no
sense.  A user who does this is likely doing so in error, and should be
flagged for it.

yes, true.


We recognize that this, and many other things, will have to change once
array classes have their own methods.

Thanks, Harold

Thanks for the explanation,
Rémi



On 9/2/2016 11:25 AM, Remi Forax wrote:

Harold,
disallowing array classes as host classes seems unrelated and knowing that jdk
10 or 11 will certainly add default methods to arrays,
we will want to have anonymous classes with arrays as host class in order to
acts as bridges/mixins.

regards,
Rémi

- Mail original -

De: "harold seigel" 
À: "Alan Bateman" , "Hotspot dev runtime"
,
core-libs-dev@openjdk.java.net
Envoyé: Vendredi 2 Septembre 2016 17:03:34
Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private
class from VM anonymous class
Thanks Alan.  I'll go ahead and make that change.

Harold


On 9/2/2016 10:43 AM, Alan Bateman wrote:

On 02/09/2016 14:02, harold seigel wrote:

Hi,

Please review this new fix for JDK-8058575.  This fix requires that a
VM anonymous class be in either the same package as its host class or
be in the unnamed package.  If the anonymous class is in the unnamed
package then this fix puts it into its host class's package, ensuring
that the anonymous class and its host class are in the same module.
This fix also throws an IllegalArgumentException if the host class is
an array class.

Additionally, the type of field ClassFileParser::_host_klass was
changed to InstanceKlass* and some comments were cleaned up.

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

Open webrevs:

 http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/

In GetModuleTest then one clean-up is to change it to use
hostClass.getPackageName() and remove packageName(String).

-Alan

Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class

[forax]

- Mail original -
> De: "harold seigel" 
> À: "Remi Forax" 
> Cc: "Alan Bateman" , "Hotspot dev runtime" 
> ,
> core-libs-dev@openjdk.java.net
> Envoyé: Vendredi 2 Septembre 2016 20:32:55
> Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private 
> class from VM anonymous class

> Hi Rémi,
> 
> Thank you for looking at this change.
> 
> Not allowing host classes to be array classes is not completely
> unrelated to this bug because it affects the implementation of the code
> that prepends the host class's package to the anonymous class.

yes, right.
but i've always believed that the name was more for debugging purpose,
i.e. because a VM anonymous class name is not registered in a Classloader,
so the VM will never find an anonymous class by it's name.

> 
> We decided to not allow array host classes in JDK-9 because it makes no
> sense.  A user who does this is likely doing so in error, and should be
> flagged for it.

yes, true.

> 
> We recognize that this, and many other things, will have to change once
> array classes have their own methods.
> 
> Thanks, Harold

Thanks for the explanation,
Rémi

> 
> 
> On 9/2/2016 11:25 AM, Remi Forax wrote:
>> Harold,
>> disallowing array classes as host classes seems unrelated and knowing that 
>> jdk
>> 10 or 11 will certainly add default methods to arrays,
>> we will want to have anonymous classes with arrays as host class in order to
>> acts as bridges/mixins.
>>
>> regards,
>> Rémi
>>
>> - Mail original -
>>> De: "harold seigel" 
>>> À: "Alan Bateman" , "Hotspot dev runtime"
>>> ,
>>> core-libs-dev@openjdk.java.net
>>> Envoyé: Vendredi 2 Septembre 2016 17:03:34
>>> Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private
>>> class from VM anonymous class
>>> Thanks Alan.  I'll go ahead and make that change.
>>>
>>> Harold
>>>
>>>
>>> On 9/2/2016 10:43 AM, Alan Bateman wrote:

 On 02/09/2016 14:02, harold seigel wrote:
> Hi,
>
> Please review this new fix for JDK-8058575.  This fix requires that a
> VM anonymous class be in either the same package as its host class or
> be in the unnamed package.  If the anonymous class is in the unnamed
> package then this fix puts it into its host class's package, ensuring
> that the anonymous class and its host class are in the same module.
> This fix also throws an IllegalArgumentException if the host class is
> an array class.
>
> Additionally, the type of field ClassFileParser::_host_klass was
> changed to InstanceKlass* and some comments were cleaned up.
>
> JBS bug: https://bugs.openjdk.java.net/browse/JDK-8058575
>
> Open webrevs:
>
> http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/
 In GetModuleTest then one clean-up is to change it to use
 hostClass.getPackageName() and remove packageName(String).

> >>> -Alan

Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class

[harold seigel]

Hi Rémi,

Thank you for looking at this change.

Not allowing host classes to be array classes is not completely 
unrelated to this bug because it affects the implementation of the code 
that prepends the host class's package to the anonymous class.


We decided to not allow array host classes in JDK-9 because it makes no 
sense.  A user who does this is likely doing so in error, and should be 
flagged for it.


We recognize that this, and many other things, will have to change once 
array classes have their own methods.


Thanks, Harold


On 9/2/2016 11:25 AM, Remi Forax wrote:

Harold,
disallowing array classes as host classes seems unrelated and knowing that jdk 
10 or 11 will certainly add default methods to arrays,
we will want to have anonymous classes with arrays as host class in order to 
acts as bridges/mixins.

regards,
Rémi

- Mail original -

De: "harold seigel" 
À: "Alan Bateman" , "Hotspot dev runtime" 
,
core-libs-dev@openjdk.java.net
Envoyé: Vendredi 2 Septembre 2016 17:03:34
Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private 
class from VM anonymous class
Thanks Alan.  I'll go ahead and make that change.

Harold


On 9/2/2016 10:43 AM, Alan Bateman wrote:


On 02/09/2016 14:02, harold seigel wrote:

Hi,

Please review this new fix for JDK-8058575.  This fix requires that a
VM anonymous class be in either the same package as its host class or
be in the unnamed package.  If the anonymous class is in the unnamed
package then this fix puts it into its host class's package, ensuring
that the anonymous class and its host class are in the same module.
This fix also throws an IllegalArgumentException if the host class is
an array class.

Additionally, the type of field ClassFileParser::_host_klass was
changed to InstanceKlass* and some comments were cleaned up.

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

Open webrevs:

http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/

In GetModuleTest then one clean-up is to change it to use
hostClass.getPackageName() and remove packageName(String).

-Alan

Re: [jdk9] (XS) RFR: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Roger Riggs]

+1

On 9/2/2016 12:39 PM, Ivan Gerasimov wrote:

Roger and Alan, thanks for suggestions!

I incorporated most of them:
http://cr.openjdk.java.net/~igerasim/8165243/02/webrev/




Is it good to go?
Using RandomFactory looks okay although more awkward to run the test 
standalone, I assume rnd should be final.


Since you changing a lot of usages then personally I have input 
stream named "in" rather than "is" easier to read.


But there are also os, baos, bais around, so changing only is to in 
would be inconsistent.



I agree with Roger on making the exception messages clearer.

Sure, I made them clearer, as suggested.
Hopefully, we won't see them too often :)

A minor comment but the method names in the test are a bit 
inconsistent, "Encoder" vs "Enc" for example.


Yes, changed to full names and got rid of new checkXXX methods, as 
they weren't really needed.


With kind regards,
Ivan

Re: RFR: 8151901: test/tools/pack200/Pack200Test fails on verifying native unpacked JAR

[Kumar Srinivasan]

Hi John,

Please review the amended patch, based on our discussions.
http://cr.openjdk.java.net/~ksrini/8151901/webrev.01/

Highlights:
* adjust the ordering of the InnerClass and BootStrapMethods computations
* enabled the test, and disabled memory leak check, which causes 
intermittent

  failures, because of GC's ergonomics etc.
* added the two test files to golden.jar, just in case.

Thanks
Kumar




Good.

Please change the comment:
s/null, no tuple change, force recomputation/null, drop ICs attribute, force CP 
recomputation/

Reordering BSMs and ICs should work.
The BSMs may need extra ICs, but ICs cannot depend on BSMs.
I wonder why we did the other order first?  I guess because that is what the 
spec. says.

Oh, there's a problem with the attribute insertion order logic; it's buggy that 
we unconditionally
insert a BSMs attr. and then delete it later.  (That goes wrong when change<0 
and we recompute
from scratch.)

Suggested amended patch enclosed.

--- John

On Mar 21, 2016, at 12:49 PM, Kumar Srinivasan  
wrote:

Hi John,

This patch contains fixes for two bugs:
8151901: test/tools/pack200/Pack200Test fails on verifying native unpacked JAR
8062335: Pack200 Java implementation differs from C version, outputs unused 
UTF8 for BootstrapMethods Note: The test case exhibits both of the above.

With this  the classes produced by java and the native implementation are 
congruent,
though the JDK image's classes exhibit these issues, as a precaution I have 
extracted the
minimal set of classes to reproduce the issues, into the golden jar.

The webrev is at:
http://cr.openjdk.java.net/~ksrini/8151901/webrev.00/

Thanks
Kumar




diff --git 
a/src/java.base/share/classes/com/sun/java/util/jar/pack/Package.java 
b/src/java.base/share/classes/com/sun/java/util/jar/pack/Package.java
--- a/src/java.base/share/classes/com/sun/java/util/jar/pack/Package.java
+++ b/src/java.base/share/classes/com/sun/java/util/jar/pack/Package.java
@@ -312,6 +312,12 @@
  void setBootstrapMethods(Collection bsms) {
  assert(bootstrapMethods == null);  // do not do this twice
  bootstrapMethods = new ArrayList<>(bsms);
+// Edit the attribute list, if necessary.
+Attribute a = getAttribute(attrBootstrapMethodsEmpty);
+if (bootstrapMethods != null && a == null)
+addAttribute(attrBootstrapMethodsEmpty.canonicalInstance());
+else if (bootstrapMethods == null && a != null)
+removeAttribute(a);
  }
  
  boolean hasInnerClasses() {

diff --git 
a/src/java.base/share/classes/com/sun/java/util/jar/pack/PackageReader.java 
b/src/java.base/share/classes/com/sun/java/util/jar/pack/PackageReader.java
--- a/src/java.base/share/classes/com/sun/java/util/jar/pack/PackageReader.java
+++ b/src/java.base/share/classes/com/sun/java/util/jar/pack/PackageReader.java
@@ -1193,16 +1193,25 @@
  cls.visitRefs(VRM_CLASSIC, cpRefs);
  
  ArrayList bsms = new ArrayList<>();

-/*
- * BootstrapMethod(BSMs) are added here before InnerClasses(ICs),
- * so as to ensure the order. Noting that the BSMs  may be
- * removed if they are not found in the CP, after the ICs expansion.
- */
-
cls.addAttribute(Package.attrBootstrapMethodsEmpty.canonicalInstance());
  
  // flesh out the local constant pool

  ConstantPool.completeReferencesIn(cpRefs, true, bsms);
  
+// Add the BSMs and references as required.

+if (!bsms.isEmpty()) {
+// Add the attirbute name to the CP (visitRefs would have wanted 
this).
+cpRefs.add(Package.getRefString("BootstrapMethods"));
+// The pack-spec requires that BootstrapMethods precedes the 
InnerClasses attribute.
+// So add BSMs now before running expandLocalICs.
+// But first delete any local InnerClasses attr.  (This is rare.)
+List localICs = cls.getInnerClasses();
+cls.setInnerClasses(null);
+Collections.sort(bsms);
+cls.setBootstrapMethods(bsms);
+// Re-add the ICs, so the attribute lists are in the required 
order.
+cls.setInnerClasses(localICs);
+}
+
  // Now that we know all our local class references,
  // compute the InnerClasses attribute.
  int changed = cls.expandLocalICs();
@@ -1221,16 +1230,6 @@
  ConstantPool.completeReferencesIn(cpRefs, true, bsms);
  }
  
-// remove the attr previously set, otherwise add the bsm and

-// references as required
-if (bsms.isEmpty()) {
-
cls.attributes.remove(Package.attrBootstrapMethodsEmpty.canonicalInstance());
-} else {
-cpRefs.add(Package.getRefString("BootstrapMethods"));
-Collections.sort(bsms);
-cls.setBootstrapMethods(bsms);
-}
-
  // construct a local 

Re: [jdk9] (XS) RFR: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Xueming Shen]

+1


On 09/02/2016 09:39 AM, Ivan Gerasimov wrote:

Roger and Alan, thanks for suggestions!

I incorporated most of them:
http://cr.openjdk.java.net/~igerasim/8165243/02/webrev/




Is it good to go?

Using RandomFactory looks okay although more awkward to run the test 
standalone, I assume rnd should be final.

Since you changing a lot of usages then personally I have input stream named "in" rather 
than "is" easier to read.


But there are also os, baos, bais around, so changing only is to in would be 
inconsistent.


I agree with Roger on making the exception messages clearer.

Sure, I made them clearer, as suggested.
Hopefully, we won't see them too often :)


A minor comment but the method names in the test are a bit inconsistent, "Encoder" vs 
"Enc" for example.


Yes, changed to full names and got rid of new checkXXX methods, as they weren't 
really needed.

With kind regards,
Ivan

Re: [jdk9] (XS) RFR: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Ivan Gerasimov]

Roger and Alan, thanks for suggestions!

I incorporated most of them:
http://cr.openjdk.java.net/~igerasim/8165243/02/webrev/




Is it good to go?
Using RandomFactory looks okay although more awkward to run the test 
standalone, I assume rnd should be final.


Since you changing a lot of usages then personally I have input stream 
named "in" rather than "is" easier to read.


But there are also os, baos, bais around, so changing only is to in 
would be inconsistent.



I agree with Roger on making the exception messages clearer.

Sure, I made them clearer, as suggested.
Hopefully, we won't see them too often :)

A minor comment but the method names in the test are a bit 
inconsistent, "Encoder" vs "Enc" for example.


Yes, changed to full names and got rid of new checkXXX methods, as they 
weren't really needed.


With kind regards,
Ivan

Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class

[Remi Forax]

Harold,
disallowing array classes as host classes seems unrelated and knowing that jdk 
10 or 11 will certainly add default methods to arrays,
we will want to have anonymous classes with arrays as host class in order to 
acts as bridges/mixins.

regards,
Rémi

- Mail original -
> De: "harold seigel" 
> À: "Alan Bateman" , "Hotspot dev runtime" 
> ,
> core-libs-dev@openjdk.java.net
> Envoyé: Vendredi 2 Septembre 2016 17:03:34
> Objet: Re: RFR 8058575: IllegalAccessError trying to access package-private   
> class from VM anonymous class

> Thanks Alan.  I'll go ahead and make that change.
> 
> Harold
> 
> 
> On 9/2/2016 10:43 AM, Alan Bateman wrote:
>>
>>
>> On 02/09/2016 14:02, harold seigel wrote:
>>> Hi,
>>>
>>> Please review this new fix for JDK-8058575.  This fix requires that a
>>> VM anonymous class be in either the same package as its host class or
>>> be in the unnamed package.  If the anonymous class is in the unnamed
>>> package then this fix puts it into its host class's package, ensuring
>>> that the anonymous class and its host class are in the same module.
>>> This fix also throws an IllegalArgumentException if the host class is
>>> an array class.
>>>
>>> Additionally, the type of field ClassFileParser::_host_klass was
>>> changed to InstanceKlass* and some comments were cleaned up.
>>>
>>> JBS bug: https://bugs.openjdk.java.net/browse/JDK-8058575
>>>
>>> Open webrevs:
>>>
>>>http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/
>> In GetModuleTest then one clean-up is to change it to use
>> hostClass.getPackageName() and remove packageName(String).
>>
> > -Alan

Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class

[harold seigel]

Thanks Alan.  I'll go ahead and make that change.

Harold


On 9/2/2016 10:43 AM, Alan Bateman wrote:



On 02/09/2016 14:02, harold seigel wrote:

Hi,

Please review this new fix for JDK-8058575.  This fix requires that a 
VM anonymous class be in either the same package as its host class or 
be in the unnamed package.  If the anonymous class is in the unnamed 
package then this fix puts it into its host class's package, ensuring 
that the anonymous class and its host class are in the same module.  
This fix also throws an IllegalArgumentException if the host class is 
an array class.


Additionally, the type of field ClassFileParser::_host_klass was 
changed to InstanceKlass* and some comments were cleaned up.


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

Open webrevs:

   http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/
In GetModuleTest then one clean-up is to change it to use 
hostClass.getPackageName() and remove packageName(String).


-Alan

Re: [jdk9] (XS) RFR: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Alan Bateman]

On 02/09/2016 09:59, Ivan Gerasimov wrote:


:


On 01.09.2016 20:39, Alan Bateman wrote:
TestBase64 is the unit test for this API, should the test be added to 
that instead?




Yes, it should be better.

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

I also added another test case to make sure the decoder doesn't expose 
the same bug.

A cleanup pass was performed to make the code more compact.

Is it good to go?
Using RandomFactory looks okay although more awkward to run the test 
standalone, I assume rnd should be final.


Since you changing a lot of usages then personally I have input stream 
named "in" rather than "is" easier to read.


I agree with Roger on making the exception messages clearer. A minor 
comment but the method names in the test are a bit inconsistent, 
"Encoder" vs "Enc" for example.


-Alan

Re: RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class

[Alan Bateman]

On 02/09/2016 14:02, harold seigel wrote:

Hi,

Please review this new fix for JDK-8058575.  This fix requires that a 
VM anonymous class be in either the same package as its host class or 
be in the unnamed package.  If the anonymous class is in the unnamed 
package then this fix puts it into its host class's package, ensuring 
that the anonymous class and its host class are in the same module.  
This fix also throws an IllegalArgumentException if the host class is 
an array class.


Additionally, the type of field ClassFileParser::_host_klass was 
changed to InstanceKlass* and some comments were cleaned up.


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

Open webrevs:

   http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/
In GetModuleTest then one clean-up is to change it to use 
hostClass.getPackageName() and remove packageName(String).


-Alan

Re: [jdk9] (XS) RFR: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Roger Riggs]

Hi Ivan,

In the TestBase64.java:

563 throw new RuntimeException("Expected IOOBEx not thrown"); And
598 throw new RuntimeException("Expected IOOBEx not thrown");

I would say "was not thrown" to make the cause clearer. 567 throw new 
RuntimeException("No output expected"); I suppose this will never fail 
but including the size in the exception message would speed debugging. 
Otherwise, looks fine Roger


On 9/2/16 4:59 AM, Ivan Gerasimov wrote:
Thank you Roger and Alan for review! On 01.09.2016 20:39, Alan Bateman 
wrote:
TestBase64 is the unit test for this API, should the test be added to 
that instead? 
Yes, it should be better. 
http://cr.openjdk.java.net/~igerasim/8165243/01/webrev/ I also added 
another test case to make sure the decoder doesn't expose the same 
bug. A cleanup pass was performed to make the code more compact. Is it 
good to go? With kind regards, Ivan 

RFR 8058575: IllegalAccessError trying to access package-private class from VM anonymous class

[harold seigel]

Hi,

Please review this new fix for JDK-8058575.  This fix requires that a VM 
anonymous class be in either the same package as its host class or be in 
the unnamed package.  If the anonymous class is in the unnamed package 
then this fix puts it into its host class's package, ensuring that the 
anonymous class and its host class are in the same module.  This fix 
also throws an IllegalArgumentException if the host class is an array 
class.


Additionally, the type of field ClassFileParser::_host_klass was changed 
to InstanceKlass* and some comments were cleaned up.


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

Open webrevs:

   http://cr.openjdk.java.net/~hseigel/bug_8058575.jdk.3/

   http://cr.openjdk.java.net/~hseigel/bug_8058575.hs.3/

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


Thanks, Harold

Re: Participating on https://bugs.openjdk.java.net/browse/JDK-8156070

[Jonathan Bluett-Duncan]

Hi Stuart Marks,

I've been told by Martijn Verburg on the discuss mailing list that you're
the lead person for the Immutable Collections enhancements piece.

Is there anything I can do to help you out with https://bugs.openjdk.
java.net/browse/JDK-8156070 or any other part of JEP 269
?

Kind regards,
Jonathan

On 28 August 2016 at 17:25, Jonathan Bluett-Duncan 
wrote:

> Hi all,
>
> My name is Jonathan, and it's my first time posting to this mailing list.
>
> I'm writing to you all as I'm rather interested in contributing to this
> area of the OpenJDK. Particularly, I'm thinking I'd quite like to help out
> with one of the subtasks for the Immutable Collections enhancements
> . However I don't have
> specific ideas and I don't know which of the subtasks would most closely
> match my skills and knowledge, so I wondered if I could have some guidance
> as to what I could do and how I can get started.
>
> FYI, I've partially read the contribution instructions
> , and I just submitted an OCA
> earlier today, so I don't expect to be actually ready to participate until
> I hear back from Oracle, but nonetheless I'd like to try to get the ball
> rolling as soon as possible.
>
> Kind regards,
> Jonathan Bluett-Duncan
>

Re: RFR: JDK-8161230 ClassLoader: add resource methods returning java.util.stream.Stream

[Patrick Reinhart]

On 2016-09-02 10:10, Tagir Valeev wrote:

Also small thing:


Spliterator.NONNULL & Spliterator.IMMUTABLE


Should be Spliterator.NONNULL | Spliterator.IMMUTABLE

With best regards,
Tagir Valeev.



Updated the existing 
http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.01


- Patrick

Re: [jdk9] (XS) RFR: 8165243: Base64.Encoder.wrap(os).write(byte[], int, int) with incorrect arguments should not produce output

[Ivan Gerasimov]

Thank you Roger and Alan for review!


On 01.09.2016 20:39, Alan Bateman wrote:
TestBase64 is the unit test for this API, should the test be added to 
that instead?




Yes, it should be better.

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

I also added another test case to make sure the decoder doesn't expose 
the same bug.

A cleanup pass was performed to make the code more compact.

Is it good to go?

With kind regards,
Ivan

Re: RFR: JDK-8161230 ClassLoader: add resource methods returning java.util.stream.Stream

[Tagir Valeev]

Also small thing:

> Spliterator.NONNULL & Spliterator.IMMUTABLE

Should be Spliterator.NONNULL | Spliterator.IMMUTABLE

With best regards,
Tagir Valeev.

On Fri, Sep 2, 2016 at 2:41 PM, Patrick Reinhart  wrote:

> On 2016-09-02 08:09, Andrej Golovnin wrote:
>
>> src/java.base/share/classes/java/lang/ClassLoader.java
>>
>> The constant RESOURCE_CHARACTERISTICS in the line 215 should be
>> defined near the #streamOf()-method. The distance between the line
>> 1412 where the #streamOf()-method is defined and the line 215 is just
>> too huge.
>>
>
> No problem, moved to the method itself
>
> Your patch seems to modify the JavaDocs of the methods #getParent(),
>> #getPlatformClassLoader(), #getSystemClassLoader(). But I don see how
>> it is related to the issue you try to solve.>
>>
>
> Was a merge issue, removed those
>
> test/java/lang/ClassLoader/resources/ResourcesFailureCase.java
>> test/java/lang/ClassLoader/resources/ResourcesSuccessCase.java
>>
>
> The indentation is broken. You use tabs. But in JDK you must use
>> spaces, see
>> http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.ht
>> ml#toc-indentation
>>
>
> Fixed those also.
>
> See updated webrev:
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.01
>
> - Patrick
>

Re: RFR: JDK-8161230 ClassLoader: add resource methods returning java.util.stream.Stream

[Patrick Reinhart]

On 2016-09-02 08:09, Andrej Golovnin wrote:

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

The constant RESOURCE_CHARACTERISTICS in the line 215 should be
defined near the #streamOf()-method. The distance between the line
1412 where the #streamOf()-method is defined and the line 215 is just
too huge.


No problem, moved to the method itself


Your patch seems to modify the JavaDocs of the methods #getParent(),
#getPlatformClassLoader(), #getSystemClassLoader(). But I don see how
it is related to the issue you try to solve.>


Was a merge issue, removed those


test/java/lang/ClassLoader/resources/ResourcesFailureCase.java
test/java/lang/ClassLoader/resources/ResourcesSuccessCase.java



The indentation is broken. You use tabs. But in JDK you must use
spaces, see
http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-indentation


Fixed those also.

See updated webrev:

http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.01

- Patrick

Re: RFR: JDK-8161230 ClassLoader: add resource methods returning java.util.stream.Stream

[Andrej Golovnin]

Hi Patrick,

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

The constant RESOURCE_CHARACTERISTICS in the line 215 should be
defined near the #streamOf()-method. The distance between the line
1412 where the #streamOf()-method is defined and the line 215 is just
too huge.

Your patch seems to modify the JavaDocs of the methods #getParent(),
#getPlatformClassLoader(), #getSystemClassLoader(). But I don see how
it is related to the issue you try to solve.


test/java/lang/ClassLoader/resources/ResourcesFailureCase.java
test/java/lang/ClassLoader/resources/ResourcesSuccessCase.java

The indentation is broken. You use tabs. But in JDK you must use
spaces, see 
http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html#toc-indentation

Best regards,
Andrej Golovnin

On Thu, Sep 1, 2016 at 10:06 PM, Patrick Reinhart  wrote:
> Hi Alan,
> Hi Paul,
>
> Here is the first revision of the implementation based on our earlier 
> conversation.
>
> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/webrev.00 
> 
>
> - Patrick