Re: RFR: JDK-8200436 - String::isBlank

[Jeremy Manson]

Blank can even mean U+2422, if it comes to that.

Jeremy

On Tue, May 15, 2018 at 12:58 PM Louis Wasserman 
wrote:

> Does this method offer significant benefits over the more general approach
> of String.codePoints().allMatch(Character::isWhitespace)?
>
> " there is a notion of all characters in > > isBlank that you do not have
> with isWhitespace."
> I'm not sure I follow.  I do tend to interpret "is this string whitespace?"
> as implying that it consists only of whitespace, whereas "blank" could mean
> "" or maybe even "___" but not obviously "   ".
>
> On Tue, May 15, 2018 at 11:19 AM Jonathan Bluett-Duncan <
> jbluettdun...@gmail.com> wrote:
>
> > How about String.isWhitespaceOrEmpty? I think that accurately describes
> the
> > method.
> >
> > Cheers,
> > Jonathan
> >
> > On 15 May 2018 at 19:15, Jeremy Manson  wrote:
> >
> > > Seems to me that consistency matters here: for Character to call it
> > > whitespace and for String to call it blank is a little weird.
> > >
> > > Is there a well-understood definition of "blank string", or did this
> just
> > > seem like a good name choice?
> > >
> > > Jeremy
> > >
> > > On Mon, May 14, 2018 at 11:14 PM Remi Forax  wrote:
> > >
> > > > Hi Louis,
> > > > I prefer isBlank to isWhitespace, there is a notion of all characters
> > in
> > > > isBlank that you do not have with isWhitespace.
> > > >
> > > > Rémi
> > > >
> > > > - Mail original -
> > > > > De: "Louis Wasserman" 
> > > > > À: "Xueming Shen" 
> > > > > Cc: "core-libs-dev" 
> > > > > Envoyé: Lundi 14 Mai 2018 22:15:50
> > > > > Objet: Re: RFR: JDK-8200436 - String::isBlank
> > > >
> > > > > It's not clear to me that "isBlank" is a good name for this method.
> > > > > "isWhitespace" might be more appropriate, perhaps.
> > > > >
> > > > > On Mon, May 14, 2018 at 12:48 PM Xueming Shen <
> > xueming.s...@oracle.com
> > > >
> > > > > wrote:
> > > > >
> > > > >> +1
> > > > >>
> > > > >> On 5/14/18, 8:25 AM, Jim Laskey wrote:
> > > > >> > New string instance method that returns true if the string is
> > empty
> > > or
> > > > >> contains only white space, where white space is defined as any
> > > codepoint
> > > > >> returns true when passed to Character::isWhitespace.
> > > > >> >
> > > > >> > webrev:
> http://cr.openjdk.java.net/~jlaskey/8200436/webrev/index.
> > > html
> > > > >> > jbs: https://bugs.openjdk.java.net/browse/JDK-8200436
> > > > >> > csr: https://bugs.openjdk.java.net/browse/JDK-8200437
> > > > >> >
> > > > >>
> > > >
> > >
> >
>

Re: RFR: jsr166 jdk integration 2018-05

[Martin Buchholz]

On Tue, May 15, 2018 at 5:34 PM, Stuart Marks 
wrote:

>
>>

> Bringing replaceAll() to the public API exposes it as a single operation,
> with its own semantics.




> Note that Vector.replaceAll() holds the lock for the entire operation, not
> merely around individual set() operations. It really is different from
> performing individual set() operations.


How many times the lock is acquired is an implementation detail, a
non-obvious tradeoff even.
vector.replaceAll holds the lock throughout, but vector.subList(0,
size).replaceAll holds the lock for each element (sigh ... racily (really a
bug!)).


> Again, imagine this use case: there is a periodic background task that
>> optimizes all the elements of a Vector
>> vector.replaceAll(x -> optimized(x))
>> That should not break any iterations in progress.
>>
>
> I don't think it's possible to do that correctly without holding a lock
> around the entire iteration.


I don't see why.


> If the lock is held, CME can't occur, as a concurrent replaceAll() will
> occur before or after the iteration, never in the middle.
>
>
I don't see why - iteration is inherently non-atomic, so replaceAll could
be called in the middle.


