date:20190621

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

2019-06-21 Thread Joe Wang

Thanks Lance!

Joe

On 6/21/19, 1:59 PM, Lance Andersen wrote:

The revised webrev looks fine Joe
On Jun 21, 2019, at 4:20 PM, Joe Wang > wrote:

On 6/21/19, 11:59 AM, Daniel Fuchs wrote:

Hi Joe,

I promise, I will not grumble about the formatting and
weird white spaces [grumble grumble]...

Someone at BCEL needs to re-format the source with a modern IDE ;-)

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantUtf8.java.frames.html

Is the new import really needed here?

No, good catch.

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/JavaClass.java.frames.html

232 if (!dir.isDirectory()) {

I see use of SecuritySupport was dropped here and I think I agree,
as I don't see why isDirectory() would require it if mkdirs() doesn't.

I honestly don't remember why I added that in the previous update.
But you're right, and all test run proved it.

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/LineNumberTable.java.udiff.html

Are the changes in toString() actually correct?

No, it's not. Good catch again! The change in the last update was correct

Otherwise I believe the rest looks mostly OK.

Thanks Daniel! Here's an updated webrev:

Full webrev (for the record)
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02/

A short version of webrev_02 that includes the only files mentioned
in this review:

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02_short/

I'm re-running all tests.

Best regards,
Joe

best regards,

-- daniel

On 20/06/2019 23:45, Joe Wang wrote:
Please review an update to BCEL 6.3.1. This changeset will go into
JDK13 if approved, 14 if not.

1. Format
The changeset looks big, the majority of the changes however
were only different in format (i.e. Const.java). Unlike previous
updates, I'm leaving the format as they are in the upstream source
so that they won't show up in future updates. The only change I
made were those that had extremely long lines.

2. Exclusions
Since the BCEL component is for xml transform only, several
classes, that were not in the JDK, were excluded. Among them,
JavaWrapper.java for example can be problematic as it may use an
user-specified classloader to load arbitrary classes. It and
related classes were therefore excluded.

3. Warnings
Warnings were the main reason for the changes made to the
original source. It has been done in the previous update. For this
update therefore, I only had to re-apply them after making copies
of the upstream source. Still, I updated the LastModified field to
indicate a modification to the original source.

4. Deprecated fields to private
Deprecated fields in the original source were changed to
private ones in previous update. The changes are inherited in this
update. Again, the LastModified fields are also updated.

5. Test
Since the update does not affect the usage of the BCEL
component, it is essential to pass all of the existing tests. I've
run the tests multiple times and noted that all of the XML
functional and unit tests passed, so were JCK XML tests. I've also
done a comparison between builds before and after applying the BCEL
update, and found no change in performance.

JBS: https://bugs.openjdk.java.net/browse/JDK-8224157
webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/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 (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

2019-06-21 Thread Lance Andersen

The revised webrev looks fine Joe
> On Jun 21, 2019, at 4:20 PM, Joe Wang  wrote:
> 
> 
> 
> On 6/21/19, 11:59 AM, Daniel Fuchs wrote:
>> Hi Joe,
>> 
>> I promise, I will not grumble about the formatting and
>> weird white spaces [grumble grumble]...
> 
> Someone at BCEL needs to re-format the source with a modern IDE ;-)
> 
>> 
>> http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantUtf8.java.frames.html
>>  
>> 
>> Is the new import really needed here?
> 
> No, good catch.
>> 
>> http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/JavaClass.java.frames.html
>>  
>> 
>> 232 if (!dir.isDirectory()) {
>> 
>> I see use of SecuritySupport was dropped here and I think I agree,
>> as I don't see why isDirectory() would require it if mkdirs() doesn't.
> 
> I honestly don't remember why I added that in the previous update. But you're 
> right, and all test run proved it.
> 
>> 
>> 
>> http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/LineNumberTable.java.udiff.html
>>  
>> 
>> Are the changes in toString() actually correct?
> 
> No, it's not. Good catch again! The change in the last update was correct
>> 
>> Otherwise I believe the rest looks mostly OK.
> 
> Thanks Daniel!  Here's an updated webrev:
> 
> Full webrev (for the record)
> http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02/
> 
> A short version of webrev_02 that includes the only files mentioned in this 
> review:
> http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02_short/
> 
> 
> I'm re-running all tests.
> 
> Best regards,
> Joe
> 
>> 
>> best regards,
>> 
>> -- daniel
>> 
>> On 20/06/2019 23:45, Joe Wang wrote:
>>> Please review an update to BCEL 6.3.1. This changeset will go into JDK13 if 
>>> approved, 14 if not.
>>> 
>>> 1. Format
>>> The changeset looks big, the majority of the changes however were only 
>>> different in format (i.e. Const.java). Unlike previous updates, I'm leaving 
>>> the format as they are in the upstream source so that they won't show up in 
>>> future updates. The only change I made were those that had extremely long 
>>> lines.
>>> 
>>> 2. Exclusions
>>> Since the BCEL component is for xml transform only, several classes, 
>>> that were not in the JDK, were excluded. Among them, JavaWrapper.java for 
>>> example can be problematic as it may use an user-specified classloader to 
>>> load arbitrary classes. It and related classes were therefore excluded.
>>> 
>>> 3. Warnings
>>> Warnings were the main reason for the changes made to the original 
>>> source. It has been done in the previous update. For this update therefore, 
>>> I only had to re-apply them after making copies of the upstream source. 
>>> Still, I updated the LastModified field to indicate a modification to the 
>>> original source.
>>> 
>>> 4. Deprecated fields to private
>>> Deprecated fields in the original source were changed to private ones 
>>> in previous update. The changes are inherited in this update. Again, the 
>>> LastModified fields are also updated.
>>> 
>>> 5. Test
>>> Since the update does not affect the usage of the BCEL component, it is 
>>> essential to pass all of the existing tests. I've run the tests multiple 
>>> times and noted that all of the XML functional and unit tests passed, so 
>>> were JCK XML tests. I've also done a comparison between builds before and 
>>> after applying the BCEL update, and found no change in performance.
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8224157
>>> webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/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: New candidate JEP: 356: Enhanced Pseudo-Random Number Generators

2019-06-21 Thread mark . reinhold

2019/6/21 13:22:17 -0700, guy.ste...@oracle.com:
> On Jun 21, 2019, at 2:36 PM, mark.reinh...@oracle.com wrote:
>> By my count this JEP would add ten new public types to the java.util
>> package.  Is it time to consider creating a java.util.random subpackage?
> 
> Sure, that would be a completely reasonable move. Who should make that
> decision?

I suggest that you propose it, as part of this JEP.

It might make sense to put the key interface(s) directly in java.util,
with static factories for convenience.  Up to you.

- Mark

Re: RFR (JDK 14/java.xml) 8224157: BCEL: update to version 6.3.1

2019-06-21 Thread Joe Wang


Thanks Lance!

Yes, JDK 14 it is, as Mark has asked us to retarget it to.

Best,
Joe

On 6/21/19, 1:23 PM, Lance Andersen wrote:

Hi Joe,

Overall looks OK.   Its unfortunate to loose the formatting but given 
it is not upstream, best to not have the additional work of going 
through this exercise each time.


I have no additional changes to propose outside of what Daniel caught.

Even though there are a large number of files, the updates/impact 
would be minimal if approved for JDFK 13.  It might be easier though 
to just target JDK 14 given where we are in the release cycle.


HTH

Lance
On Jun 20, 2019, at 6:45 PM, Joe Wang > wrote:


Please review an update to BCEL 6.3.1. This changeset will go into 
JDK13 if approved, 14 if not.


1. Format
   The changeset looks big, the majority of the changes however were 
only different in format (i.e. Const.java). Unlike previous updates, 
I'm leaving the format as they are in the upstream source so that 
they won't show up in future updates. The only change I made were 
those that had extremely long lines.


2. Exclusions
   Since the BCEL component is for xml transform only, several 
classes, that were not in the JDK, were excluded. Among them, 
JavaWrapper.java for example can be problematic as it may use an 
user-specified classloader to load arbitrary classes. It and related 
classes were therefore excluded.


3. Warnings
   Warnings were the main reason for the changes made to the original 
source. It has been done in the previous update. For this update 
therefore, I only had to re-apply them after making copies of the 
upstream source. Still, I updated the LastModified field to indicate 
a modification to the original source.


4. Deprecated fields to private
   Deprecated fields in the original source were changed to private 
ones in previous update. The changes are inherited in this update. 
Again, the LastModified fields are also updated.


5. Test
   Since the update does not affect the usage of the BCEL component, 
it is essential to pass all of the existing tests. I've run the tests 
multiple times and noted that all of the XML functional and unit 
tests passed, so were JCK XML tests. I've also done a comparison 
between builds before and after applying the BCEL update, and found 
no change in performance.


JBS: https://bugs.openjdk.java.net/browse/JDK-8224157
webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/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 (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

2019-06-21 Thread Lance Andersen

Hi Joe,

Overall looks OK.   Its unfortunate to loose the formatting but given it is not 
upstream, best to not have the additional work of going through this exercise 
each time.

I have no additional changes to propose outside of what Daniel caught.

Even though there are a large number of files, the updates/impact would be 
minimal if approved for JDFK 13.  It might be easier though to just target JDK 
14 given where we are in the release cycle.

HTH

Lance
> On Jun 20, 2019, at 6:45 PM, Joe Wang  wrote:
> 
> Please review an update to BCEL 6.3.1. This changeset will go into JDK13 if 
> approved, 14 if not.
> 
> 1. Format
>The changeset looks big, the majority of the changes however were only 
> different in format (i.e. Const.java). Unlike previous updates, I'm leaving 
> the format as they are in the upstream source so that they won't show up in 
> future updates. The only change I made were those that had extremely long 
> lines.
> 
> 2. Exclusions
>Since the BCEL component is for xml transform only, several classes, that 
> were not in the JDK, were excluded. Among them, JavaWrapper.java for example 
> can be problematic as it may use an user-specified classloader to load 
> arbitrary classes. It and related classes were therefore excluded.
> 
> 3. Warnings
>Warnings were the main reason for the changes made to the original source. 
> It has been done in the previous update. For this update therefore, I only 
> had to re-apply them after making copies of the upstream source. Still, I 
> updated the LastModified field to indicate a modification to the original 
> source.
> 
> 4. Deprecated fields to private
>Deprecated fields in the original source were changed to private ones in 
> previous update. The changes are inherited in this update. Again, the 
> LastModified fields are also updated.
> 
> 5. Test
>Since the update does not affect the usage of the BCEL component, it is 
> essential to pass all of the existing tests. I've run the tests multiple 
> times and noted that all of the XML functional and unit tests passed, so were 
> JCK XML tests. I've also done a comparison between builds before and after 
> applying the BCEL update, and found no change in performance.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8224157
> webrevs: http://cr.openjdk.java.net/~joehw/jdk13/8224157/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: New candidate JEP: 356: Enhanced Pseudo-Random Number Generators

2019-06-21 Thread Guy Steele

Sure, that would be a completely reasonable move. Who should make that decision?

—Guy


> On Jun 21, 2019, at 2:36 PM, mark.reinh...@oracle.com wrote:
> 
> By my count this JEP would add ten new public types to the java.util
> package.  Is it time to consider creating a java.util.random subpackage?
> 
> - Mark

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

2019-06-21 Thread Joe Wang

On 6/21/19, 11:59 AM, Daniel Fuchs wrote:

Hi Joe,

I promise, I will not grumble about the formatting and
weird white spaces [grumble grumble]...

Someone at BCEL needs to re-format the source with a modern IDE ;-)

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantUtf8.java.frames.html

Is the new import really needed here?

No, good catch.

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/JavaClass.java.frames.html

232 if (!dir.isDirectory()) {

I see use of SecuritySupport was dropped here and I think I agree,
as I don't see why isDirectory() would require it if mkdirs() doesn't.

I honestly don't remember why I added that in the previous update. But
you're right, and all test run proved it.

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/LineNumberTable.java.udiff.html

Are the changes in toString() actually correct?

No, it's not. Good catch again! The change in the last update was correct

Otherwise I believe the rest looks mostly OK.

Thanks Daniel! Here's an updated webrev:

Full webrev (for the record)
http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02/

A short version of webrev_02 that includes the only files mentioned in
this review:

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev_02_short/

I'm re-running all tests.

Best regards,
Joe

best regards,

-- daniel

On 20/06/2019 23:45, Joe Wang wrote:
Please review an update to BCEL 6.3.1. This changeset will go into
JDK13 if approved, 14 if not.

1. Format
The changeset looks big, the majority of the changes however
were only different in format (i.e. Const.java). Unlike previous
updates, I'm leaving the format as they are in the upstream source so
that they won't show up in future updates. The only change I made
were those that had extremely long lines.

3. Warnings
Warnings were the main reason for the changes made to the
original source. It has been done in the previous update. For this
update therefore, I only had to re-apply them after making copies of
the upstream source. Still, I updated the LastModified field to
indicate a modification to the original source.

4. Deprecated fields to private
Deprecated fields in the original source were changed to private
ones in previous update. The changes are inherited in this update.
Again, the LastModified fields are also updated.

5. Test
Since the update does not affect the usage of the BCEL
component, it is essential to pass all of the existing tests. I've
run the tests multiple times and noted that all of the XML functional
and unit tests passed, so were JCK XML tests. I've also done a
comparison between builds before and after applying the BCEL update,
and found no change in performance.

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

Thanks,
Joe

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

2019-06-21 Thread Vladimir Kozlov


Thank you, Mandy

On 6/20/19 6:20 PM, Mandy Chung wrote:

Hi Vladimir,

As long as the owner of the test review the patch and confirm the
test policy intends to extend the default policy, those test change
will be fine.

test/jdk/java/lang/Class/getDeclaredField/FieldSetAccessibleTest.java

417 DEFAULT_POLICY.implies(domain, permission)) return true; Nit: separating this to a new if-case after 426 to make it 
distinct.


I did as you suggested:

@@ -420,6 +422,7 @@
 return true;
 }
 }
