[14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal

[Vladimir Kozlov]

http://cr.openjdk.java.net/~kvn/8185139/webrev.00/

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

For Graal to work we have to give Graal module all permissions which is 
specified in default policy [1].
Unfortunately this cause problem for Graal running tests which overwrite 
default policy.

I discussed this with Mandy and she suggested that such tests should also check default policy. I implemented her 
suggestion. Note, this is not only Graal problem. There were already similar fixes before [2].


I also updated Graal's problem list. Several tests were left on problem list (with different bug id) because they would 
not run with Java Graal (for example, they use --limit-modules flag which prevents loading Graal module). We will enable 
such tests again when libgraal is supported.


I ran testing which execute these tests with Graal. It shows only known 
problems which are not related to these changes.

Thanks,
Vladimir

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/49ed5e31fe1e/src/java.base/share/lib/security/default.policy#l156
[2] https://bugs.openjdk.java.net/browse/JDK-8189291

Re: RFR: JDK-8225428: CLI change to remove "mode", rename to "package", and build only one target

[Andy Herrick]

On 6/19/2019 6:26 PM, Alexander Matveev wrote:

Hi Andy,

1) Looks like you forgot to update HelpResources_ja.properties and 
HelpResources_zh_CN.properties.

Yes I need to re-run my script to update these.


2) 
http://cr.openjdk.java.net/~herrick/8225428/webrev.01/src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/MainResources.properties.frames.html

Line 84: Error: creating app-image requires
Should we change it to "Error: Creating application image requires", 
since we do not have app-image option anymore?

yes - will fix .
/Andy


Otherwise looks fine.

Thanks,
Alexander


On 6/19/2019 10:34 AM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


JDK-8225428: CLI change to remove "mode", rename to "package", and 
build only one target


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

[2] http://cr.openjdk.java.net/~herrick/8225428/webrev.01/ 



/Andy

Re: RFR: JDK-8225428: CLI change to remove "mode", rename to "package", and build only one target

[Alexander Matveev]

Hi Andy,

1) Looks like you forgot to update HelpResources_ja.properties and 
HelpResources_zh_CN.properties.


2) 
http://cr.openjdk.java.net/~herrick/8225428/webrev.01/src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/MainResources.properties.frames.html

Line 84: Error: creating app-image requires
Should we change it to "Error: Creating application image requires", 
since we do not have app-image option anymore?


Otherwise looks fine.

Thanks,
Alexander


On 6/19/2019 10:34 AM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


JDK-8225428: CLI change to remove "mode", rename to "package", and 
build only one target


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

[2] http://cr.openjdk.java.net/~herrick/8225428/webrev.01/ 



/Andy

Re: RFR: JDK-8225428: CLI change to remove "mode", rename to "package", and build only one target

[Andy Herrick]

On 6/19/2019 2:08 PM, Alexey Semenyuk wrote:
http://cr.openjdk.java.net/~herrick/8225428/webrev.01/test/jdk/tools/jpackage/JPackageMissingArgumentsTest.java.sdiff.html: 


---
private static final String [] RESULT_7 = {"--module-path", 
"--runtime-image", "app-image"};

---
Should it be "--app-image"?
yes - there is no need for a change here, either will work (since 
checking code uses String.contains()) but will revert in the next revision.

/Andy


- Alexey

On 6/19/2019 1:34 PM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


JDK-8225428: CLI change to remove "mode", rename to "package", and 
build only one target


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

[2] http://cr.openjdk.java.net/~herrick/8225428/webrev.01/ 



/Andy

Re: RFR: JDK-8225428: CLI change to remove "mode", rename to "package", and build only one target

[Alexey Semenyuk]

http://cr.openjdk.java.net/~herrick/8225428/webrev.01/test/jdk/tools/jpackage/JPackageMissingArgumentsTest.java.sdiff.html:
---
private static final String [] RESULT_7 = {"--module-path", 
"--runtime-image", "app-image"};

---
Should it be "--app-image"?

- Alexey

On 6/19/2019 1:34 PM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


JDK-8225428: CLI change to remove "mode", rename to "package", and 
build only one target


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

[2] http://cr.openjdk.java.net/~herrick/8225428/webrev.01/ 



/Andy

RFR: JDK-8225428: CLI change to remove "mode", rename to "package", and build only one target

[Andy Herrick]

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


JDK-8225428: CLI change to remove "mode", rename to "package", and build 
only one target


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

[2] http://cr.openjdk.java.net/~herrick/8225428/webrev.01/ 