> If an iteration over a Vector doesn't hold a lock, any read-modify-write
> operations (consider a loop with a ListIterator on which set() is called)
> can be interleaved with bulk operations (like replaceAll) which is clearly
> incorrect. In such cases, CME should be thrown.
>
>
Imagine your iterators are all read-only, and don't care whether they see
an element or its replacement.


> Also, this use case cannot be written today, because CME is thrown.


??  Imagine there's only one writer thread, with some Iterating reader
threads.  Every midnight, the writer thread optimizes all the elements
  for (int i = 0; i < size; i++) vector.set(i, optimized(vector.get(i))
This code has worked well since the '90s without CME.  One day the
maintainer notices shiny new Vector.replaceAll and "fixes" the code.
"""It's perfectly safe""".
The CME is rare and so only caught when the maintainer has gone on to the
next job.  The CMEs only happen at midnight!

Re: RFR: jsr166 jdk integration 2018-05

[David Holmes]

I'm still with Martin on this. It makes no sense to me to allow 
replacing one element to not cause CME, but replacing more than one does 
cause CME. This is inconsistent and confusing and the end result is the 
programmer won't know what to expect when or why.


The original intent of CME, from my recollections back in 
lead-up-to-Java-5 days, was to prevent iterators from breaking i.e. 
throwing exceptions, due to the occurrence of the "concurrent" operation 
that changed the structure. It was not intended as an indicator of a 
semantic programming error. Replacing one element whilst there is a live 
iterator can be just as semantically wrong as replacing them all.


Cheers,
David
-

On 16/05/2018 10:34 AM, Stuart Marks wrote:

(TL;DR - replaceAll incrementing modCount is a bug.)


I acknowledge that statement is the one in dispute.


Hmmm ... my previous convincing arguments have failed to convince ?!

Your argument above applies to List.set just as much as 
List.repladeAll, because the latter is nothing more semantically than 
a bunch of calls to the former. They should have the same behavior.  
Not having the same behavior leads to inconsistency, seen today in 
subList operations on ArrayList and Vector having different modCount 
behavior than on the root list.


Right, I read those arguments, and I'm not convinced.

Just because an individual operation has some characteristic doesn't 
necessarily imply that an aggregate operation must have the same 
characteristic. (This is variously called a fallacy of composition, or 
in U.S. tax law, the step transaction doctrine.)


Bringing replaceAll() to the public API exposes it as a single 
operation, with its own semantics. Note that Vector.replaceAll() holds 
the lock for the entire operation, not merely around individual set() 
operations. It really is different from performing individual set() 
operations.


Again, imagine this use case: there is a periodic background task that 
optimizes all the elements of a Vector

vector.replaceAll(x -> optimized(x))
That should not break any iterations in progress.


I don't think it's possible to do that correctly without holding a lock 
around the entire iteration. If the lock is held, CME can't occur, as a 
concurrent replaceAll() will occur before or after the iteration, never 
in the middle.


If an iteration over a Vector doesn't hold a lock, any read-modify-write 
operations (consider a loop with a ListIterator on which set() is 
called) can be interleaved with bulk operations (like replaceAll) which 
is clearly incorrect. In such cases, CME should be thrown.


Also, this use case cannot be written today, because CME is thrown. I'm 
not aware of an actual use case that's been prevented because CME is 
thrown. Of course, as API designers we have to anticipate needs and not 
just react to complaints. My view of this kind of situation 
(interleaving of bulk operation(s) with an iteration loop) is much more 
likely to be a programming error than an actual use case.


To strengthen that, the default method List.replaceAll is specified to 
be equivalent to

 final ListIterator li = list.listIterator();
 while (li.hasNext()) {
 li.set(operator.apply(li.next()));
 }

and incrementing modCount breaks "equivalent to".


Note carefully: that is an "Implementation Requirement" on the default 
implementation of List.replaceAll(). It doesn't govern implementations 
in implementing classes such as ArrayList.


s'marks

Re: RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[David Holmes]

+1 on both comments.

In addition I'd prefer

+ u4 useed = (u4)seed;

for clarity, rather than just 's'.

Thanks,
David

On 16/05/2018 2:16 AM, Aleksey Shipilev wrote:

On 05/15/2018 06:11 PM, Severin Gehwolf wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8203223
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.01/


*) Um:
   assert(seed > 0 && "invariant");

