Re: [10] RFR(M) 8182701: Modify JVMCI to allow Graal Compiler to expose platform MBean

[Mandy Chung]

Vladimir,

I believe you don’t want to add the dependency from JVMCI to java.management.  
Otherwise, JVMCI can’t run with only java.base.  Is the MBean in 
jdk.internal.vm.compiler or in Lab’s Graal compiler?

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/services/src/jdk/vm/ci/services/internal/JVMCIMXBeans.java
- I suspect this file meant to be in 
src/jdk.internal.vm.ci/share/classes/src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/services/internal/JVMCIMXBeans.java

Mandy

> On Jul 12, 2017, at 2:20 PM, Vladimir Kozlov  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8182701
> webrev:
> http://cr.openjdk.java.net/~kvn/8182701/webrev.jdk/
> http://cr.openjdk.java.net/~kvn/8182701/webrev.hs/
> 
> Contributed by Jaroslav Tulach.
> 
> JDK itself contains quite a lot of platform MBeans which get registered "on 
> demand". Graal compiler (jdk.internal.vm.compiler) provides its own MBean(s) 
> - however currently there is no way to register it "on demand". JDK9 already 
> contains support for collecting platform MBeans from various modules. We just 
> need to expose Graal MBean through JVMCI.
> 
> Thanks,
> Vladimir

Re: RFR: JDK-8184217: Redundant tag before list

[Lance Andersen]

+1

> On Jul 24, 2017, at 4:44 PM, Jonathan Gibbons  
> wrote:
> 
> Please review a small fix to the documentation for the java.rmi module.
> 
> No webrev; here is the patch to remove an unnecessary line:
> 
> $ hg diff -R jdk
> diff -r 13119f57b8da src/java.rmi/share/classes/module-info.java
> --- a/src/java.rmi/share/classes/module-info.javaMon Jul 24 10:18:33 2017 
> -0400
> +++ b/src/java.rmi/share/classes/module-info.javaMon Jul 24 13:37:15 2017 
> -0700
> @@ -31,7 +31,6 @@
>  * object registry, and the {@index rmid rmid tool} tool to start
>  * the activation system daemon.
>  *
> - * 
>  * 
>  * Tool Guides:
>  *  {@extLink rmiregistry_tool_reference rmiregistry},
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8184217
> 
> -- Jon

 
  

 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: JDK-8184217: Redundant tag before list

[Jonathan Gibbons]

Please review a small fix to the documentation for the java.rmi module.

No webrev; here is the patch to remove an unnecessary line:

$ hg diff -R jdk
diff -r 13119f57b8da src/java.rmi/share/classes/module-info.java
--- a/src/java.rmi/share/classes/module-info.javaMon Jul 24 10:18:33 
2017 -0400
+++ b/src/java.rmi/share/classes/module-info.javaMon Jul 24 13:37:15 
2017 -0700

@@ -31,7 +31,6 @@
  * object registry, and the {@index rmid rmid tool} tool to start
  * the activation system daemon.
  *
- * 
  * 
  * Tool Guides:
  *  {@extLink rmiregistry_tool_reference rmiregistry},

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

-- Jon

Re: RFR: 8185150: javax/activation/CommandInfo.html has empty

[Jonathan Gibbons]

Thanks, Lance.

-- Jon

On 07/24/2017 01:16 PM, Lance Andersen wrote:

+1
On Jul 24, 2017, at 4:07 PM, Jonathan Gibbons 
> wrote:


Please review a trivial fix to remove a superfluous  in a javadoc 
comment.


No webrev; here is the patch:

$ hg diff -R jaxws
diff -r 97e67df03f88 
src/java.activation/share/classes/javax/activation/CommandInfo.java
--- 
a/src/java.activation/share/classes/javax/activation/CommandInfo.java 
Thu Jul 20 18:17:14 2017 +
+++ 
b/src/java.activation/share/classes/javax/activation/CommandInfo.java 
Mon Jul 24 13:03:03 2017 -0700

@@ -112,7 +112,7 @@
 * this method will check if it implements the
 * java.io.Externalizable interface. If it does, the bean's
 * readExternal method will be called if an InputStream
