Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Mandy Chung]

What I wanted to say is that you are creating a new CacheKey which is used to 
load a resource bundle and if found, it will be put in the cacheList.

It seems wrong to continue the request for loading a bundle but its caller 
module and target module is GC’ed and put a NONEXISTENT_BUNDLE in the cache. I 
consider it as an error state if we ever get there.

Mandy

> On Jan 13, 2017, at 4:54 PM, Naoto Sato  wrote:
> 
> Modified as:
> 
> diff -r a6d3c80ea436 src/java.base/share/classes/java/util/ResourceBundle.java
> --- a/src/java.base/share/classes/java/util/ResourceBundle.java
> +++ b/src/java.base/share/classes/java/util/ResourceBundle.java
> @@ -2192,7 +2192,7 @@
> private static void clearCacheImpl(Module callerModule, ClassLoader 
> loader) {
> cacheList.keySet().removeIf(
> key -> key.getCallerModule() == callerModule &&
> -   getLoader(key.getModule()) == loader
> +   key.getModule() != null && getLoader(key.getModule()) == 
> loader
> );
> }
> 
> This is the only occurence where CacheKey's modules are passed to 
> getLoader(Module). Entire webrev is here:
> 
> http://cr.openjdk.java.net/~naoto/8171139/webrev.05/
> 
> Naoto
> 
> On 01/13/2017 04:25 PM, Mandy Chung wrote:
>> 
>>> On Jan 13, 2017, at 4:05 PM, Naoto Sato  wrote:
 
 Daniel asks whether there is any possibility that the target module or 
 caller module would be GC’ed.
 
 CacheKey is copied in two places, one in findBundle and the other is 
 putBundleInCache which is actually passed with a newly cloned CacheKey.  
 IIUC, the cacheKey passed to findBundle ia always a new instance (as it 
 subsequently sets to different locale during the lookup).  Its caller 
 module is the caller on the stack and the target module is also a strong 
 reference either caller’s module, unnamed module from a given class 
 loader, or a given module.
 
 Naoto will have to double check.
>>> 
>>> I think this is correct. The current way of using CacheKey is safe from its 
>>> modules to be GC'ed. However in general, it'd be good prepare them to be 
>>> GC'ed. I modified the impl to hold them in local variables preventing them 
>>> to be GC'ed before instantiating new References. (Also I modified not to 
>>> call the other constructor in the copy constructor, reinstating some piece 
>>> what Peter's diff originally had). So here is the updated webrev:
>>> 
>>> http://cr.openjdk.java.net/~naoto/8171139/webrev.04/
>> 
>> So callerRef and moduleRef may be null. getLoader(null) may be called which 
>> will throw NPE.
>> 
>> Mandy
>> 
> 

Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Naoto Sato]

Modified as:

diff -r a6d3c80ea436 
src/java.base/share/classes/java/util/ResourceBundle.java

--- a/src/java.base/share/classes/java/util/ResourceBundle.java
+++ b/src/java.base/share/classes/java/util/ResourceBundle.java
@@ -2192,7 +2192,7 @@
 private static void clearCacheImpl(Module callerModule, 
ClassLoader loader) {

 cacheList.keySet().removeIf(
 key -> key.getCallerModule() == callerModule &&
-   getLoader(key.getModule()) == loader
+   key.getModule() != null && 
getLoader(key.getModule()) == loader

 );
 }

This is the only occurence where CacheKey's modules are passed to 
getLoader(Module). Entire webrev is here:


http://cr.openjdk.java.net/~naoto/8171139/webrev.05/

Naoto

On 01/13/2017 04:25 PM, Mandy Chung wrote:



On Jan 13, 2017, at 4:05 PM, Naoto Sato  wrote:


Daniel asks whether there is any possibility that the target module or caller 
module would be GC’ed.

CacheKey is copied in two places, one in findBundle and the other is 
putBundleInCache which is actually passed with a newly cloned CacheKey.  IIUC, 
the cacheKey passed to findBundle ia always a new instance (as it subsequently 
sets to different locale during the lookup).  Its caller module is the caller 
on the stack and the target module is also a strong reference either caller’s 
module, unnamed module from a given class loader, or a given module.