This should be this, right?

   assert(seed > 0, "invariant");

*) I would also write:

   return (s4)s & 0x7FFF;

as

   return (s4)(s & 0x7FFF);

-Aleksey

Re: RFR: jsr166 jdk integration 2018-05

[Stuart Marks]

(TL;DR - replaceAll incrementing modCount is a bug.)


I acknowledge that statement is the one in dispute.


Hmmm ... my previous convincing arguments have failed to convince ?!

Your argument above applies to List.set just as much as List.repladeAll, because 
the latter is nothing more semantically than a bunch of calls to the former.  
They should have the same behavior.  Not having the same behavior leads to 
inconsistency, seen today in subList operations on ArrayList and Vector having 
different modCount behavior than on the root list.


Right, I read those arguments, and I'm not convinced.

Just because an individual operation has some characteristic doesn't necessarily 
imply that an aggregate operation must have the same characteristic. (This is 
variously called a fallacy of composition, or in U.S. tax law, the step 
transaction doctrine.)


Bringing replaceAll() to the public API exposes it as a single operation, with 
its own semantics. Note that Vector.replaceAll() holds the lock for the entire 
operation, not merely around individual set() operations. It really is different 
from performing individual set() operations.


Again, imagine this use case: there is a periodic background task that optimizes 
all the elements of a Vector

vector.replaceAll(x -> optimized(x))
That should not break any iterations in progress.


I don't think it's possible to do that correctly without holding a lock around 
the entire iteration. If the lock is held, CME can't occur, as a concurrent 
replaceAll() will occur before or after the iteration, never in the middle.


If an iteration over a Vector doesn't hold a lock, any read-modify-write 
operations (consider a loop with a ListIterator on which set() is called) can be 
interleaved with bulk operations (like replaceAll) which is clearly incorrect. 
In such cases, CME should be thrown.


Also, this use case cannot be written today, because CME is thrown. I'm not 
aware of an actual use case that's been prevented because CME is thrown. Of 
course, as API designers we have to anticipate needs and not just react to 
complaints. My view of this kind of situation (interleaving of bulk operation(s) 
with an iteration loop) is much more likely to be a programming error than an 
actual use case.


To strengthen that, the default method List.replaceAll is specified to be equivalent to 


 final ListIterator li = list.listIterator();
 while (li.hasNext()) {
 li.set(operator.apply(li.next()));
 }

and incrementing modCount breaks "equivalent to".


Note carefully: that is an "Implementation Requirement" on the default 
implementation of List.replaceAll(). It doesn't govern implementations in 
implementing classes such as ArrayList.


s'marks

RFR JDK-8191533,jar --describe-module prints service provider class names in lower case

[Xueming Shen]

Hi,

Please help review the change for JDK-8191533

issue: https://bugs.openjdk.java.net/browse/JDK-8191533
webrev: http://cr.openjdk.java.net/~sherman/8191533/webrev/

The jmod change has been verify by applying jmod on those modules at jdk 
home.


thanks!
sherman

Re: RFR: jsr166 jdk integration 2018-05

[Martin Buchholz]

On Tue, May 15, 2018 at 11:35 AM, Martin Buchholz 
wrote:

>
> Your argument above applies to List.set just as much as List.repladeAll,
> because the latter is nothing more semantically than a bunch of calls to
> the former.  They should have the same behavior.  Not having the same
> behavior leads to inconsistency, seen today in subList operations on
> ArrayList and Vector having different modCount behavior than on the root
> list.
>

To strengthen that, the default method List.replaceAll is *specified* to be
equivalent to

 final ListIterator li = list.listIterator();
 while (li.hasNext()) {
 li.set(operator.apply(li.next()));
 }

https://download.java.net/java/early_access/jdk11/docs/api/java.base/java/util/List.html#replaceAll(java.util.function.UnaryOperator)
and incrementing modCount breaks "equivalent to"