- * can be acquired from the DataHandler.
+ * can be acquired from the DataHandler.
 *
 * @param dhThe DataHandler that describes the data to be
 *  passed to the command.


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

-- Jon




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: 8185150: javax/activation/CommandInfo.html has empty

[Lance Andersen]

+1
> On Jul 24, 2017, at 4:07 PM, Jonathan Gibbons  
> wrote:
> 
> Please review a trivial fix to remove a superfluous  in a javadoc comment.
> 
> No webrev; here is the patch:
> 
> $ hg diff -R jaxws
> diff -r 97e67df03f88 
> src/java.activation/share/classes/javax/activation/CommandInfo.java
> --- a/src/java.activation/share/classes/javax/activation/CommandInfo.java Thu 
> Jul 20 18:17:14 2017 +
> +++ b/src/java.activation/share/classes/javax/activation/CommandInfo.java Mon 
> Jul 24 13:03:03 2017 -0700
> @@ -112,7 +112,7 @@
>  * this method will check if it implements the
>  * java.io.Externalizable interface. If it does, the bean's
>  * readExternal method will be called if an InputStream
> - * can be acquired from the DataHandler.
> + * can be acquired from the DataHandler.
>  *
>  * @param dhThe DataHandler that describes the data to be
>  *  passed to the command.
> 
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8185150
> 
> -- Jon

 
  

 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: 8185150: javax/activation/CommandInfo.html has empty

[Jonathan Gibbons]

Please review a trivial fix to remove a superfluous  in a javadoc 
comment.


No webrev; here is the patch:

$ hg diff -R jaxws
diff -r 97e67df03f88 
src/java.activation/share/classes/javax/activation/CommandInfo.java
--- 
a/src/java.activation/share/classes/javax/activation/CommandInfo.java 
Thu Jul 20 18:17:14 2017 +
+++ 
b/src/java.activation/share/classes/javax/activation/CommandInfo.java 
Mon Jul 24 13:03:03 2017 -0700

@@ -112,7 +112,7 @@
  * this method will check if it implements the
  * java.io.Externalizable interface. If it does, the bean's
  * readExternal method will be called if an InputStream
- * can be acquired from the DataHandler.
+ * can be acquired from the DataHandler.
  *
  * @param dhThe DataHandler that describes the data to be
  *  passed to the command.


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

-- Jon

Re: JDK 10 RFR of JDK-8183377: Refactor java/lang/ClassLoader/deadlock shell tests to java

[Mandy Chung]

> On Jul 17, 2017, at 12:49 AM, Amy Lu  wrote:
> 
> On 7/14/17 4:41 PM, Mandy Chung wrote:
>> I think compiling the other classes to a destination explicitly to replace:
>>   29  * @build Alice Bob SupAlice SupBob
>>   30  * @run driver SetupLoader
>> 
>> will make the test clearer.  It’d be good to make that change.
> 
> Webrev updated: http://cr.openjdk.java.net/~amlu/8183377/webrev.01/

Looks good.  One minor suggestion is to place the source files under a 
directory showing its package hierachy e.g. src/comSA/Alice.java, 
src/comSB/Bob.java

No need for a new webrev.

thanks
Mandy

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Roger Riggs]

Hi Ivan,

Thanks for bringing this up again.

Some initial comments, not a full review.

Though the enhancement says that no consideration is given to sign 
characters they
may produce puzzling results for strings like "-2000" and "-2001" where 
the strings appear
to be signed numbers but the comparison will be for the "-" prefix and 
the rest unsigned.
Including the word 'unsigned' in the description might help reinforce 
the semantics.


Also, I don't see test cases for strings like:  "abc200123" and 
"abc2123" which share
a prefix part of which is numeric but differ in the number of "leading" 
zeros after the prefix.


What about strings that include more than 1 numeric segment: 
abc2003def0001 and abc02003def001

in the leading zero case?

Though the test adds the @key randomness, it would be useful to use the 
test library
mechanisms to report the seed and be able to run the test with a seed, 
if case they fail.

(As might be provided by the test library jdk.test.lib.RandomFactory).


Comparator.java:
576: "the common prefix is skipped" is problematic when considering 
leading zeros.

   The common prefix may contain non-zero digits.
585: it is not clear whether the "different number of leading zeros" is 
applied regardless

   of the common prefix or only starting after the common prefix.