Naoto will have to double check.


I think this is correct. The current way of using CacheKey is safe from its 
modules to be GC'ed. However in general, it'd be good prepare them to be GC'ed. 
I modified the impl to hold them in local variables preventing them to be GC'ed 
before instantiating new References. (Also I modified not to call the other 
constructor in the copy constructor, reinstating some piece what Peter's diff 
originally had). So here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8171139/webrev.04/


So callerRef and moduleRef may be null. getLoader(null) may be called which 
will throw NPE.

Mandy

Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Mandy Chung]

> On Jan 13, 2017, at 4:05 PM, Naoto Sato  wrote:
>> 
>> Daniel asks whether there is any possibility that the target module or 
>> caller module would be GC’ed.
>> 
>> CacheKey is copied in two places, one in findBundle and the other is 
>> putBundleInCache which is actually passed with a newly cloned CacheKey.  
>> IIUC, the cacheKey passed to findBundle ia always a new instance (as it 
>> subsequently sets to different locale during the lookup).  Its caller module 
>> is the caller on the stack and the target module is also a strong reference 
>> either caller’s module, unnamed module from a given class loader, or a given 
>> module.
>> 
>> Naoto will have to double check.
> 
> I think this is correct. The current way of using CacheKey is safe from its 
> modules to be GC'ed. However in general, it'd be good prepare them to be 
> GC'ed. I modified the impl to hold them in local variables preventing them to 
> be GC'ed before instantiating new References. (Also I modified not to call 
> the other constructor in the copy constructor, reinstating some piece what 
> Peter's diff originally had). So here is the updated webrev:
> 
> http://cr.openjdk.java.net/~naoto/8171139/webrev.04/

So callerRef and moduleRef may be null. getLoader(null) may be called which 
will throw NPE.

Mandy

Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Naoto Sato]

Thank you again, Daniel & Mandy.

On 1/13/17 1:25 PM, Mandy Chung wrote:



On Jan 12, 2017, at 1:20 PM, Naoto Sato  wrote:

Realized moduleRef & callerRef would never be null with this new version. Thus 
modified the patch as:

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


CacheKey
   Nit: line 576 can be moved after 581 to group the volatile fields together


As in the comment line 574, the first 4 fields are the actual keys for 
lookup, so I'd keep them that way.




 619 this(src.getName(), src.getLocale(), src.getModule(), 
src.getCallerModule());

Daniel asks whether there is any possibility that the target module or caller 
module would be GC’ed.

CacheKey is copied in two places, one in findBundle and the other is 
putBundleInCache which is actually passed with a newly cloned CacheKey.  IIUC, 
the cacheKey passed to findBundle ia always a new instance (as it subsequently 
sets to different locale during the lookup).  Its caller module is the caller 
on the stack and the target module is also a strong reference either caller’s 
module, unnamed module from a given class loader, or a given module.

Naoto will have to double check.


I think this is correct. The current way of using CacheKey is safe from 
its modules to be GC'ed. However in general, it'd be good prepare them 
to be GC'ed. I modified the impl to hold them in local variables 
preventing them to be GC'ed before instantiating new References. (Also I 
modified not to call the other constructor in the copy constructor, 
reinstating some piece what Peter's diff originally had). So here is the 
updated webrev:


http://cr.openjdk.java.net/~naoto/8171139/webrev.04/

Naoto

Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Mandy Chung]

> On Jan 12, 2017, at 1:20 PM, Naoto Sato  wrote:
> 
> Realized moduleRef & callerRef would never be null with this new version. 
> Thus modified the patch as:
> 
> http://cr.openjdk.java.net/~naoto/8171139/webrev.03/

CacheKey
   Nit: line 576 can be moved after 581 to group the volatile fields together

 619 this(src.getName(), src.getLocale(), src.getModule(), 
src.getCallerModule());

Daniel asks whether there is any possibility that the target module or caller 
module would be GC’ed.