Re: Fwd: Question about Reflection details

[Peter Levart]

Hi Thomas,

On 05/15/18 14:20, Thomas Stüfe wrote:

Hi all,

Redirecting my question to core-libs as suggested by David.

Thanks, Thomas


-- Forwarded message --
From: Thomas Stüfe 
Date: Tue, May 15, 2018 at 12:06 PM
Subject: Question about Reflection details
To: Hotspot dev runtime 


Hi all,

I have two questions about the non-native reflection mechanism in the VM:

1) For each method invocation we generate a new child class of
internal.reflect.MagicAccessorImpl, which is loaded by its own
instance of DelegatingClassLoader.


That's not entirely true. Only the Nth invocation of a particular Method 
generates it. Unless overridden by a system property, the 1st invocation 
constructs a NativeMethodAccessorImpl and wraps it with 
DelegatingMethodAccessorImpl which delegates invocations to the former 
which uses JNI to perform the invocations and also counts them. By 
default, after 15 invocations, NativeMethodAccessorImpl replaces itself 
as the delegate of DelegatingMethodAccessorImpl with a generated 
subclass of MethodAccessorImpl which is a subclass of MagicAccessorImpl 
as you pointed out. So in reality, only the 15th invocation triggers 
generation of new class and its loading in new class loader...



The comment in jdk.internal.reflect.ClassDefiner states the reasons for this:


There are two primary reasons for creating a new loader
instead of defining these bytecodes directly into the defining
loader of the target class: first, it avoids any possible
security risk of having these bytecodes in the same loader.
Second, it allows the generated bytecodes to be unloaded earlier
than would otherwise be possible, decreasing run-time
footprint.


Do I understand this correctly:

the lifetime of the MagicAccessorImpl instance, its class and its
loading DelegatingClassLoader are defined by the lifetime of the
java.lang.reflect.Method/Constructor object which keeps the
MagicAccessorImpl instance in its methodAccessor/constructorAccessor
field?


Right, but this same MethodAccessor instance is also set on the 
corresponding "root" Method instance - this is the instance that is 
never exposed to users, but is kept in per-declaring-Class cache. When 
users ask for Method/Field/Constructor, they get a copy of the "root" 
object, because Method/Field/Constructor are unfortunately mutable 
objects (remember .setAccessible())...


So the MethodAccessor is also reachable from the "root" Method object, 
which is softly reachable from the method's declaring Class. The cache 
of Methods/Fields/Constructors in Class object is managed by a 
SoftReference. So in theory, in the presence of memory pressure, cached 
Methods/Fields/Constructors may be collected and their XxxAccessors with 
them and the generated classes too.



So, if that Method/Constructor object is collected, its
MagicAccessorImpl instance is collected, and since it is the only
instance its class too, and since it is the only class in that
DelegatingClassLoader that gets collected as well?


Right.


2) I see in my traces that for each Method.invoke(obj.foo()) we generate
   - one GeneratedMethodAccessorImplXXX class living in its own
DelegatingClassLoader instance, which invokes obj.foo


As described above, not in each invocation, but only in the 15th 
invocation and never more for the same Method (unless it gets unloaded 
in the meantime of course).



   - and then one additional GeneratedConstructorAccessorXXX, again
living in its very own DelegatingClassLoader instance, which invokes
the constructor for the just generated GeneratedMethodAccessorImplXXX.


This should not happen as that "newInstance()" invocation should be the 
1st and the only invocation of the Constructor for that 
GeneratedMethodAccessorImplXX subclass.


Unless you set a system property to disable inflation in which case the 
GeneratedMethodAccessorImplXXX is generated on 1st invocation of the 
Method and constructing its instance triggers generating 
GeneratedConstructorAccessorXXX on its 1st invocation too. But only the 
1st invocation. Following invocations of the same Method just use the 
already instantiated accessor object.



The latter I see only if I switch off inflation. The very first (and
only) time GeneratedMethodAccessorImplXXX is instantiated it will
cause GeneratedConstructorAccessorXXX to be created for that one
newInstance() call.


Righ. Your observation is correct.