550, 586: for many readers, it is easier to read 'for example' than 
"e.g." or "i.e.".


562, 598:  editorial: "is to compare" -> "compares"

Comparators.java:

192: @param for param o is missing;  (the param name "o" usually refers 
to an object, not a string).
200:   Can skipLeadingZeros be coded to correctly work if cnt == 0; it 
would be more robust
 SkipLeading zeros works correctly only if pos is given the first 
numeric digit in the subsequence

 so the numStart1 and numStart2 must be first digit in each string.

compare():

Line 223: if strings typically have non-numeric prefixes, it might 
perform slightly better
to detect the end of the common prefix and then scan back to find the 
beginning of the numeric part.

(Avoiding checking every char for isDigit).

Line 224: If assigned for every digit, it will hold the last equal digit 
in the common prefix, not the first digit.


239, 240:  chars at o1(index) and o2(index) are already known to be 
digits, can it be avoided checking them twice

by starting at index+1?

$.02, Roger


On 7/19/2017 4:41 AM, Ivan Gerasimov wrote:

Hello!

It is a proposal to provide a String comparator, which will pay 
attention to the numbers embedded into the strings (should they present).


This proposal was initially discussed back in 2014 and seemed to bring 
some interest from the community:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-December/030343.html 



In the latest webrev two methods are added to the public API:
j.u.Comparator.comparingNumerically() and
j.u.Comparator.comparingNumericallyLeadingZerosAhead().

The regression test is extended to exercise this new comparator.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8134512
WEBREV: http://cr.openjdk.java.net/~igerasim/8134512/01/webrev/

Comments, suggestions are very welcome!

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Jonathan Bluett-Duncan]

You're welcome Ivan!

I'm just proofreading the new webrev, and a few more things have occurred
to me:

1. What happens if the comparators are given the elements {"1abc", "2abc",
"10abc"} to compare? Would they sort the elements as {"1abc", "2abc",
"10abc"} or as {"1abc", "10abc", "2abc"}?
What about the array {"x1yz", "x2yz", "x10yz"}?
I wonder if these two cases should be added to the test too and given as
additional examples in the javadocs.

2. The example arrays which you kindly added to the comparators' javadoc
have no whitespace at the start but one space between the last element and
the }, so I think there either should be no space at the end or an extra
space at the start.

3. Very sorry, error on my part: on the javadoc lines which now say "then the
corresponding numerical values are compared; otherwise", I suggested to add
a "then" at the start; re-reading it though, I personally think it reads
better without, but I would understand and not press it further if you
disagree and think that the "then" is a useful addition.

Best regards,
Jonathan


On 24 Jul 2017 06:21, "Ivan Gerasimov"  wrote:

Thank you Jonathan for looking into that!

I agree with all your suggestions.

Here's the updated webrev with suggested modifications:
WEBREV: http://cr.openjdk.java.net/~igerasim/8134512/02/webrev/

With kind regards,
Ivan


On 7/23/17 3:56 AM, Jonathan Bluett-Duncan wrote:

Meant to reply to core-libs-dev as well; doing so now.

Jonathan

On 23 July 2017 at 11:50, Jonathan Bluett-Duncan 
wrote:

> Hi Ivan,
>
> I'm not a reviewer per se, but I thought I'd chime in.
>
> There's a few things with Comparator.java which I think could be improved:
>
>1. `comparingNumericallyLeadingZerosAhead()` is a confusing name for
>me, as the "ahead" has no meaning in my mind; IMO the name
>`comparingNumericallyLeadingZerosFirst()` better implies that "0001" <
>"001" < "01".
>2. It wasn't clear to me from the javadoc that the comparators compare
>strings like "abc9" and "abc10" as "abc9" < "abc10", so I think they should
>include more examples.
>3. There's a typo in the javadocs for both methods: "represets" -->
>"represents".
>4. Where the javadocs say "follows the procedure", I think it'd make
>more grammatical sense if they said "does the following".
>5. The lines which say "at the boundary of a subsequence, consisting
>of decimal digits, the" would flow better if they said "at the boundary of
>a subsequence *consisting solely of decimal digits*, then the". Note
>the removal of the comma between "subsequence" and "consisting".
>
> Hope this helps!
>
> Jonathan
>
> On 23 July 2017 at 05:36, Ivan Gerasimov 
> wrote:
>
>> Hello!
>>
>> This is a gentle reminder.
>>
>> The proposed comparator implementation would be particularly useful when
>> one will need to compare version strings.
>> Some popular file managers also use similar comparing algorithm, as the
>> results often look more natural to the human eyes (e.g. File Explorer on
>> Windows, Files on Ubuntu).
>>
>> Now, as Java 10 is been worked on, to sort the list of Java names
>> correctly, this kind of comparator is needed:
>>
>> Look: a list { ... "Java 8", "Java 9", "Java 10" } definitely looks nicer
>> than { "Java 1", "Java 10", "Java 2", ... }  :-)
>>
>> Would you please help review the proposal?
>>
>> With kind regards,
>> Ivan
>>
>>
>>
>> On 7/19/17 1:41 AM, Ivan Gerasimov wrote:
>>
>>> Hello!
>>>
>>> It is a proposal to provide a String comparator, which will pay
>>> attention to the numbers embedded into the strings (should they present).
>>>
>>> This proposal was initially discussed back in 2014 and seemed to bring
>>> some interest from the community:
>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-De
>>> cember/030343.html
>>>
>>> In the latest webrev two methods are added to the public API:
>>> j.u.Comparator.comparingNumerically() and
>>> j.u.Comparator.comparingNumericallyLeadingZerosAhead().
>>>
>>> The regression test is extended to exercise this new comparator.
>>>
>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8134512
>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8134512/01/webrev/
>>>
>>> Comments, suggestions are very welcome!
>>>
>>>
>> --
>> With kind regards,
>> Ivan Gerasimov
>>
>>
>

-- 
With kind regards,
Ivan Gerasimov

Re: [10] RFR: 8185104: Generate CharacterDataLatin1 lookup tables directly

[Claes Redestad]

On 07/24/2017 10:29 AM, Alan Bateman wrote:


On 23/07/2017 14:37, Claes Redestad wrote:
Proposed patch is to drop passing -string to GenerateCharacter for 
the latin1 case:


Webrev: http://cr.openjdk.java.net/~redestad/8185104/jdk.00/
Bug:https://bugs.openjdk.java.net/browse/JDK-8185104
This is a good sleuthing. I can't see of any reason why the tables in 
CharacterDataLatin1 need to be generated as Strings now I think this 
change is good.


Thanks for reviewing, Alan!

/Claes

Re: [10] RFR: 8185104: Generate CharacterDataLatin1 lookup tables directly

[Alan Bateman]

On 23/07/2017 14:37, Claes Redestad wrote:

Hi,

java.lang.CharacterDataLatin1 and others are generated at build time 
by the GenerateCharacter tool, which has a -string mode to generate 
lookup tables as Strings literals rather than arrays directly. This 
serves multiple purposes:


1. it reduces the size of the generated bytecode, which is necessary 
to keep code below method bytecode limits if the arrays generated are 
very large
2. it may under certain circumstances (large enough arrays, JITed 
code) be a performance optimization


While having other performance benefits, the compact strings feature 
that went into 9 made String::toCharArray less friendly to startup, 
and since the same feature means we're now always loading 
CharacterDataLatin1 on bootstrap in all locales it seemed reasonable 
to re-examine whether this class in particular really gains from 
generating its lookup tables via String literals.


Turns out it doesn't. By generating CharacterDataLatin1 tables as 
arrays explicitly:


- executed bytecode drop from 21,782 to 2,077 when initializing this 
class (approximately 2% of executed bytecode; 1.5% of total instructions)

- slight reduction (~1Kb) in the minimum retained heap usage
- the size of CharacterDataLatin1.class only grows from 6458 to 7385 
bytes


Proposed patch is to drop passing -string to GenerateCharacter for the 
latin1 case:


Webrev: http://cr.openjdk.java.net/~redestad/8185104/jdk.00/
Bug:https://bugs.openjdk.java.net/browse/JDK-8185104
This is a good sleuthing. I can't see of any reason why the tables in 
CharacterDataLatin1 need to be generated as Strings now I think this 
change is good.


-Alan