CacheKey is copied in two places, one in findBundle and the other is 
putBundleInCache which is actually passed with a newly cloned CacheKey.  IIUC, 
the cacheKey passed to findBundle ia always a new instance (as it subsequently 
sets to different locale during the lookup).  Its caller module is the caller 
on the stack and the target module is also a strong reference either caller’s 
module, unnamed module from a given class loader, or a given module.

Naoto will have to double check.

Otherwise look fine.
Mandy

Re: Guaranteed order of annotations?

[Gunnar Morling]

Hi Yuri,

Thanks for the pointer.

But do you see any clear description of an order mandated for
getDeclaredAnnotations()? The only references to an order I can find
there are for repeatable annotations (where source code order is
maintained when retrieving them via the container annotation type).
But I cannot find any guaranteed order when obtaining all declared
annotations from an element.

The reason why I'm asking is Bean Validation, where we've seen
requests of people that wish to validate the constraints of an element
in a fixed order, aborting after the first failed constraint:

@NotNull
@Email
String email;

Here one may want to first validate @NotNull and don't proceed with
validating @Email if the field is null.

Relying on source order would be a very natural way to express the
order of constraints. Without a guaranteed ordering of annotations
we'd have to add some other means of ordering, e.g. an attribute with
the index (@NotNull(order=0) @Email(order=1)) which is more verbose of
course.

--Gunnar