But surely, in that case we could save one class loader and let the
GeneratedConstructorAccessorXXX get loaded by the
DelegatingClassLoader which also loaded the associated
GeneratedMethodAccessorImplXXX? Or is it too much bother, since we are
in an artificial scenario (noInflation=true)?


Better yet, GeneratedXXXAccessor(s) should be instantiated (constructed) 
by special constructor accessors - just like the 
GeneratedConstructorAccessorXXX(s) are - 
BootstrapConstructorAccessorImpl is used for that purpose, to prevent 
dependency loop. It would not be hard to re-purpose it to

Re: RFR(JDK11/JAXP/java.xml) 8202426: NPE thrown by Transformer when XMLStreamReader reports no xml attribute type

[Joe Wang]

Thanks Lance!  I pushed after removing line 76.

Best,
Joe

On 5/15/18, 11:01 AM, Lance Andersen wrote:

Hi Joe,

Looks fine overall

Perhaps remove the commented out line of code on line 76 
in StAXSourceTest.java before pushing


Best
Lance
On May 15, 2018, at 1:00 PM, Joe Wang > wrote:


Hi,

Please review a fix that adds better tolerance for a XMLStreamReader 
reporting no attribute type, in which case, the default CDATA type is 
used.


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



Thanks,
Joe




Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: jsr166 jdk integration 2018-05

[Martin Buchholz]

On Tue, May 15, 2018 at 10:19 AM, Stuart Marks 
wrote:

>
>
> I mentally revised the history when doing the collections/stream API work
>> since we added more bulk operations, since this is on a “best effort” 
>> basis
>> and if it’s cheap to do then there is no real harm in it and it might 
>> help.
>>
>> Spec says:
>>
>> """protected transient int modCount
>>
>> The number of times this list has been structurally modified.
>> Structural modifications are those that change the size of the list, or
>> otherwise perturb it in such a fashion that iterations in progress may
>> yield incorrect results."""
>>
>> replaceAll doesn't qualify as a structural modification.
>>
>
> Why not? It can "perturb it in such a fashion that iterations in
> progress may yield incorrect results”.
>
> Why?  It replaces every element "inplace" in the style of List.set(i)
> which is also not a structural modification.
>
 I get that, but I would argue that (placing the implementation aside)
 the bulk operation as a whole is compatible with the “otherwise” part of
 the modCount definition, since it can perturb the list and effect the yet
 to be traversed elements. Such usage is a likely source of hard to track
 down bugs that CMEs are designed to help flag.
 I doubt i am gonna change your mind on this :-) So we may just have to
 agree to disagree on the interpretation of the definition and move on. I
 would prefer it remains but it's your call.

>>>
>>> FWIW I agree with Martin - sorry Paul :)
>>>
>>
>> That’s ok. Its clear i am out numbered here, and overspent my budget on
>> debating CME for the month :-)
>>
>
> Don't give up so quickly, Paul. :-)
>
> I remember working on the bulk operations back in JDK 8 (with Mike
> Duigou?) and we discussed the issue of bulk operations vs. concurrent
> modification.
>
> Even though operations like replaceAll() and sort() don't "structurally
> modify" the list, our interpretation was that they did perturb in-progress
> iterations. These operations potentially change every element, thus an
> in-progress iteration is likely to observe some unspecified mixture of the
> "before" and "after" states of the list. That sounds like a programming
> error. The intent of CME is to prevent programming errors, so we focused on
> that instead of on "structural modification." The "perturb" clause seemed
> to cover this behavior, so we left that part of the spec unchanged.
>
> Note that in JDK 8, both ArrayList's and Vector's replaceAll() both will
> cause CME to be thrown by an in-progress iteration. I think the onus should
> be on Martin to show why replaceAll() should no longer trigger CME.
>
>
(TL;DR - replaceAll incrementing modCount is a bug.)

Hmmm ... my previous convincing arguments have failed to convince ?!

Your argument above applies to List.set just as much as List.repladeAll,
because the latter is nothing more semantically than a bunch of calls to
the former.  They should have the same behavior.  Not having the same
behavior leads to inconsistency, seen today in subList operations on
ArrayList and Vector having different modCount behavior than on the root
list.