/Andy

Re: RFR: 8213561: ZipFile/MultiThreadedReadTest.java timed out in tier1

[Lance Andersen]

Hi Sean,

+1
> On Jun 19, 2019, at 12:29 PM, Seán Coffey  wrote:
> 
> Reports that this test is failing intermittently over past few months. It's a 
> rare occurrence but I'd like to take steps to correct it.
> 
> I've removed the dependence on randomness in the bug.
> I've fixed up the zip file creation logic to produce a real zip file
> I've renamed the file to a unique file name per test run (and recorded how 
> long it takes to delete it)
> Test now logs any exception encountered during zip entry read
> 
> I've verified that the test still fails and passes as expected.
> 
> https://bugs.openjdk.java.net/browse/JDK-8213561
> http://cr.openjdk.java.net/~coffeys/webrev.8213561/webrev/
> 
> -- 
> Regards,
> Sean.
> 

 
  

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

RFR: 8213561: ZipFile/MultiThreadedReadTest.java timed out in tier1

[Seán Coffey]

Reports that this test is failing intermittently over past few months. 
It's a rare occurrence but I'd like to take steps to correct it.


I've removed the dependence on randomness in the bug.
I've fixed up the zip file creation logic to produce a real zip file
I've renamed the file to a unique file name per test run (and recorded 
how long it takes to delete it)

Test now logs any exception encountered during zip entry read

I've verified that the test still fails and passes as expected.

https://bugs.openjdk.java.net/browse/JDK-8213561
http://cr.openjdk.java.net/~coffeys/webrev.8213561/webrev/

--
Regards,
Sean.

Re: RFR(XS): 8226286 Remove unused method java.lang.Integer::formatUnsignedInt

[Brian Burkhalter]

+1

Thanks,

Brian

> On Jun 19, 2019, at 4:49 AM, Claes Redestad  wrote:
> 
> On 2019-06-19 07:28, Tagir Valeev wrote:
>> Hello, Brian!
>> Thank you for review. Here's updated version:
>> http://cr.openjdk.java.net/~tvaleev/webrev/8226286/r2/ 
>> 
> 
> this looks like a good cleanup!

Re: [PATCH] Use StringJoiner where appropriate in java.base

[Andrew Haley]

Hi,

On 6/19/19 2:12 PM, Сергей Цыпанов wrote:

> in JDK code base we have many places (mainly in j.u.Arrays) where we
> convert array to String using verbose constructions with
> StringBuilder.
> 
> As far as we have got StringJoiner for a long time we can use it
> making the code more simple.
> 
> Also toString() of AtomicIntegerArray, AtomicLongArray and
> AtomicReferenceArray partially duplicate logic of corresponding
> method available in j.u.Arrays and can be replaced with call to
> delegate.

I agree this makes the code apparently simpler, but it's not clear
that it's any faster, and there is always a cost in any large code
base associated with changes. This "churn", as it is called, has a
long-term cost for maintainers. Also, there is some risk in any part
of the core library that might be used when bootstrapping the system.

-- 
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

[PATCH] Use StringJoiner where appropriate in java.base

[Сергей Цыпанов]

Hello,

in JDK code base we have many places (mainly in j.u.Arrays) where we convert 
array to String using verbose constructions with StringBuilder.

As far as we have got StringJoiner for a long time we can use it making the 
code more simple.

Also toString() of AtomicIntegerArray, AtomicLongArray and AtomicReferenceArray 
partially duplicate logic of corresponding method available in j.u.Arrays and 
can be replaced with call to delegate.