2017-01-13 21:15 GMT+01:00 Yuri Gaevsky :
> Hi Gunnar,
>
> Please take a look at JDK-8010679 'Clarify "present" and annotation ordering 
> in Core Reflection for Annotations' [*].
>
> Best regards,
> -Yuri
>
> [*] https://bugs.openjdk.java.net/browse/JDK-8010679
>
>
> -Original Message-
> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf 
> Of Gunnar Morling
> Sent: Friday, January 13, 2017 08:14 PM
> To: core-libs-dev@openjdk.java.net
> Subject: Guaranteed order of annotations?
>
> Hi,
>
> Is there any order guaranteed in which an element's annotations are
> returned by AnnotatedElement#getDeclaredAnnotations()? Specifically,
> is this the order in which the annotations are given in the source
> code?
>
> Section 9.7.5. of the JLS ("Multiple Annotations of the Same Type")
> makes a statement of the source order being considered by the array of
> the implicit container annotation created for a repeatable annotation
> ("[...] all the base annotations in the left-to-right order in which
> they appeared in the context").
>
> But I couldn't find any authoritative description on the ordering
> behaviour of getDeclaredAnnotations(). Should we assume that we cannot
> rely on the order in the source?
>
> Thanks for any pointers,
>
> --Gunnar

Re: RFR of JDK-7146543: TEST_BUG: java/rmi/registry/readTest/readTest.sh failing intermittently with port in use

[Roger Riggs]

Hi Hamlin,

Looks good, thanks for converting that shell test to java.

Roger




On 1/13/2017 2:32 AM, Hamlin Li wrote:

Would you please review the below patch?

bug: https://bugs.openjdk.java.net/browse/JDK-7146543
webrev: http://cr.openjdk.java.net/~mli/7146543/webrev.00/

The patch rewrite the test in java, fixes the bug.

Thank you
-Hamlin

RE: Guaranteed order of annotations?

[Yuri Gaevsky]

Hi Gunnar,

Please take a look at JDK-8010679 'Clarify "present" and annotation ordering in 
Core Reflection for Annotations' [*].

Best regards,
-Yuri

[*] https://bugs.openjdk.java.net/browse/JDK-8010679


-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf 
Of Gunnar Morling
Sent: Friday, January 13, 2017 08:14 PM
To: core-libs-dev@openjdk.java.net
Subject: Guaranteed order of annotations?

Hi,

Is there any order guaranteed in which an element's annotations are
returned by AnnotatedElement#getDeclaredAnnotations()? Specifically,
is this the order in which the annotations are given in the source
code?

Section 9.7.5. of the JLS ("Multiple Annotations of the Same Type")
makes a statement of the source order being considered by the array of
the implicit container annotation created for a repeatable annotation
("[...] all the base annotations in the left-to-right order in which
they appeared in the context").

But I couldn't find any authoritative description on the ordering
behaviour of getDeclaredAnnotations(). Should we assume that we cannot
rely on the order in the source?

Thanks for any pointers,

--Gunnar

Re: RFR 8172765, closed/javax/sound/sampled/Clip/AppletAudioClip/SoundBug.java failed in headless system

[Felix Yang]

oops! Incidentally sent to wrong alias. Sorry, please ignore it.

Thanks,
Felix
> On 13 Jan 2017, at 9:57 AM, Felix Yang  wrote:
> 
> Hi team,
>please review the patch to mark following test as headful. It has been 
> failing in Mach 5 for several runs
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8172765
> 
> Thanks,
> Felix
> 
> Patch:
> 
> diff -r 2ca43b220611 javax/sound/sampled/Clip/AppletAudioClip/SoundBug.java
> --- a/javax/sound/sampled/Clip/AppletAudioClip/SoundBug.java  Fri Jan 13 
> 01:36:12 2017 +
> +++ b/javax/sound/sampled/Clip/AppletAudioClip/SoundBug.java  Fri Jan 13 
> 09:34:46 2017 -0800
> @@ -11,7 +11,7 @@
>  * @test
>  * @bug 4181910
>  * @summary AudioClip incompatibility
> - * @key closed-binary
> + * @key closed-binary headful
>  */
> public class SoundBug extends Frame {
> 
> 

RFR 8172765, closed/javax/sound/sampled/Clip/AppletAudioClip/SoundBug.java failed in headless system

[Felix Yang]

Hi team,
please review the patch to mark following test as headful. It has been 
failing in Mach 5 for several runs

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

Thanks,
Felix

Patch:

diff -r 2ca43b220611 javax/sound/sampled/Clip/AppletAudioClip/SoundBug.java
--- a/javax/sound/sampled/Clip/AppletAudioClip/SoundBug.javaFri Jan 13 
01:36:12 2017 +
+++ b/javax/sound/sampled/Clip/AppletAudioClip/SoundBug.javaFri Jan 13 
09:34:46 2017 -0800
@@ -11,7 +11,7 @@
  * @test
  * @bug 4181910
  * @summary AudioClip incompatibility
- * @key closed-binary
+ * @key closed-binary headful
  */
 public class SoundBug extends Frame {

Re: Guaranteed order of annotations?

[Joel Borggrén-Franck]

Iirc there are no guatantees but we try as best as we can to maintain
source code order. It has been a couple of years now but I think there are
a few corner cases with repeating annotations.

Cheers
/Joel

On Fri, 13 Jan 2017 at 18:14, Gunnar Morling  wrote:

> Hi,
>
>
>
> Is there any order guaranteed in which an element's annotations are
>
> returned by AnnotatedElement#getDeclaredAnnotations()? Specifically,
>
> is this the order in which the annotations are given in the source
>
> code?
>
>
>
> Section 9.7.5. of the JLS ("Multiple Annotations of the Same Type")
>
> makes a statement of the source order being considered by the array of
>
> the implicit container annotation created for a repeatable annotation
>
> ("[...] all the base annotations in the left-to-right order in which
>
> they appeared in the context").
>
>
>
> But I couldn't find any authoritative description on the ordering
>
> behaviour of getDeclaredAnnotations(). Should we assume that we cannot
>
> rely on the order in the source?
>
>
>
> Thanks for any pointers,
>
>
>
> --Gunnar
>
>

Re: [9] RFR: 8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed

[Daniel Fuchs]

On 12/01/17 21:20, Naoto Sato wrote:

Realized moduleRef & callerRef would never be null with this new
version. Thus modified the patch as:

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


Hi Naoto,

619 this(src.getName(), src.getLocale(), src.getModule(), 
src.getCallerModule());


this is going to throw NPE if either module is null.

I wonder if there is a risk that the modules might get
garbage collected between the time at which 'src' was obtained
and the time at which src.get[Caller]Module() is called.
If there's such a risk then maybe Reference::reachabilityFence
could be used at strategic places to prevent this?

Also the copy constructor will recompute the hash - (through the
this(...) constructor) and I wonder whether that's intended.

best regards,

-- daniel



Naoto

On 1/12/17 11:16 AM, Naoto Sato wrote:

Thank you, Mandy and Daniel for your comments. I incorporated your
suggestions and updated the fix as follows:

http://cr.openjdk.java.net/~naoto/8171139/webrev.02/
http://cr.openjdk.java.net/~naoto/8171140/webrev.00/

Please review.

Naoto


On 1/11/17 5:45 PM, Naoto Sato wrote:

Decided to include the fix to 8171140 [1] as well, as they are closely
related. Here is the updated webrev.

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

Additional changes to the version 00 are to mainly remove
clearCache(Module) method, as clearCache() is chiefly used in
combination with ResourceBundle.Control, which is not supported in named
modules. Still named modules can issue clearCache() with no argument
which will result in the same effect. Also, other clearCache() overloads
have clearer method descriptions.

CCC for 8171140 will be filed accordingly.

Naoto
--
[1]: https://bugs.openjdk.java.net/browse/JDK-8171140

On 1/10/17 4:10 PM, Naoto Sato wrote:

Hello,

Please review the changes to the subject issue:

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

The changes are located at:

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

The changeset is the same as the one in the mail thread [1]
contributed by Peter Levart, plus additional clearCache() test cases.

Naoto

[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-January/045790.html

Guaranteed order of annotations?

[Gunnar Morling]

Hi,

Is there any order guaranteed in which an element's annotations are
returned by AnnotatedElement#getDeclaredAnnotations()? Specifically,
is this the order in which the annotations are given in the source
code?

Section 9.7.5. of the JLS ("Multiple Annotations of the Same Type")
makes a statement of the source order being considered by the array of
the implicit container annotation created for a repeatable annotation
("[...] all the base annotations in the left-to-right order in which
they appeared in the context").

But I couldn't find any authoritative description on the ordering
behaviour of getDeclaredAnnotations(). Should we assume that we cannot
rely on the order in the source?

Thanks for any pointers,

--Gunnar

RFR: 8037325: Class.getConstructor() performance regression

[Christoph Dreis]

Hey,

your extraction of methodName() made me aware of another small thing. What 
about rewriting argumentTypesToString() to avoid creating the StringJoiner if 
not needed?

private static String argumentTypesToString(Class[] argTypes) {
if (argTypes != null) {
StringJoiner sj = new StringJoiner(", ", "(", ")");
for (int i = 0; i < argTypes.length; i++) {
Class c = argTypes[i];
sj.add((c == null) ? "null" : c.getName());
}
return sj.toString();
}
return "()";
}

Should have a very minor impact though. Apart from that looks good to me.

Cheers,
Christoph

-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf 
Of Claes Redestad
Sent: Thursday, January 12, 2017 3:48 PM
To: core-libs-dev Libs ; Security Dev OpenJDK 

Subject: RFR: 8037325: Class.getConstructor() performance regression

Hi,

please review this fix to various performance regressions observed
as the security model has evolved over the years.

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

- For cases where a SecurityManager is not installed, this improves
performance by avoiding calls to Reflection.getCallerClass, which
can be very expensive when not inlined. Regrettably this adds some
boilerplate.

- For cases where a SecurityManager is installed, this improves
performance slightly by avoiding repeated calls to 
System.getSecurityManager (which does volatile read).

- Use of Class.getPackageName when appropriate reduce the
number of allocations done.

- Places where doPrivileged calls were used to bypass access checking 
when calling methods on Class can safely be replaced by calling
corresponding private methods directly if care is taken to copy the
end result as appropriate.

- Finally, by using the recently used 
ReflectionFactory.getExecutableSharedParameterTypes we also get rid of
some unnecessary copying.

Thanks!

/Claes