Again, imagine this use case: there is a periodic background task that
optimizes all the elements of a Vector
vector.replaceAll(x -> optimized(x))
That should not break any iterations in progress.

(I also lean to removing the modCount increment in sort, but there the
argument is much weaker, since that likely WILL perturb any iteration in
progress)


> An alternative would be to adjust the specification of modCount to clarify
> that it accommodates bulk modifications that aren't strictly structural
> modifications.
>
> s'marks
>

Re: RFR: JDK-8200436 - String::isBlank

[Jonathan Bluett-Duncan]

How about String.isWhitespaceOrEmpty? I think that accurately describes the
method.

Cheers,
Jonathan

On 15 May 2018 at 19:15, Jeremy Manson  wrote:

> Seems to me that consistency matters here: for Character to call it
> whitespace and for String to call it blank is a little weird.
>
> Is there a well-understood definition of "blank string", or did this just
> seem like a good name choice?
>
> Jeremy
>
> On Mon, May 14, 2018 at 11:14 PM Remi Forax  wrote:
>
> > Hi Louis,
> > I prefer isBlank to isWhitespace, there is a notion of all characters in
> > isBlank that you do not have with isWhitespace.
> >
> > Rémi
> >
> > - Mail original -
> > > De: "Louis Wasserman" 
> > > À: "Xueming Shen" 
> > > Cc: "core-libs-dev" 
> > > Envoyé: Lundi 14 Mai 2018 22:15:50
> > > Objet: Re: RFR: JDK-8200436 - String::isBlank
> >
> > > It's not clear to me that "isBlank" is a good name for this method.
> > > "isWhitespace" might be more appropriate, perhaps.
> > >
> > > On Mon, May 14, 2018 at 12:48 PM Xueming Shen  >
> > > wrote:
> > >
> > >> +1
> > >>
> > >> On 5/14/18, 8:25 AM, Jim Laskey wrote:
> > >> > New string instance method that returns true if the string is empty
> or
> > >> contains only white space, where white space is defined as any
> codepoint
> > >> returns true when passed to Character::isWhitespace.
> > >> >
> > >> > webrev: http://cr.openjdk.java.net/~jlaskey/8200436/webrev/index.
> html
> > >> > jbs: https://bugs.openjdk.java.net/browse/JDK-8200436
> > >> > csr: https://bugs.openjdk.java.net/browse/JDK-8200437
> > >> >
> > >>
> >
>

Re: RFR(JDK11/JAXP/java.xml) 8202426: NPE thrown by Transformer when XMLStreamReader reports no xml attribute type

[Lance Andersen]

Hi Joe,

Looks fine overall

Perhaps remove the commented out line of code on line 76 in StAXSourceTest.java 
before pushing

Best
Lance
> On May 15, 2018, at 1:00 PM, Joe Wang  wrote:
> 
> Hi,
> 
> Please review a fix that adds better tolerance for a XMLStreamReader 
> reporting no attribute type, in which case, the default CDATA type is used.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8202426
> webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8202426/webrev/
> 
> Thanks,
> Joe

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR: jsr166 jdk integration 2018-05

[Stuart Marks]

I mentally revised the history when doing the collections/stream API work since 
we added more bulk operations, since this is on a “best effort” basis and if 
it’s cheap to do then there is no real harm in it and it might help.

Spec says:

"""protected transient int modCount

The number of times this list has been structurally modified. Structural modifications are 
those that change the size of the list, or otherwise perturb it in such a fashion that 
iterations in progress may yield incorrect results."""

replaceAll doesn't qualify as a structural modification.


Why not? It can "perturb it in such a fashion that iterations in progress may 
yield incorrect results”.

Why?  It replaces every element "inplace" in the style of List.set(i) which is 
also not a structural modification.

I get that, but I would argue that (placing the implementation aside) the bulk 
operation as a whole is compatible with the “otherwise” part of the modCount 
definition, since it can perturb the list and effect the yet to be traversed 
elements. Such usage is a likely source of hard to track down bugs that CMEs 
are designed to help flag.
I doubt i am gonna change your mind on this :-) So we may just have to agree to 
disagree on the interpretation of the definition and move on. I would prefer it 
remains but it's your call.