With kind regards,
Sergey Tsypanovdiff --git a/src/java.base/share/classes/java/util/Arrays.java b/src/java.base/share/classes/java/util/Arrays.java
--- a/src/java.base/share/classes/java/util/Arrays.java
+++ b/src/java.base/share/classes/java/util/Arrays.java
@@ -4879,18 +4879,14 @@
 public static String toString(long[] a) {
 if (a == null)
 return "null";
-int iMax = a.length - 1;
-if (iMax == -1)
+if (a.length == 0)
 return "[]";
 
-StringBuilder b = new StringBuilder();
-b.append('[');
-for (int i = 0; ; i++) {
-b.append(a[i]);
-if (i == iMax)
-return b.append(']').toString();
-b.append(", ");
+StringJoiner joiner = new StringJoiner(", ", "[", "]");
+for (long l : a) {
+joiner.add(Long.toString(l));
 }
+return joiner.toString();
 }
 
 /**
@@ -4909,18 +4905,14 @@
 public static String toString(int[] a) {
 if (a == null)
 return "null";
-int iMax = a.length - 1;
-if (iMax == -1)
+if (a.length == 0)
 return "[]";
 
-StringBuilder b = new StringBuilder();
-b.append('[');
-for (int i = 0; ; i++) {
-b.append(a[i]);
-if (i == iMax)
-return b.append(']').toString();
-b.append(", ");
+StringJoiner joiner = new StringJoiner(", ", "[", "]");
+for (int i : a) {
+joiner.add(Integer.toString(i));
 }
+return joiner.toString();
 }
 
 /**
@@ -4939,18 +4931,14 @@
 public static String toString(short[] a) {
 if (a == null)
 return "null";
-int iMax = a.length - 1;
-if (iMax == -1)
+if (a.length == 0)
 return "[]";
 
-StringBuilder b = new StringBuilder();
-b.append('[');
-for (int i = 0; ; i++) {
-b.append(a[i]);
-if (i == iMax)
-return b.append(']').toString();
-b.append(", ");
+StringJoiner joiner = new StringJoiner(", ", "[", "]");
+for (short s : a) {
+joiner.add(Short.toString(s));
 }
+return joiner.toString();
 }
 
 /**
@@ -4969,18 +4957,14 @@
 public static String toString(char[] a) {
 if (a == null)
 return "null";
-int iMax = a.length - 1;
-if (iMax == -1)
+if (a.length == 0)
 return "[]";
 
-StringBuilder b = new StringBuilder();
-b.append('[');
-for (int i = 0; ; i++) {
-b.append(a[i]);
-if (i == iMax)
-return b.append(']').toString();
-b.append(", ");
+StringJoiner joiner = new StringJoiner(", ", "[", "]");
+for (char c : a) {
+joiner.add(String.valueOf(c));
 }
+return joiner.toString();
 }
 
 /**
@@ -4999,18 +4983,14 @@
 public static String toString(byte[] a) {
 if (a == null)
 return "null";
-int iMax = a.length - 1;
-if (iMax == -1)
+if (a.length == 0)
 return "[]";
 
-StringBuilder b = new StringBuilder();
-b.append('[');
-for (int i = 0; ; i++) {
-b.append(a[i]);
-if (i == iMax)
-return b.append(']').toString();
-b.append(", ");
+StringJoiner joiner = new StringJoiner(", ", "[", "]");
+for (byte b : a) {
+joiner.add(Byte.toString(b));
 }
+return joiner.toString();
 }
 
 /**
@@ -5029,18 +5009,14 @@
 public static String toString(boolean[] a) {
 if (a == null)
 return "null";
-int iMax = a.length - 1;
-if (iMax == -1)
+if (a.length == 0)
 return "[]";
 
-StringBuilder b = new StringBuilder();
-b.append('[');
-for (int i = 0; ; i++) {
-b.append(a[i]);
-if (i == iMax)
-return b.append(']').toString();
-b.append(", ");
+StringJoiner joiner = new StringJoiner(", ", "[", "]");
+for (boolean b : a) {
+joiner.add(Boolean.toString(b));
 }
+return joiner.toString();
 }
 
 /**
@@ -5059,19 +5035,14 @@
 public static String toString(float[] a) {
 if (a == null)
 return "null";
-
-int iMax = a.length - 1;
-if (iMax == -1)
+if 

Re: RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes

[Alan Bateman]

On 19/06/2019 11:07, Andrew Dinn wrote:

:
Do I still need to wait for confirmation for the CSR from Joe Darcy
before pushing to the jdk13 repo? (He already knows about the CSR).


Yes, anything that has a CSR needs to wait until it is approved.

-Alan

Re: RFR(XS): 8226286 Remove unused method java.lang.Integer::formatUnsignedInt

[Claes Redestad]

Hi Tagir,

On 2019-06-19 07:28, Tagir Valeev wrote:

Hello, Brian!

Thank you for review. Here's updated version:
http://cr.openjdk.java.net/~tvaleev/webrev/8226286/r2/


this looks like a good cleanup!

It's partly my fault that these format methods ended up a bit messy. I
exposed some of the internal to optimize UUID and, IIRC, a few other
internal APIs, then compact strings made all but the UUID optimization
redundant, which was moved into Long.java (see JDK-8148936) to narrow
down the API surface. All in all it's good to see this cleaned up and
improved further.

Thanks!

/Claes

RE: Suggestion for a more sensible implementation of EMPTY_MAP

[Anthony Vanelverdinghe]

Hi Abraham



Wouldn’t it have been possible to use Map::isEmpty? Ideally just returning at 
the very beginning of your method if `data` is an empty Map. That way you’d 
still be able to use computeIfPresent, and use emptyMap() as well.



Kind regards,

Anthony




From: core-libs-dev  on behalf of 
Abraham Marín Pérez 
Sent: Wednesday, June 19, 2019 9:51:46 AM
To: Roger Riggs
Cc: core-libs-dev@openjdk.java.net
Subject: Re: Suggestion for a more sensible implementation of EMPTY_MAP

Hi Stuart, Roger,

First of all, thank you for such a detailed response, this really shows the big 
picture. I guess no implementation will be perfect, there will always be some 
wrinkles that we need to accept, and the most suitable implementation will be 
the one with the fewest or least impacting wrinkles. Given this, maybe showing 
the particular wrinkle that I faced can shed light on the overall development 
experience that the current implementation provides.

For multiple reasons that I cannot explain at this point, I have a method that 
looks like the following:

private void decorate(Map data) {
//...
data.computeIfPresent("field", (k, v) -> highlightDifferences(v, 
otherValue));
//...
}

At one point an emptyMap() was passed to this method, causing an UOE. This left 
me with two choices:

1. Change code to:

if(data.hasKey(“field"))
data.compute("field", (k, v) -> highlightDifferences(v, otherValue));

2. Avoid usage of emptyMap() and use new HashMap<>() instead


The first option defeats the purpose of having a computeIfPresent method, and 
causes confusion among team members (people keep asking why computeIfPresent 
isn't used). The second option is pretty much what Roger mentioned (and what we 
ended up doing).

On the other hand, there are some points that you mentioned that I believe 
merit some extra debate, please find some comments below inline.

In general, I tend to agree that a stricter implementation is better since it 
usually prevents bugs. However, at least in this particular case, I believe a 
stricter implementation also has the side effect of worsening the development 
experience and adding confusion (see below comments). Of course, as I mentioned 
before, I understand that no implementation will be perfect, and maybe the 
current option is the lesser evil, but I wanted to throw in an alternative for 
consideration.

Many thanks,
Abraham


El vie., 14 jun. 2019 a las 14:18, Roger Riggs (mailto:roger.ri...@oracle.com>>) escribió:
Hi Stuart,

Not withstanding all the details. It would be useful (and possibly
expected) that an EMPTY_MAP
could be substituted for a Map with no entries.  As it stands, the
caller needs know whether any optional
possibly modifying operations will be used and decide whether to create
an empty map or an EMPTY_MAP.
That makes using an EMPTY_MAP a risk and less useful and a cautious
programmer will avoid it.

$.02, Roger


On 6/13/19 8:42 PM, Stuart Marks wrote:
>
>
> On 6/10/19 7:34 AM, Abraham Marín Pérez wrote:
>> I agree that it makes sense for EMPTY_MAP to have the same logic as
>> Map.of() or unmodifiable(new HashMap()), which means that my
>> suggestion cannot be applied to just EMPTY_MAP... but maybe that’s
>> ok: maybe we can change all of them? If we keep the default
>> implementation for Map.computeIfPresent in EMPTY_MAP, Map.of(),
>> unmodifiableMap, etc., instead of overriding it to throw an
>> exception, then:
>>
>> - For cases where the key isn’t present (regardless of empty or
>> non-empt map), the call will be a safe no-op.
>> - For cases where the key is present, the call will still throw UOE
>> via the implementation of put()
>>
>> I believe this would still respect the spec for Map.computeIfPresent
>> while providing a more forgiving implementation (in the sense that it
>> will avoid throwing an exception when possible).
>
> Hi Abraham,
>
> The specification does not mandate one behavior or another. Either
> behavior is permitted, and in fact both behaviors are present in the JDK.
>
> The second and more significant point is raised by your last statement
> suggesting a "more forgiving implementation." The question is, do we
> actually want a more forgiving implementation?
>
> The collections specs define certain methods as "optional", but it's
> not clear whether this means that calling such a method should always
> throw an exception ("strict" behavior) or whether the method should
> throw an exception only when it attempts to do something that's
> disallowed ("lenient" behavior).
>
> Take for example the putAll() method. What happens if we do this?
>
> Map map1 = Collections.emptyMap();
> map1.putAll(otherMap);
>
> The answer is that it depends on the state of otherMap. If otherMap is
> empty, then the operation succeeds (and does nothing). However, if
> otherMap is non-empty, then the operation will throw
> UnsupportedOperationException. That's an example of lenient behavior.
> What's notable about 

Re: RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes

[Andrew Dinn]

On 19/06/2019 11:03, Alan Bateman wrote:
> I added myself as Reviewer to the the CSR so you can finalize. The
> webrev looks good.
Thanks, Alan. I have finalized the CSR.

Do I still need to wait for confirmation for the CSR from Joe Darcy
before pushing to the jdk13 repo? (He already knows about the CSR).

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes

[Alan Bateman]

On 19/06/2019 10:21, Andrew Dinn wrote:

:
I raised this CSR:

   CSR:https://bugs.openjdk.java.net/browse/JDK-8226385

and tagged it for jdk13.

Also, I labelled it SE -- but is it, perhaps, meant to be JDK?
(apologies, I am still a noob to this process).

Yes, it's "SE" as it's normative text in Java SE API spec.

I added myself as Reviewer to the the CSR so you can finalize. The 
webrev looks good.


-Alan

Re: RFR (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes

[Andrew Dinn]

Hi Alan,

On 18/06/2019 18:08, Alan Bateman wrote:
> This looks good. Will you create a CSR for this? I think it can be fixed
> in jdk/jdk13 as it follows JDK-8221397 and JDK-8221696 (and there is no
> risk as it's javadoc only).
I raised this CSR:

  CSR:https://bugs.openjdk.java.net/browse/JDK-8226385

and tagged it for jdk13.

Also, I labelled it SE -- but is it, perhaps, meant to be JDK?
(apologies, I am still a noob to this process).

  Bug:https://bugs.openjdk.java.net/browse/JDK-8226203
  Webrev: http://cr.openjdk.java.net/~adinn/8226203/webrev.00/

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: Suggestion for a more sensible implementation of EMPTY_MAP

[Abraham Marín Pérez]

Hi Stuart, Roger,

First of all, thank you for such a detailed response, this really shows the big 
picture. I guess no implementation will be perfect, there will always be some 
wrinkles that we need to accept, and the most suitable implementation will be 
the one with the fewest or least impacting wrinkles. Given this, maybe showing 
the particular wrinkle that I faced can shed light on the overall development 
experience that the current implementation provides.

For multiple reasons that I cannot explain at this point, I have a method that 
looks like the following:

private void decorate(Map data) {
//...
data.computeIfPresent("field", (k, v) -> highlightDifferences(v, 
otherValue));
//...
}

At one point an emptyMap() was passed to this method, causing an UOE. This left 
me with two choices:

1. Change code to:

if(data.hasKey(“field"))
data.compute("field", (k, v) -> highlightDifferences(v, otherValue));

2. Avoid usage of emptyMap() and use new HashMap<>() instead


The first option defeats the purpose of having a computeIfPresent method, and 
causes confusion among team members (people keep asking why computeIfPresent 
isn't used). The second option is pretty much what Roger mentioned (and what we 
ended up doing).

On the other hand, there are some points that you mentioned that I believe 
merit some extra debate, please find some comments below inline.

In general, I tend to agree that a stricter implementation is better since it 
usually prevents bugs. However, at least in this particular case, I believe a 
stricter implementation also has the side effect of worsening the development 
experience and adding confusion (see below comments). Of course, as I mentioned 
before, I understand that no implementation will be perfect, and maybe the 
current option is the lesser evil, but I wanted to throw in an alternative for 
consideration.

Many thanks,
Abraham


El vie., 14 jun. 2019 a las 14:18, Roger Riggs (mailto:roger.ri...@oracle.com>>) escribió:
Hi Stuart,

Not withstanding all the details. It would be useful (and possibly 
expected) that an EMPTY_MAP
could be substituted for a Map with no entries.  As it stands, the 
caller needs know whether any optional
possibly modifying operations will be used and decide whether to create 
an empty map or an EMPTY_MAP.
That makes using an EMPTY_MAP a risk and less useful and a cautious 
programmer will avoid it.

$.02, Roger


On 6/13/19 8:42 PM, Stuart Marks wrote:
>
>
> On 6/10/19 7:34 AM, Abraham Marín Pérez wrote:
>> I agree that it makes sense for EMPTY_MAP to have the same logic as 
>> Map.of() or unmodifiable(new HashMap()), which means that my 
>> suggestion cannot be applied to just EMPTY_MAP... but maybe that’s 
>> ok: maybe we can change all of them? If we keep the default 
>> implementation for Map.computeIfPresent in EMPTY_MAP, Map.of(), 
>> unmodifiableMap, etc., instead of overriding it to throw an 
>> exception, then:
>>
>> - For cases where the key isn’t present (regardless of empty or 
>> non-empt map), the call will be a safe no-op.
>> - For cases where the key is present, the call will still throw UOE 
>> via the implementation of put()
>>
>> I believe this would still respect the spec for Map.computeIfPresent 
>> while providing a more forgiving implementation (in the sense that it 
>> will avoid throwing an exception when possible).
>
> Hi Abraham,
>
> The specification does not mandate one behavior or another. Either 
> behavior is permitted, and in fact both behaviors are present in the JDK.
>
> The second and more significant point is raised by your last statement 
> suggesting a "more forgiving implementation." The question is, do we 
> actually want a more forgiving implementation?
>
> The collections specs define certain methods as "optional", but it's 
> not clear whether this means that calling such a method should always 
> throw an exception ("strict" behavior) or whether the method should 
> throw an exception only when it attempts to do something that's 
> disallowed ("lenient" behavior).
>
> Take for example the putAll() method. What happens if we do this?
>
> Map map1 = Collections.emptyMap();
> map1.putAll(otherMap);
>
> The answer is that it depends on the state of otherMap. If otherMap is 
> empty, then the operation succeeds (and does nothing). However, if 
> otherMap is non-empty, then the operation will throw 
> UnsupportedOperationException. That's an example of lenient behavior. 
> What's notable about this is that you can't predict the outcome unless 
> you know the state of otherMap.
>
> Now, how about this example?
>
> Map map2 = Map.of();
> map2.putAll(otherMap);
>
> This always throws UnsupportedOperationException, regardless of the 
> state of otherMap. I'd call this strict behavior.
>
> More recent implementations, including the Java 8 default methods, and 
> the Java 9 unmodifiable collections (Map.of et al), have all preferred 
> strict behavior. This is because less 

Re: RFR: 8225648:[TESTBUG] java/lang/annotation/loaderLeak/Main.java fails with -Xcomp

[Jie Fu]

Hi Alan,

I've updated the patch by adding the review info.
http://cr.openjdk.java.net/~jiefu/8225648/webrev.01/

Is it OK to be pushed?

Thanks a lot.
Best regards,
Jie

On 2019/6/19 下午2:04, Ioi Lam wrote:

Sure, you can add me as reviewer.

Thanks


On Jun 18, 2019, at 10:54 PM, Jie Fu  wrote:

Hi Ioi,

Are you OK if I add you as a reviewer for the change: 
http://cr.openjdk.java.net/~jiefu/8225648/webrev.00/

Thanks a lot.
Best regards,
Jie


On 2019/6/13 下午4:33, Jie Fu wrote:
Hi Ioi,

Thank you for your review.
You've taught me another way to fix this issue.

Reference.reachabilityFence seems to be widely used to handle this situation.
For example, you can just grep it in the test dir like this:

~/jdk-dev/test$ grep reachabilityFence . -r | wc -l
62


I think both approaches can solve this problem.
However, reachabilityFence seems more readable than a static field.

Thanks a lot.
Best regards,
Jie


On 2019/6/13 下午3:48, Ioi Lam wrote:
Hi Jie,

Instead of using an obscure call Reference.reachabilityFence(loader), how about just 
making "loader" a static field in the test class, so it will be kept alive 
until it's explicitly set to null?

Thanks
- Ioi


On 6/12/19 1:37 AM, Jie Fu wrote:
Hi all,

JBS:https://bugs.openjdk.java.net/browse/JDK-8225648
Webrev: http://cr.openjdk.java.net/~jiefu/8225648/webrev.00/

The failure was caused by the lost of an OopMap item for the object "loader"[1] 
which was optimized out by JIT with liveness analysis optimization.
It seems that this case is only suitable for testing interpreters which is not 
friendly to optimized JITs at all.

Although not every test would work with Xcomp (for various reasons), it's 
beneficial to make more tests suitable for JIT testing.
I just hope tests in tier1 would pass with Xcomp.
The goal seems to be reached if this bug and JDK-8225644[2] could be fixed.

Could you please review it?

Thanks a lot.
Best regards,
Jie

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/ae3dbc712839/test/jdk/java/lang/annotation/loaderLeak/Main.java#l48
[2] https://bugs.openjdk.java.net/browse/JDK-8225644