+if (DEFAULT_POLICY.implies(domain, permission)) return true;
 return false;
 }



I reviewed test/jdk/java/lang/Class/getDeclaredField/* and
test/jdk/java/lang/invoke/* patch and they look fine.


Thanks.



My suggestion is to minimize the risk of your patch.  Since Daniel
has reviewed the logging test change (which is the bulk of this patch),
I have no issue if the reviewers approve this patch.  I will file
a separate RFE for the test library idea.


I will be pushing my changes since I don't see objections in reviews.

Main discussion is how make these tests more robust which could be done 
separately.

Thanks,
Vladimir



Mandy


On 6/20/19 11:05 AM, Vladimir Kozlov wrote:

Hi Mandy

I am not sure about this suggestion. Graal module may not be present in the JDK 
(on SPARC for example).
And I don't want pollute general tests with Graal specific code: 
ModulePolicy.get("jdk.internal.vm.compiler").

If you or other have concerns about some tests looking on default policy I can keep them on problem list to run them 
later only with libgraal.


I found only 2 closed tests in which I had doubt about using default policy. I 
kept them on problem list.
Other tests, as I understand, manipulate permissions for test classes. They don't extend system classes so default 
policy should not affect test class permissions.


Thanks,
Vladimir

On 6/19/19 11:23 PM, Mandy Chung wrote:

Hi Vladimir,

Indeed these are test issues that the tests will need to grant permissions
to jdk.internal.vm.compiler as the default policy grants.

Thanks for going extra miles to fix the tests.

My suggestion may be a bit general.  What I intend to say the custom
security policy should extend the default policy unless it intentionally
excludes configuring permissions for specific modules.  I review the
the patch but the test doesn't clearly tell what the test intends to
do w.r.t. security configuration.

We should avoid inadvertently granting permissions that the test expects
to disallow.  A better solution is to limit granting permissions just for
`jdk.internal.vm.compiler` module rather than all.

Attached is ModulePolicy class that allows you to get the Policy for
a specific module. It can be put in the test library that these tests
can use them.

So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and
implies method will call the returned ModulePolicy if present.

test/lib/jdk/test/lib/security is one existing testlibrary for security
related stuff.  You can consider putting ModulePolicy.java there.

This is one idea.

Mandy

On 6/19/19 6:03 PM, Vladimir Kozlov wrote:

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: SizeOf an object? Why so hard?

2019-06-21 Thread Roger Riggs

Hi David,

What is the use case? What are you trying to optimize?

Thanks, Roger

On 6/19/19 10:10 PM, David Ryan wrote:

I'm in the process of moving a project from Java 8 to Java 11 and I've hit
a stumbling block on what I thought should be a rather simple problem.
During runtime, how do I get the size of an object in memory? Here are the
methods I've found so far:

1. The Unsafe way.

Use sun.misc.Unsafe to get the declared fields and find the field with the
largest objectFieldOffset. Use the sun.arch.data.model to calculate the
machine word (WORD = NR_BITS/8) size. Return ((maxOffset / WORD ) +1 ) *
WORD. This is reasonably accurate but obviously not friendly in Java 11.
Found on Stackoverflow [1].

2. Use java.lang.instrument.Instrumentation

Been around for a long time [2]. However, this requires configuring
special manifests for the project. Not very friendly for application
deployment, etc.

3. Use before/after memory consumption

Also mentioned in various stack overflow responses is the idea of calling
the garbage collector and memory usage before and after allocating and
using the difference for the size. I suppose this had the advantage of
providing a deeper estimate of the object. However, with other threads
doing work in the background this is mostly luck that we get the right
answer.

4. Add up the base types.

Also observed, investigate the types of each field and estimate the size of
the object based on the size of an arbitrary header length plus sizes for
each boolean, int, long, reference, etc. This misses padding added by the
JVM, so not as accurate as using UNSAFE in 1.

A few questions:
Is there another method I've missed?
Have I missed an obvious method that has been added in Java 11+?
Is there any plans to add anything in a future release?

[1]
https://stackoverflow.com/questions/9368764/calculate-size-of-object-in-java
[2]
https://amitstechblog.wordpress.com/2011/05/19/estimating-the-memory-usage-of-a-java-object/

Re: RFR: JDK-8226532: cleanup is not called when jpackage command fails.

2019-06-21 Thread Alexey Semenyuk


Looks good

- Alexey

On 6/21/2019 9:01 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).


Trivial fix to clean up the temp directory created by jpackage.

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

[2] http://cr.openjdk.java.net/~herrick/8226532/

/Andy

Re: RFR XS,docs,13 JDK-8226593 Fix HTML in com/sun/jdi/doc-files/signature.html

2019-06-21 Thread serguei.spit...@oracle.com


Hi Jon,

It looks good to me.

Thanks,
Serguei


On 6/21/19 11:58, Jonathan Gibbons wrote:

Please review a fix for an accessibility issue reported by Axe in
src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html

The issue is a conflict between an explicit `role="main">...``  specified in the file and the automatic 
`` added by javadoc.  The fix is just to remove the `` 
element.


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

-- Jon


$ hg diff -R open
diff -r 97c75e545302 
src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
--- a/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html 
Fri Jun 21 11:41:29 2019 -0700
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html 
Fri Jun 21 11:52:35 2019 -0700

@@ -40,7 +40,6 @@
 
 
 
-
 JDI Type Signatures
 
 JDI Type Signatures
@@ -74,6 +73,5 @@
 has the following type signature:
 (ILjava/lang/String;[I)J
 
-

Re: RFR XS,docs,13 JDK-8226593 Fix HTML in com/sun/jdi/doc-files/signature.html

2019-06-21 Thread Lance Andersen

+1
> On Jun 21, 2019, at 2:58 PM, Jonathan Gibbons  
> wrote:
> 
> Please review a fix for an accessibility issue reported by Axe in
> src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
> 
> The issue is a conflict between an explicit `...``  
> specified in the file and the automatic `` added by javadoc.  The fix 
> is just to remove the `` element.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8226593
> 
> -- Jon
> 
> 
> $ hg diff -R open
> diff -r 97c75e545302 
> src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
> --- a/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html Fri Jun 
> 21 11:41:29 2019 -0700
> +++ b/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html Fri Jun 
> 21 11:52:35 2019 -0700
> @@ -40,7 +40,6 @@
>  
>  
>  
> -
>  JDI Type Signatures
>  
>  JDI Type Signatures
> @@ -74,6 +73,5 @@
>  has the following type signature:
>  (ILjava/lang/String;[I)J
>  
> -
>  
>  
> 

 
  

 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 XS,docs,13 JDK-8226593 Fix HTML in com/sun/jdi/doc-files/signature.html

2019-06-21 Thread Jonathan Gibbons


Please review a fix for an accessibility issue reported by Axe in
src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html

The issue is a conflict between an explicit `role="main">...``  specified in the file and the automatic 
`` added by javadoc.  The fix is just to remove the `` element.


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

-- Jon


$ hg diff -R open
diff -r 97c75e545302 
src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html
--- a/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html Fri 
Jun 21 11:41:29 2019 -0700
+++ b/src/jdk.jdi/share/classes/com/sun/jdi/doc-files/signature.html Fri 
Jun 21 11:52:35 2019 -0700

@@ -40,7 +40,6 @@
 
 
 
-
 JDI Type Signatures
 
 JDI Type Signatures
@@ -74,6 +73,5 @@
 has the following type signature:
 (ILjava/lang/String;[I)J
 
-

Re: RFR (JDK13/java.xml) 8224157: BCEL: update to version 6.3.1

2019-06-21 Thread Daniel Fuchs

Hi Joe,

I promise, I will not grumble about the formatting and
weird white spaces [grumble grumble]...

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ConstantUtf8.java.frames.html

Is the new import really needed here?

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/JavaClass.java.frames.html

232 if (!dir.isDirectory()) {

I see use of SecuritySupport was dropped here and I think I agree,
as I don't see why isDirectory() would require it if mkdirs() doesn't.

http://cr.openjdk.java.net/~joehw/jdk13/8224157/webrev/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/LineNumberTable.java.udiff.html

Are the changes in toString() actually correct?

Otherwise I believe the rest looks mostly OK.

best regards,

-- daniel

On 20/06/2019 23:45, Joe Wang wrote:
Please review an update to BCEL 6.3.1. This changeset will go into JDK13
if approved, 14 if not.

1. Format
The changeset looks big, the majority of the changes however were
only different in format (i.e. Const.java). Unlike previous updates, I'm
leaving the format as they are in the upstream source so that they won't
show up in future updates. The only change I made were those that had
extremely long lines.

3. Warnings
Warnings were the main reason for the changes made to the original
source. It has been done in the previous update. For this update
therefore, I only had to re-apply them after making copies of the
upstream source. Still, I updated the LastModified field to indicate a
modification to the original source.

4. Deprecated fields to private
Deprecated fields in the original source were changed to private
ones in previous update. The changes are inherited in this update.
Again, the LastModified fields are also updated.

5. Test
Since the update does not affect the usage of the BCEL component,
it is essential to pass all of the existing tests. I've run the tests
multiple times and noted that all of the XML functional and unit tests
passed, so were JCK XML tests. I've also done a comparison between
builds before and after applying the BCEL update, and found no change in
performance.

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

Thanks,
Joe

Re: SizeOf an object? Why so hard?

2019-06-21 Thread Brian Burkhalter

Hi David,

I am not an expert in this area by any means but you might find the Java Object 
Layout (JOL) tool [1] useful.

Brian

[1] http://openjdk.java.net/projects/code-tools/jol/

> On Jun 19, 2019, at 7:10 PM, David Ryan  wrote:
> 
> A few questions:
> Is there another method I've missed?
> Have I missed an obvious method that has been added in Java 11+?
> Is there any plans to add anything in a future release?

Re: RFR: XS,docs,13 JDK-8226592: Fix HTML in table for jdk.zipfs module-info

2019-06-21 Thread Brian Burkhalter

Hi Jon,

+1

Brian

> On Jun 21, 2019, at 11:31 AM, Jonathan Gibbons  
> wrote:
> 
> Please review a tiny fix to the HTML for a table in jdk.zipfs 
> module-info.java.  The fix is just to change some cells from  to .  
> Because of the specified style (striped) for the table, there is no change in 
> the default appearance of this table.
> 
> The change is simple enough to show the patch below.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8226592
> 
> -- Jon
> 
> $ hg diff -R open
> diff -r e00591da418d src/jdk.zipfs/share/classes/module-info.java
> --- a/src/jdk.zipfs/share/classes/module-info.java Fri Jun 21 10:38:53 2019 
> -0700
> +++ b/src/jdk.zipfs/share/classes/module-info.java Fri Jun 21 11:25:09 2019 
> -0700
> @@ -60,7 +60,7 @@
>   *
>   * 
>   * 
> - *   create
> + *   create
>   *   java.lang.String
>   *   false
>   *   
> @@ -69,7 +69,7 @@
>   *   
>   * 
>   * 
> - *   encoding
> + *   encoding
>   *   java.lang.String
>   *   UTF-8
>   *   
>

Re: New candidate JEP: 356: Enhanced Pseudo-Random Number Generators

2019-06-21 Thread mark . reinhold

By my count this JEP would add ten new public types to the java.util
package.  Is it time to consider creating a java.util.random subpackage?

- Mark

SizeOf an object? Why so hard?

2019-06-21 Thread David Ryan

1. The Unsafe way.

2. Use java.lang.instrument.Instrumentation

Been around for a long time [2]. However, this requires configuring
special manifests for the project. Not very friendly for application
deployment, etc.

3. Use before/after memory consumption

4. Add up the base types.

A few questions:
Is there another method I've missed?
Have I missed an obvious method that has been added in Java 11+?
Is there any plans to add anything in a future release?

[1]
https://stackoverflow.com/questions/9368764/calculate-size-of-object-in-java
[2]
https://amitstechblog.wordpress.com/2011/05/19/estimating-the-memory-usage-of-a-java-object/

Re: RFR: XS,docs,13 JDK-8226592: Fix HTML in table for jdk.zipfs module-info

2019-06-21 Thread Lance Andersen

+1

> On Jun 21, 2019, at 2:31 PM, Jonathan Gibbons  
> wrote:
> 
> Please review a tiny fix to the HTML for a table in jdk.zipfs 
> module-info.java.  The fix is just to change some cells from  to .  
> Because of the specified style (striped) for the table, there is no change in 
> the default appearance of this table.
> 
> The change is simple enough to show the patch below.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8226592
> 
> -- Jon
> 
> $ hg diff -R open
> diff -r e00591da418d src/jdk.zipfs/share/classes/module-info.java
> --- a/src/jdk.zipfs/share/classes/module-info.java Fri Jun 21 10:38:53 2019 
> -0700
> +++ b/src/jdk.zipfs/share/classes/module-info.java Fri Jun 21 11:25:09 2019 
> -0700
> @@ -60,7 +60,7 @@
>   *
>   * 
>   * 
> - *   create
> + *   create
>   *   java.lang.String
>   *   false
>   *   
> @@ -69,7 +69,7 @@
>   *   
>   * 
>   * 
> - *   encoding
> + *   encoding
>   *   java.lang.String
>   *   UTF-8
>   *   
> 

 
  

 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: XS,docs,13 JDK-8226592: Fix HTML in table for jdk.zipfs module-info

2019-06-21 Thread Jonathan Gibbons

Please review a tiny fix to the HTML for a table in jdk.zipfs 
module-info.java.  The fix is just to change some cells from  to 
.  Because of the specified style (striped) for the table, there is 
no change in the default appearance of this table.


The change is simple enough to show the patch below.

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

-- Jon

$ hg diff -R open
diff -r e00591da418d src/jdk.zipfs/share/classes/module-info.java
--- a/src/jdk.zipfs/share/classes/module-info.java Fri Jun 21 10:38:53 
2019 -0700
+++ b/src/jdk.zipfs/share/classes/module-info.java Fri Jun 21 11:25:09 
2019 -0700

@@ -60,7 +60,7 @@
  *
  * 
  * 
- *   create
+ *   create
  *   java.lang.String
  *   false
  *   
@@ -69,7 +69,7 @@
  *   
  * 
  * 
- *   encoding
+ *   encoding
  *   java.lang.String
  *   UTF-8
  *

Re: java.util.regex.Matcher and StringBuilder/StringBuffer APIs

2019-06-21 Thread Robert Marcano


On 6/21/19 1:02 PM, Rob Spoor wrote:

Maybe because Appendable comes with IOExceptions?


Good catch. The alternative Appendable I was using don't declare throws 
IOException either, so I didn't noticed.


Implementing the find/appendReplacement/appendTail for other kind of 
Appendable like objects on client code instead of the JDK is not hard, 
the only part missing is access to the appendExpandedReplacement() 
implementation as some kind of API, It parses the replacement pattern 
and generate the replacement.


Another interesting thing is that appendReplacement() is using a 
temporary StringBuilder [1] instead of appending directly to the 
supplied builder. The only reason I see for that is that if the pattern 
is illegal, the intervening text isn't appended, so the client code can 
retry, or something like that, but I don't think it is common or 
recommended to be capturing IllegalArgumentException for something like 
this. I wonder if changing it to not use a temporary builder makes sense.



[1] 
http://hg.openjdk.java.net/jdk/jdk/file/e764228f71dc/src/java.base/share/classes/java/util/regex/Matcher.java#l997





On 21/06/2019 17:06, Robert Marcano wrote:
Greetings. Is there a reason the newest APIs added to Matcher 
(performance maybe?) with StringBuilder arguments weren't added as 
Appendable instead?


For example:

   public StringBuilder appendTail(StringBuilder sb)
   public Matcher appendReplacement(StringBuilder sb, String replacement)

Could have been:

   public  T appendTail(T ap)
   public Matcher appendReplacement(Appendable ap, String replacement)

Both appendReplacement(...) implementations are copies, that could be 
reduced to a simple one if Appendable was the argument, and the 
present ones calling to that.


If this sounds reasonable, I could write a patch for testing.

Note: I was hit with this when trying to use another kind of 
Appendable optimized for my use case.

New candidate JEP: 356: Enhanced Pseudo-Random Number Generators

2019-06-21 Thread mark . reinhold

https://openjdk.java.net/jeps/356

- Mark

Re: java.util.regex.Matcher and StringBuilder/StringBuffer APIs

2019-06-21 Thread Rob Spoor


Maybe because Appendable comes with IOExceptions?


On 21/06/2019 17:06, Robert Marcano wrote:
Greetings. Is there a reason the newest APIs added to Matcher 
(performance maybe?) with StringBuilder arguments weren't added as 
Appendable instead?


For example:

   public StringBuilder appendTail(StringBuilder sb)
   public Matcher appendReplacement(StringBuilder sb, String replacement)

Could have been:

   public  T appendTail(T ap)
   public Matcher appendReplacement(Appendable ap, String replacement)

Both appendReplacement(...) implementations are copies, that could be 
reduced to a simple one if Appendable was the argument, and the present 
ones calling to that.


If this sounds reasonable, I could write a patch for testing.

Note: I was hit with this when trying to use another kind of Appendable 
optimized for my use case.

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

2019-06-21 Thread Daniel Fuchs


Hi Sean,

On 21/06/2019 15:36, Sean Mullan wrote:
Ok, I see that is challenging, and I understand why you had to do that. 
You probably could have done something similar by breaking up the test 
into multiple classes in separate directories (or jars) with different 
ProtectionDomains, but that would require a bit of work, and maybe it is 
overkill. But I think then you could probably use the JDK Policy 
provider with a single policy file. (I'm not suggesting doing this right 
now, just was wondering if you considered this).




Yes - I considered it - but then it makes the test much
more difficult to understand (as its logic is split over
several files) - and requires either invoking
the compiler from within the test or moving classes around
after jtreg has compiled them in a @driver command so that
you can actually have different codesource locations.

Using a custom Policy implementation looked much simpler at the time.
And when you've done it once - well - copy & paste is your friend...
If there comes a time where having a custom policy implementation
becomes a maintenance issue and a technical debt then we/I will need
to take what time is required to revisit. But I'd rather not do it
if it's not a necessity.

best regards,

-- daniel

java.util.regex.Matcher and StringBuilder/StringBuffer APIs

2019-06-21 Thread Robert Marcano

Greetings. Is there a reason the newest APIs added to Matcher 
(performance maybe?) with StringBuilder arguments weren't added as 
Appendable instead?


For example:

  public StringBuilder appendTail(StringBuilder sb)
  public Matcher appendReplacement(StringBuilder sb, String replacement)

Could have been:

  public  T appendTail(T ap)
  public Matcher appendReplacement(Appendable ap, String replacement)

Both appendReplacement(...) implementations are copies, that could be 
reduced to a simple one if Appendable was the argument, and the present 
ones calling to that.


If this sounds reasonable, I could write a patch for testing.

Note: I was hit with this when trying to use another kind of Appendable 
optimized for my use case.

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

2019-06-21 Thread Sean Mullan

On 6/20/19 9:02 PM, Mandy Chung wrote:

On 6/20/19 1:40 PM, Sean Mullan wrote:

Sorry, I'm just catching up and looking at this now.

The one thing I don't like about these tests that use their own Policy
implementation is that the permissions that are granted inside the
test are granted to all code, and not just the test, which may lead to
cases where permissions that should be granted to JDK modules in
default.policy may be missed.

When the test wants a fine-control on turning on security manager
programmatically and test various security permission combinations,
I prefer to use a custom Policy implementation.

Otherwise, I have to maintain multiple different test.policy files to
test different permission combination which will be prone to editing
error.

I think we should provide a test library to help building a custom Policy
implementation that can take the default policy, preferrably the policy
for the resolved modules such that the test can focus its logic of
security configuration and does not need to code with the system
modules in mind.

It seems a little related to
https://bugs.openjdk.java.net/browse/JDK-8080294

--Sean

That's what I started with ModulePolicy and could potentially build up
test library for configuring security permissions programmatically.

Mandy

In tests that use policy files, we should grant permissions to only
the test, for example:

grant codebase "file:${test.classes}/.../-" {
permission ...;
};

However, in looking through our policy files, there are many that are
not doing that. Something to fix, so I'll file a bug.

This could also be fixed in these tests by inspecting the CodeSource
of the ProtectionDomain. Or better yet, they should just use the jtreg
java.security.policy option and a policy file and avoid the need to
create a Policy implementation.

Thanks,
Sean

On 6/20/19 11:04 AM, Mandy Chung wrote:

The Policy API does not assume the presence of the default policy for
the runtime to use.

jdk.internal.vm.compiler already uses doPrivileged. The module ends up
with no permission as the test policy does not consult the default
policy.

Mandy

On 6/20/19 6:32 AM, Roger Riggs wrote:

Hi,

If this were java.base, we would use doPrivilege to ignore the
policy and place specific limits.
Encumbering the default policy with conditions needed by a trusted
subsystem seems

like distributing what should be a local implementation issue.

$.02, Roger

On 6/20/19 2:23 AM, Mandy Chung wrote:

Hi Vladimir,

Indeed these are test issues that the tests will need to grant
permissions

to jdk.internal.vm.compiler as the default policy grants.

Thanks for going extra miles to fix the tests.

My suggestion may be a bit general. What I intend to say the custom
security policy should extend the default policy unless it
intentionally

excludes configuring permissions for specific modules. I review the
the patch but the test doesn't clearly tell what the test intends to
do w.r.t. security configuration.

We should avoid inadvertently granting permissions that the test
expects
to disallow. A better solution is to limit granting permissions
just for

`jdk.internal.vm.compiler` module rather than all.

Attached is ModulePolicy class that allows you to get the Policy for
a specific module. It can be put in the test library that these tests
can use them.

So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and
implies method will call the returned ModulePolicy if present.

test/lib/jdk/test/lib/security is one existing testlibrary for
security

related stuff. You can consider putting ModulePolicy.java there.

This is one idea.

Mandy

On 6/19/19 6:03 PM, Vladimir Kozlov wrote:

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: [14] RFR(M) 8185139: [Graal] Tests which set too restrictive security manager fail with Graal

2019-06-21 Thread Sean Mullan

On 6/21/19 7:15 AM, Daniel Fuchs wrote:

Hi Sean,

On 20/06/2019 21:40, Sean Mullan wrote:
This could also be fixed in these tests by inspecting the CodeSource of
the ProtectionDomain. Or better yet, they should just use the jtreg
java.security.policy option and a policy file and avoid the need to
create a Policy implementation.

In what logging tests are concerned that would be quite an undertaking.
For those tests, part of the test need to have permission to change
the configuration, and part of the test need to not have the
permission to double check that either the permission is not
required for those operations, or that the permission is checked - if
it is required.

Hence the use of a custom Policy implementation that uses
ThreadLocale to figure out whether to grant or
deny permissions.

Ok, I see that is challenging, and I understand why you had to do that.
You probably could have done something similar by breaking up the test
into multiple classes in separate directories (or jars) with different
ProtectionDomains, but that would require a bit of work, and maybe it is
overkill. But I think then you could probably use the JDK Policy
provider with a single policy file. (I'm not suggesting doing this right
now, just was wondering if you considered this).

--Sean

best regards,

-- daniel

Thanks,
Sean

On 6/20/19 11:04 AM, Mandy Chung wrote:

The Policy API does not assume the presence of the default policy for
the runtime to use.

jdk.internal.vm.compiler already uses doPrivileged. The module ends up
with no permission as the test policy does not consult the default
policy.

Mandy

On 6/20/19 6:32 AM, Roger Riggs wrote:

Hi,

If this were java.base, we would use doPrivilege to ignore the
policy and place specific limits.
Encumbering the default policy with conditions needed by a trusted
subsystem seems

like distributing what should be a local implementation issue.

$.02, Roger

On 6/20/19 2:23 AM, Mandy Chung wrote:

Hi Vladimir,

Indeed these are test issues that the tests will need to grant
permissions

to jdk.internal.vm.compiler as the default policy grants.

Thanks for going extra miles to fix the tests.

My suggestion may be a bit general. What I intend to say the custom
security policy should extend the default policy unless it
intentionally

excludes configuring permissions for specific modules. I review the
the patch but the test doesn't clearly tell what the test intends to
do w.r.t. security configuration.

We should avoid inadvertently granting permissions that the test
expects
to disallow. A better solution is to limit granting permissions
just for

`jdk.internal.vm.compiler` module rather than all.

Attached is ModulePolicy class that allows you to get the Policy for
a specific module. It can be put in the test library that these tests
can use them.

So the test can call ModulePolicy.get("jdk.internal.vm.compiler") and
implies method will call the returned ModulePolicy if present.

test/lib/jdk/test/lib/security is one existing testlibrary for
security

related stuff. You can consider putting ModulePolicy.java there.

This is one idea.

Mandy

On 6/19/19 6:03 PM, Vladimir Kozlov wrote:

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 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 (T): 8226203: MappedByteBuffer.force method may have no effect on implementation specific map modes

2019-06-21 Thread Andrew Dinn

Hi Alan,

On 21/06/2019 12:34, Alan Bateman wrote:
> I saw Joe's comment on the CSR. This refinement looks good to me.
I have pushed the doc fix to jdk13.

Once it percolates to jdk14 I will post a new webrev for the JEP 352
implementation which accomodates this change. That webrev will  also
include the changes pending after Joe's review of the implementation CSR
leaving (I hope) only my recently proposed JEP updates and endorsement
to finish before it can be pushed.

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: [11u]: RFR: 8215281: Use String.isEmpty() when applicable in java.base

2019-06-21 Thread Langer, Christoph

Hi Matthias,

thanks for your careful review of this tedious change.

I think the change in jdk.internal.module.Checks.java is ok to backport. It is 
no public interface and regression tests show no issue. So I wouldn't modify 
the patch in this place.

Any other opinions?

Best regards
Christoph

From: Baesken, Matthias
Sent: Dienstag, 18. Juni 2019 15:16
To: Langer, Christoph ; 
jdk-updates-...@openjdk.java.net
Cc: Java Core Libs 
Subject: RE: [11u]: RFR: 8215281: Use String.isEmpty() when applicable in 
java.base

Hello Christoph,   looks good   with  the exception of

src/java.base/share/classes/jdk/internal/module/Checks.java

http://cr.openjdk.java.net/~clanger/webrevs/8215281.11u/src/java.base/share/classes/jdk/internal/module/Checks.java.frames.html


 /**
- * Returns {@code true} if the given name is a legal module name.
- */
-public static boolean isModuleName(String name) {

   

-private static boolean isJavaIdentifier(CharSequence cs) {
-if (cs.length() == 0 || RESERVED.contains(cs))
+private static boolean isJavaIdentifier(String str) {
+if (str.isEmpty() || RESERVED.contains(str))


 ... where I am a bit  unsure - should we really  remove a method in jdk11  
(and change signature of another) ?  I know ,  it is  from  jdk/internal   , 
but still  I have a bad feeling about this .
This was probably fine for  the higher release  but for jdk11  it might be 
better   to keep what we have .

Best regards, Matthias


From: Langer, Christoph
Sent: Dienstag, 18. Juni 2019 10:59
To: jdk-updates-...@openjdk.java.net
Cc: Java Core Libs 
mailto:core-libs-dev@openjdk.java.net>>; 
Baesken, Matthias mailto:matthias.baes...@sap.com>>
Subject: [11u]: RFR: 8215281: Use String.isEmpty() when applicable in java.base

Hi,

please review the backport of the String.isEmpty() cleanups in java.base.

The main reason to bring this down to JDK11u is that it'll ease backporting of 
later changes which otherwise run into conflicts and won't resolve. 
Furthermore, the original change is a nice cleanup and improves performance.

Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8215281.11u/
Bug: https://bugs.openjdk.java.net/browse/JDK-8215281
Change in jdk/jdk: http://hg.openjdk.java.net/jdk/jdk/rev/8bf9268df0e2

The original patch merely applies (with a little fuzz in some places). There 
were 4 rejects:

src/java.base/share/classes/java/lang/VersionProps.java.template
--- VersionProps.java.template
+++ VersionProps.java.template
@@ -95,7 +95,7 @@
 props.put("java.version.date", java_version_date);
 props.put("java.runtime.version", java_runtime_version);
 props.put("java.runtime.name", java_runtime_name);
-if (VENDOR_VERSION_STRING.length() > 0)
+if (!VENDOR_VERSION_STRING.isEmpty())
 props.put("java.vendor.version", VENDOR_VERSION_STRING);

 props.put("java.class.version", CLASSFILE_MAJOR_MINOR);

-> manually resolved that to the right location

src/java.base/share/classes/java/text/CompactNumberFormat.java

-> file does not exist in 11u, so dropping

src/java.base/share/classes/jdk/internal/org/objectweb/asm/util/CheckMethodAdapter.java
--- CheckMethodAdapter.java
+++ CheckMethodAdapter.java
@@ -1314,7 +1314,7 @@
   * @param message the message to use in case of error.
   */
 static void checkMethodIdentifier(final int version, final String name, 
final String message) {
-if (name == null || name.length() == 0) {
+if (name == null || name.isEmpty()) {
 throw new IllegalArgumentException(INVALID + message + 
MUST_NOT_BE_NULL_OR_EMPTY);
 }
 if ((version & 0x) >= Opcodes.V1_5) {
@@ -1347,7 +1347,7 @@
   * @param message the message to use in case of error.
   */
 static void checkInternalName(final int version, final String name, final 
String message) {
-if (name == null || name.length() == 0) {
+if (name == null || name.isEmpty()) {
 throw new IllegalArgumentException(INVALID + message + 
MUST_NOT_BE_NULL_OR_EMPTY);
 }
 if (name.charAt(0) == '[') {
@@ -1457,7 +1457,7 @@
   * @param descriptor the string to be checked.
   */
 static void checkMethodDescriptor(final int version, final String 
descriptor) {
-if (descriptor == null || descriptor.length() == 0) {
+if (descriptor == null || descriptor.isEmpty()) {
 throw new IllegalArgumentException("Invalid method descriptor 
(must not be null or empty)");
 }
 if (descriptor.charAt(0) != '(' || descriptor.length() < 3) {

-> file looks different in 11u due to a missing change. So, modified the right 
places in the 11u version

src/java.base/share/classes/jdk/internal/org/objectweb/asm/util/CheckSignatureAdapter.java
--- CheckSignatureAdapter.java
+++ CheckSignatureAdapter.java
@@ -365,7 +365,7 @@
 }

 private void checkClassName(final String name,

Re: RFR: JDK-8226532: cleanup is not called when jpackage command fails.

2019-06-21 Thread Kevin Rushforth


Looks good.

-- Kevin

On 6/21/2019 6:01 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).


Trivial fix to clean up the temp directory created by jpackage.

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

[2] http://cr.openjdk.java.net/~herrick/8226532/

/Andy

RFR: JDK-8226532: cleanup is not called when jpackage command fails.

2019-06-21 Thread 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).


Trivial fix to clean up the temp directory created by jpackage.

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

[2] http://cr.openjdk.java.net/~herrick/8226532/

/Andy

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

2019-06-21 Thread Сергей Цыпанов

> Yes, StringJoiner is really good at joining Strings (hence the name).
> But it is not very good at joining primitives. Because you have to
> convert each primitive to a String before you can add it.
> StringBuilder does not need to allocate a String when you add a
> primitive.

I've reworked my patch to affect only the spots where we either already have 
Strings 
or do convertion to String (just like StringBuilder.append(Object) does using 
String::valueOf).

Also I've dropped erroneous changes to Atomic*Array (thanks Martin for spotting 
this).

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
@@ -5123,19 +5123,14 @@
 public static String toString(Object[] 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(String.valueOf(a[i]));
-if (i == iMax)
-return b.append(']').toString();
-b.append(", ");
+StringJoiner sj = new StringJoiner(", ", "[", "]");
+for (Object o : a) {
+sj.add(String.valueOf(o));
 }
+return sj.toString();
 }
 
 /**
diff --git a/src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java b/src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
--- a/src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
+++ b/src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
@@ -309,12 +309,10 @@
  * of an annotation.
  */
 private static String toSourceString(String s) {
-StringBuilder sb = new StringBuilder();
-sb.append('"');
+StringJoiner sb = new StringJoiner("", "\"", "\"");
 for (int i = 0; i < s.length(); i++) {
-sb.append(quote(s.charAt(i)));
+sb.add(quote(s.charAt(i)));
 }
-sb.append('"');
 return sb.toString();
 }
 
diff --git a/src/java.base/share/classes/sun/security/pkcs/SigningCertificateInfo.java b/src/java.base/share/classes/sun/security/pkcs/SigningCertificateInfo.java
--- a/src/java.base/share/classes/sun/security/pkcs/SigningCertificateInfo.java
+++ b/src/java.base/share/classes/sun/security/pkcs/SigningCertificateInfo.java
@@ -26,10 +26,9 @@
 package sun.security.pkcs;
 
 import java.io.IOException;
-import java.util.ArrayList;
+import java.util.StringJoiner;
 
 import sun.security.util.HexDumpEncoder;
-import sun.security.util.DerInputStream;
 import sun.security.util.DerValue;
 import sun.security.x509.GeneralNames;
 import sun.security.x509.SerialNumber;
@@ -92,14 +91,10 @@
 }
 
 public String toString() {
-StringBuilder sb = new StringBuilder();
-sb.append("[\n");
-for (int i = 0; i < certId.length; i++) {
-sb.append(certId[i].toString());
+StringJoiner sb = new StringJoiner("", "[\n", "\n]");
+for (ESSCertId id : certId) {
+sb.add(id.toString());
 }
-// format policies as a string
-sb.append("\n]");
-
 return sb.toString();
 }
 
diff --git a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ArrayElementValue.java b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ArrayElementValue.java
--- a/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ArrayElementValue.java
+++ b/src/java.xml/share/classes/com/sun/org/apache/bcel/internal/classfile/ArrayElementValue.java
@@ -23,6 +23,7 @@
 
 import java.io.DataOutputStream;
 import java.io.IOException;
+import java.util.StringJoiner;
 
 /**
  * @since 6.0
@@ -35,16 +36,10 @@
 @Override
 public String toString()
 {
-final StringBuilder sb = new StringBuilder();
-sb.append("{");
-for (int i = 0; i < evalues.length; i++)
-{
-sb.append(evalues[i]);
-if ((i + 1) < evalues.length) {
-sb.append(",");
-}
+StringJoiner sb = new StringJoiner(",", "{", "}");
+for (ElementValue evalue : evalues) {
+sb.add(evalue.stringifyValue());
 }
-sb.append("}");
 return sb.toString();
 }
 
@@ -71,16 +66,10 @@
 @Override
 public String stringifyValue()
 {
-final StringBuilder sb = new StringBuilder();
-sb.append("[");
-for (int i = 0; i < evalues.length; i++)
-{
-sb.append(evalues[i].stringifyValue());
-if ((i + 1) < evalues.length) {
-sb.append(",");
-}
+StringJoiner sb = new StringJoiner(",", "[",

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

2019-06-21 Thread Alan Bateman


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

:
+ *  If this buffer was not mapped in read/write mode
+ * ({@link java.nio.channels.FileChannel.MapMode#READ_WRITE})
+ * then invoking this method may have no effect. In particular,
+ * the method has no effect for buffers mapped in read-only or
+ * private mapping modes. This method may or may not have an
+ * effect for implementation-specific mapping modes. 

Would you agree with that change or do you prefer to stick with the
original? If necessary I'll amend the patch and CSR then push whichever
version you prefer to JDK13.


I saw Joe's comment on the CSR. This refinement looks good to me.

-Alan

Re: Question re: Usage of JNICALL with varargs function with 32bit compilers

2019-06-21 Thread Steve Groeger

Hi Thomas,

Although this is just a warning and doesn't cause any issues, other than 
them being generated and possibly causing the builds to fail, are you 
saying we would not want to spend time removing these warnings? I know 
this is only an issue when building using 32bit platforms and these 
platforms are few and far between as most platforms are 64 bit and as such 
I wouldn't want to spend time looking at this if this is deemed 
unnecessary. 

Thanks
Steve Groeger
IBM Runtime Technologies
Hursley, Winchester
Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
Fax (44) 1962 816800
Lotus Notes: Steve Groeger/UK/IBM
Internet: groe...@uk.ibm.com

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU



From:   "Thomas Stüfe" 
To: Steve Groeger 
Cc: core-libs 
Date:   18/06/2019 13:01
Subject:Re: Question re: Usage of JNICALL with varargs function 
with 32bit compilers



Hi Steve,

I always saw this as a harmless warning, safe to ignore, since both caller 
and callee are forced back to the same convention so it all works out in 
the end. Or can you envision a scenario where this would be harmfull?

Thank god Microsoft abandoned this calling convention circus with 64bit.

Cheers, Thomas


On Tue, Jun 18, 2019 at 1:31 PM Steve Groeger  wrote:
Hi all, 

I had a query regarding the use of JNICALL with varargs functions, which I 

am hoping someone can comment on.

As explained on MSDN at [1], all functions declared as __stdcall that have 

variable parameters (varargs) will be forced back to __cdecl by the MSVC. 
In such case, keeping JNICALL for varargs functions/function pointers 
turns out be inappropriate.
e.g.

include/jni.h
jobject (JNICALL *NewObject)
  (JNIEnv *env, jclass clazz, jmethodID methodID, ...);

According to [2], stdcall does not support variadic calls in C on 32-bit 
x86 targets. 

When compiling a JNI native (calling NewObject) with Clang, it ends up 
with the warning as follows:
...\include\jni.h:277:14: warning: stdcall calling convention ignored on 
variadic function [-Wignored-attributes]
jobject (JNICALL *NewObject)
 ^

So basically the compiler is saying it can't do what was requested, as 
such should we stop asking for JNICALL on varargs functions. 
Doing so will only improve the portability of code to a wider set of 
compilers.

Comments?

[1] https://msdn.microsoft.com/en-us/library/zxk0tw93.aspx
[2] https://clang.llvm.org/docs/AttributeReference.html#stdcall

Thanks
Steve Groeger
IBM Runtime Technologies
Hursley, Winchester
Tel: (44) 1962 816911  Mobex: 279990  Mobile: 07718 517 129
Fax (44) 1962 816800
Lotus Notes: Steve Groeger/UK/IBM
Internet: groe...@uk.ibm.com

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU



Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

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

2019-06-21 Thread Andrew Dinn

Hi Alan,

On 19/06/2019 14:11, Alan Bateman wrote:
> 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.
Joe Darcy has approved the CSR. However, he suggested by way of code
review that the comment be tweaked to explicitly state that the force
method does have an effect when the buffer was mapped with mode
READ_WRITE e.g. instead of

*  This method has no effect for buffers mapped in read-only
* or private mapping modes. It may also have no effect for other
* implementation specific map modes. 

this

+ *  If this buffer was not mapped in read/write mode
+ * ({@link java.nio.channels.FileChannel.MapMode#READ_WRITE})
+ * then invoking this method may have no effect. In particular,
+ * the method has no effect for buffers mapped in read-only or
+ * private mapping modes. This method may or may not have an
+ * effect for implementation-specific mapping modes. 

Would you agree with that change or do you prefer to stick with the
original? If necessary I'll amend the patch and CSR then push whichever
version you prefer to JDK13.

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: 8224502 & 8224506 Container testbugs

2019-06-21 Thread Severin Gehwolf

Hi Bob,

On Thu, 2019-06-20 at 09:31 -0400, Bob Vandette wrote:
> Please review these two Docker container test bugs that I’d like to get fixed 
> in JDK13.
> Misha has already reviewed and approved these fixes.
> 
> I’ve tested these changes by running all Docker container tests locally with 
> a fastdebug build
> and also under mach5.
> 
> BUG:
> https://bugs.openjdk.java.net/browse/JDK-8224502
> [TESTBUG] JDK docker test TestSystemMetrics.java fails with access issues and 
> OOM
> 
> WEBREV:
> http://cr.openjdk.java.net/~bobv/8224502/webrev.01

This looks good to me.

> This problem is caused by the test consuming too much memory.  The author
> expected to allocate a 64MB array but instead allocated 512MB.  I also
> added a container memory limit of 256MB in order to ensure that if the host 
> running
> the test does not have sufficient memory, we’ll get a better handle on the 
> cause of the failure.
> 
> --
> 
> BUG:
> https://bugs.openjdk.java.net/browse/JDK-8224506
> [TESTBUG] TestDockerMemoryMetrics.java fails with exitValue = 137
> 
> WEBREV:
> http://cr.openjdk.java.net/~bobv/8224506/webrev.01
> 
> This problem occurs when the test is run with a fastdebug build.  The 
> container
> that is being created has a memory limit of 20MB which is too small.

This looks good to me too.

Thanks,
Severin

> 
> NOTE: I will commit each fix individually with incremental changes to 
> ProblemList.txt.
> 
> 
> Bob.
> 
>

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

2019-06-21 Thread Peter Levart





On 6/21/19 9:41 AM, Andrew Haley wrote:

On 6/20/19 9:31 PM, Peter Levart wrote:


I would also add overflow checks when computing the length of
resulting byte[]. First I would pre-check the length of passed in
int[] array (it must be less than Integer.MAX_VALUE / 3), then
checking for negative size after each addition of element length,
throwing OOME if overflow happens.

OutOfMemoryException? Are you sure? The system isn't out of memory or
any other resource, it's just that the arguments are too large. Also,
it might be cleaner to use addExact().



StringBuilder throws OutOfMemoryError when appending over its maximum 
capacity, so I thought this would keep that behavior.


Regards, Peter

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

2019-06-21 Thread Andrew Haley

On 6/20/19 9:31 PM, Peter Levart wrote:

> I would also add overflow checks when computing the length of
> resulting byte[]. First I would pre-check the length of passed in
> int[] array (it must be less than Integer.MAX_VALUE / 3), then
> checking for negative size after each addition of element length,
> throwing OOME if overflow happens.

OutOfMemoryException? Are you sure? The system isn't out of memory or
any other resource, it's just that the arguments are too large. Also,
it might be cleaner to use addExact().

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

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

2019-06-21 Thread Kasper Nielsen

On Thu, 20 Jun 2019 at 21:31, Peter Levart  wrote:
>
>
> On 6/20/19 2:39 PM, Kasper Nielsen wrote:
> >> If you allowed a two-time pass of the primitive array you could
> >> actually create a version that only allocated the actual String and
> >> backing array.
> > I tried implementing it
> > https://gist.github.com/kaspernielsen/62e4eedffdb395228777925551a45e7f
> > And got a 30-40 % performance increase.
>
> This is nice and garbage-free.
>
> This method is not very performance critical I think (unless someone is
> using it to format int[] into JSON for example), so I don't know whether
> optimizing it is that important. OTOH the optimized code is not that
> difficult to understand, so why not? I would also add overflow checks
> when computing the length of resulting byte[]. First I would pre-check
> the length of passed in int[] array (it must be less than
> Integer.MAX_VALUE / 3), then checking for negative size after each
> addition of element length, throwing OOME if overflow happens. Then I
> would re-check if the performance is still favorable and see if the
> resulting method is not too complicated to understand after all.

You actually don't need to check on each element. Use a long instead of an
int to sum up all sizes. And then just check once, which should not effect
performance.

But implementing it do require adding a public method to (preferable) every
primitive wrapper. As Arrays is in java.util and needs access to package
private methods (stringSize() and getChars()) in each wrapper. One way to do
it would be to add:

class Integer
   public static String toArrayString(int... ints)

I do actually occasionally miss such a method with varargs sometimes.
I also think I would let the method throw an NPE on null instead of
returning "null".
And change the implementation of Arrays.toString to

public static String toString(int[] a) {
  if (a == null)
return "null";
  return Integer.toStringArray(a);
}

/Kasper

39 matches

Mail list logo