FWIW I agree with Martin - sorry Paul :)


That’s ok. Its clear i am out numbered here, and overspent my budget on 
debating CME for the month :-)


Don't give up so quickly, Paul. :-)

I remember working on the bulk operations back in JDK 8 (with Mike Duigou?) and 
we discussed the issue of bulk operations vs. concurrent modification.


Even though operations like replaceAll() and sort() don't "structurally modify" 
the list, our interpretation was that they did perturb in-progress iterations. 
These operations potentially change every element, thus an in-progress iteration 
is likely to observe some unspecified mixture of the "before" and "after" states 
of the list. That sounds like a programming error. The intent of CME is to 
prevent programming errors, so we focused on that instead of on "structural 
modification." The "perturb" clause seemed to cover this behavior, so we left 
that part of the spec unchanged.


Note that in JDK 8, both ArrayList's and Vector's replaceAll() both will cause 
CME to be thrown by an in-progress iteration. I think the onus should be on 
Martin to show why replaceAll() should no longer trigger CME.


An alternative would be to adjust the specification of modCount to clarify that 
it accommodates bulk modifications that aren't strictly structural modifications.


s'marks

RFR(JDK11/JAXP/java.xml) 8202426: NPE thrown by Transformer when XMLStreamReader reports no xml attribute type

[Joe Wang]

Hi,

Please review a fix that adds better tolerance for a XMLStreamReader 
reporting no attribute type, in which case, the default CDATA type is used.


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

Thanks,
Joe

Re: RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[Aleksey Shipilev]

On 05/15/2018 06:11 PM, Severin Gehwolf wrote:
> Bug: https://bugs.openjdk.java.net/browse/JDK-8203223
> webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.01/

*) Um:
  assert(seed > 0 && "invariant");

This should be this, right?

  assert(seed > 0, "invariant");

*) I would also write:

  return (s4)s & 0x7FFF;

as

  return (s4)(s & 0x7FFF);

-Aleksey

RFR: 8203223: Signed integer overflow in ImageStrings::hash_code (libjimage.so)

[Severin Gehwolf]

Hi,

Could somebody please review this patch for libjimage.so. It fixes one
instance of undefined behavour (signed integer overflow), which
prevents JDK 11 to build on Fedora 28 (GCC 8) with a rather strange
error:

./build/linux-x86_64-normal-server-fastdebug/support/interim-image/bin/java 
-version
Error occurred during initialization of VM
java/lang/NoClassDefFoundError: java/lang/Object

The proposed fix is to perform the calculations on a local variable of
unsigned type where overflow is well defined.

Bug: https://bugs.openjdk.java.net/browse/JDK-8203223
webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8203223/webrev.01/

Testing: tools/jlink tests (no new failures), currently running through
jdk-submit.

Thanks,
Severin

Re: [core-libs] RFR (L): 8010319: Implementation of JEP 181: Nest-Based Access Control

[Alan Bateman]

On 15/05/2018 01:52, David Holmes wrote:
This review is being spread across four groups: langtools, core-libs, 
hotspot and serviceability. This is the specific review thread for 
core-libs - webrev:


http://cr.openjdk.java.net/~dholmes/8010319-JEP181/webrev.corelibs.v1/ 

The API additions mostly look good, just a few comments:

- Can the spec for Class::getNestHost be explicit that it returns "this" 
when the class is a primitive or array class?


- Can Class::getNestMembers specify if it runs the class initializer of 
any classes that it loads? Less of an issue for getNestHost as the host 
is likely loaded already.


- I suspect the @throws SecurityException in getNestMembers was copied 
from getNestHost as it uses "returned class" (singular). As the host and 
members are in the same runtime package then maybe it can be specified 
in terms of the host or members package?


- isNestmateOf could be a bit clearer that it returns false in the event 
that the attributes can't be parsed. I realize it's specified in terms 
of getNestHost but I'm sure you see what I mean.


The tests for core reflection are in test/jdk/java/lang/reflect and I'm 
wondering if there has been any discussion about adding at least some 
basic tests there? I see the tests in 
test/hotspot/jtreg/runtime/Nestmates/reflectionAPI but this isn't an 
obvious location for someone touching this code.


The test in InterfaceAccessFlagsTest.java can be disabled with 
@Test(enabled=false), might be cleaner than commenting it out.


Maybe a question for Kumar but are we planning to pull in any ASM 
updates for JDK 11? NestMembers extends Attribute looks okay, I'm less 
sure about the change to ClassReader as I don't know if there is 
somewhere else in ASM that has the list of attributes to always parse.


The update to the access check in Reflection.java looks okay (just need 
to use 4-space indent, not 2).  I think it's okay in VerifyAccess too 
but the "FIX ME" suggests there are doubts so I assume this needs more 
eyes to triple check.


In passing, you might want to double check the javadoc links. I noticed 
one #getNestHost that is missing parenthesis and I'm not sure how 
javadoc handles that.


-Alan.

Fwd: Question about Reflection details

[Thomas Stüfe]

Hi all,

Redirecting my question to core-libs as suggested by David.

Thanks, Thomas


-- Forwarded message --
From: Thomas Stüfe 
Date: Tue, May 15, 2018 at 12:06 PM
Subject: Question about Reflection details
To: Hotspot dev runtime 


Hi all,

I have two questions about the non-native reflection mechanism in the VM:

1) For each method invocation we generate a new child class of
internal.reflect.MagicAccessorImpl, which is loaded by its own
instance of DelegatingClassLoader.

The comment in jdk.internal.reflect.ClassDefiner states the reasons for this:


There are two primary reasons for creating a new loader
instead of defining these bytecodes directly into the defining
loader of the target class: first, it avoids any possible
security risk of having these bytecodes in the same loader.
Second, it allows the generated bytecodes to be unloaded earlier
than would otherwise be possible, decreasing run-time
footprint.


Do I understand this correctly:

the lifetime of the MagicAccessorImpl instance, its class and its
loading DelegatingClassLoader are defined by the lifetime of the
java.lang.reflect.Method/Constructor object which keeps the
MagicAccessorImpl instance in its methodAccessor/constructorAccessor
field?

So, if that Method/Constructor object is collected, its
MagicAccessorImpl instance is collected, and since it is the only
instance its class too, and since it is the only class in that
DelegatingClassLoader that gets collected as well?

2) I see in my traces that for each Method.invoke(obj.foo()) we generate
  - one GeneratedMethodAccessorImplXXX class living in its own
DelegatingClassLoader instance, which invokes obj.foo
  - and then one additional GeneratedConstructorAccessorXXX, again
living in its very own DelegatingClassLoader instance, which invokes
the constructor for the just generated GeneratedMethodAccessorImplXXX.

The latter I see only if I switch off inflation. The very first (and
only) time GeneratedMethodAccessorImplXXX is instantiated it will
cause GeneratedConstructorAccessorXXX to be created for that one
newInstance() call.

But surely, in that case we could save one class loader and let the
GeneratedConstructorAccessorXXX get loaded by the
DelegatingClassLoader which also loaded the associated
GeneratedMethodAccessorImplXXX? Or is it too much bother, since we are
in an artificial scenario (noInflation=true)?



In general, I have the following question:

Will the 1:1 relationship between MagicAccessorImpl class and
DelegatingClassLoader instance always hold true?

Or is there work in progress somewhere which would maybe bundle
MagicAccessorImpl classes in one loader (e.g. (2) would be a candidate
for this), or maybe do it without loaders?

Thanks!

Thomas

Re: RFR: 8196340: (coll) Examine overriding inherited methods in ArrayList and ArrayList.SubList

[Claes Redestad]

On 2018-05-15 03:45, Ivan Gerasimov wrote:

let's move the line
586 final int otherModCount = other.modCount;
to the beginning of equalsArrayList(ArrayList other), so it is 
initialized before other.size is read? 


Sure, let's also pick up on Martin's suggestion to rewrite the control 
flow and minimize bytecode:


http://cr.openjdk.java.net/~redestad/8196340/open.03/

/Claes