Re: RFR (12): 8213300: jaxp/unittest/transform/CR6551600Test.java fails due to exception in jdk/jdk CI

[Joe Wang]

Hi Frank,

The change looks good to me. Thanks for fixing the failure!

Best,
Joe

On 12/4/18, 6:19 PM, Frank Yuan wrote:

Hi all



Would you like to review this patch?

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

Webrev: http://cr.openjdk.java.net/~fyuan/8213300/webrev.00/



This patch made the following changes:

1.   change the path of test file from the root directory of drive C to 
C:\temp directory

2.   check if the UNC path is accessible before running the jaxp test

3.   skip the test if it's not Windows because the UNC test is only valid 
on Windows



Thanks

Frank

[12]RFR 8213127: Refactor test/java/util/ResourceBundle/Control/MissingResourceCauseTest.sh to plain java tests

[Dora Zhou]

Hello,

Please help review the fix for refactor 
test/java/util/ResourceBundle/Control/MissingResourceCauseTest.sh to 
plain java tests. Thank you.



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

Webrev: http://cr.openjdk.java.net/~dzhou/8213127/webrev.02/

Regards,
Dora

RFR (12): 8213300: jaxp/unittest/transform/CR6551600Test.java fails due to exception in jdk/jdk CI

[Frank Yuan]

Hi all

 

Would you like to review this patch? 

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

Webrev: http://cr.openjdk.java.net/~fyuan/8213300/webrev.00/

 

This patch made the following changes:

1.   change the path of test file from the root directory of drive C to 
C:\temp directory

2.   check if the UNC path is accessible before running the jaxp test

3.   skip the test if it's not Windows because the UNC test is only valid 
on Windows

 

Thanks

Frank

 

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Brian Burkhalter]

Hi Daniel,

You are correct: at line 154 ’n’ should have been passed instead of ‘-1’ here:

http://cr.openjdk.java.net/~bpb/6516099/webrev.08-delta/ 


Fixing that exposed an error in the test at line 222 which I’ve fixed.

An updated version is at

http://cr.openjdk.java.net/~bpb/6516099/webrev.09/ 


with the 08-09 delta at

http://cr.openjdk.java.net/~bpb/6516099/webrev.08-09-delta/ 


Thanks,

Brian

> On Dec 3, 2018, at 9:27 AM, Daniel Fuchs  wrote:
> 
> Looks good to me, though I don't understand the change
> at line 154:
> 
> The comment says:
> 
> 152 // skip(n) returns negative value: IOE
> 
> but if you pass -1 - you're no longer calling skip(n)?

Re: RFR: 8214712: Archive Attributes$Name.KNOWN_NAMES

[Stuart Marks]

On 12/4/18 12:32 AM, Claes Redestad wrote:
These non-standard and tool/library specific names appear in the manifest of a 
great many jar files, for example on maven central, and including the most 
common non-standard manifest attributes significantly reduce allocation churn 
reading such manifests. Especially true for ones that were oft repeated within 
the same manifest, e.g.,"Name" and "SHA1-Digest" for signed entries.


Was it a crucial optimization? Probably not, a few percent on some startup 
applications.


Ideals vs trade-offs... Hard-coding a bunch of arbitrarily chosen external 
values isn't exactly pretty, sure, but I do think the JDK should try to optimize 
for typical behavior, and the fact that many (most?) libraries on maven central 
have OSGi attributes ("Bundle-*") in their manifests is one such typical 
behavior, and this optimization was very low cost and net beneficial on most. 
With 8214712 the overhead cost is even less (almost zero in fact).


It might be worth adding a comment to say that's where these names come from. 
That is, they appear to occur frequently "in the wild" but otherwise have no 
special significance or meaning in the JDK.


Also, could addName() return a Map.Entry, and then you could just initialize 
KNOWN_NAMES using Map.ofEntries(), passing the whole list of entries from the 
addName() call? It might be preferable to building an intermediate HashMap.


s'marks

Re: RFR 8214794 : java.specification.version should be only the major version number

[Magnus Ihse Bursie]

Looks good to me, too. 

/Magnus

> 4 dec. 2018 kl. 20:34 skrev Mandy Chung :
> 
> The revised webrev looks okay.
> 
> Mandy
> 
>> On 12/4/18 11:32 AM, Roger Riggs wrote:
>> Hi Mandy, Martin,
>> 
>> The new test is unnecessary, the case is covered by 
>> java/lang/System/Versions test
>> and uses the stronger comparison for the version numbers.
>> 
>> It would not detect the problem unless the version included more than the 
>> major version.
>> 
>> Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/
>> 
>> Thanks, Roger
>> 
>>> On 12/04/2018 01:41 PM, Mandy Chung wrote:
>>> 
>>> 
 On 12/4/18 8:16 AM, Roger Riggs wrote:
 Please review correctly setting the java.specification.version property
 with only the major version number.  A test is added to ensure the
 java spec version agrees with the major version.
 
 The symptoms are that jtreg would fail with a full version number.
 
 Webrev:
 http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/
 
>>> 
>>> Looks okay.   I agree with Martin to go with a stronger assertion without 
>>> converting into a number. test/jdk/java/lang/System/Versions.java looks 
>>> like also covering this test case.  At some point it'd be good to 
>>> consolidate these two tests.
>>> 
>>> Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are a 
>>> relevant group.   VERSION_SPECIFICATION can be moved to group with 
>>> VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.
>>> 
>>> Mandy
>> 
> 

Re: RFR(S)JDK-8214074: Ghash optimization using AVX instructions

[Anthony Scarpino]

I believe we have at least two approvals, Vladimir and me.  Unless there 
is something else to be done, I'll do a final sanity check and push the 
code wednesday or thursday.


Tony

On 12/4/18 5:18 AM, Dmitry Chuyko wrote:

Hello,

AArch64 aes test runs pass in -Dmode=GCM.

-Dmitry

On 12/2/18 3:37 AM, Vladimir Kozlov wrote:

I am fine with Hotspot changes.

But we need to verify changes on all platforms.
I see that aarch64 also supports it in addition to SPARC.

I will run compiler/codegen/aes/ test to make sure it pass on SPARC 
but we don't test aarch64.

I let you know testing results when they are done.

Thanks,
Vladimir


On 11/20/18 6:45 PM, Kamath, Smita wrote:

Hi Tony,

Thanks for your comments.

1)  This intrinsic is also used with solaris-sparc, has there been a 
sanity check by anyone to make sure this does not break the sparc 
intrinsic? It may work as the code is now since the sparc intrinsic 
will only use the first two longs in the subkeyHtbl.
Would it be possible to help with this sanity check?  I don't have 
the required set-up to test this scenario.


2) I have changed the code so that subkeyH corresponds to the first 
two entries of subkeyHtbl.  Please find the updated webrev link.


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

Webrev link: http://cr.openjdk.java.net/~svkamath/ghash/webrev01/

Thanks and Regards,
Smita



-Original Message-
From: Anthony Scarpino [mailto:anthony.scarp...@oracle.com]
Sent: Tuesday, November 20, 2018 2:05 PM
To: Kamath, Smita ; 'Vladimir Kozlov' 

Cc: Viswanathan, Sandhya ; 
core-libs-dev@openjdk.java.net; hotspot compiler 

Subject: Re: RFR(S)JDK-8214074: Ghash optimization using AVX 
instructions


On 11/19/18 12:50 PM, Kamath, Smita wrote:

Hi Vladimir,

I'd like to contribute an optimization for GHASH Algorithm using AVX
Instructions. I have tested this optimization on SKX x86_64 platform
and it shows ~20-30% performance improvement for larger message sizes
(for example 8k).

I, smita.kam...@intel.com  , Shay
Gueuron, (shay.gue...@intel.com )and
Regev Shemy (regev.sh...@intel.com ) are
contributors to this code.

Link to Bug: https://bugs.openjdk.java.net/browse/JDK-8214074

Link to webrev: http://cr.openjdk.java.net/~svkamath/ghash/webrev/

For testing the implementation, I have executed TestAESMain.java. I
have executed Jtreg tests and tested this code on 64 bit Windows and
Linux platforms.

Please review and let me know if you have any comments.

Thanks and Regards,

Smita



Hi,

This intrinsic is also used with solaris-sparc, has there been a 
sanity check by anyone to make sure this does not break the sparc 
intrinsic?
It may work as the code is now since the sparc intrinsic will only 
use the first two longs in the subkeyHtbl.


In that same idea, have you consider combining the subkeyH to be the 
first two of subkeyHtbl for the non-avx operations?  It would 
eliminate an extra two longs per instance.


Tony

Re: RFR (JDK 12/java.base) 8213325: (props) Properties.loadFromXML does not fully comply with the spec

[Joe Wang]

Thanks Naoto for the fast review!

Best regards,
Joe

On 12/4/18, 1:53 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Those changes in the resource bundles look fine to me.

Naoto

On 12/4/18 1:45 PM, Joe Wang wrote:

Hi Naoto and all,

Could you please take a look at the changes to the resource files for 
tests of JDK-8172365 & JDK-8210408?


Current webrevs:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v04/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/

Thanks,
Joe

On 11/16/18, 2:27 PM, Joe Wang wrote:

Hi Daniel,

On 11/16/18, 10:32 AM, Daniel Fuchs wrote:

Hi Joe,

1. It looks as if the parser will not complain if the root element
   is seen twice - and it looks as if it is a supported feature
   since the previous implementation also had a counter.

   Should this be tested?
   If this is a feature and multiple roots are supported
   then the new impl might have an issue with allowing only
   one comment (maybe).


Removed "properties" from the ALLOWED_ELEMENTS so that it will 
report an error on properties inside properties, and added a test 
(IllegalElement.xml).


2. The rootElm qName is now set at two locations. Is the overridden
   method below still needed? Could it have been used instead of
   introducing a new startDTD method?

 212 @Override
 213 public void notationDecl(String name, String publicId, 
String systemId) throws SAXException {

 214 rootElm = name;
 215 }


Removed the method override. startDTD is the correct place to 
accomplish this.


Updated webrev:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/

Best regards,
Joe



best regards,

-- daniel

On 15/11/2018 20:26, Joe Wang wrote:

Hi Daniel,

I deleted the endDTD method. It could have been used to signal the 
end of DTD parsing and therefore serve as a validation point. 
However, the parser would have thrown Exceptions if a DTD parsing 
wasn't completed properly, that seems to make an endDTD check 
unnecessary, at least as the current parser impl goes.


The small footprint parser has its problems, for example, not 
providing specific error messages when things go wrong. But since 
this is a Properties usage only, it serves the purpose well enough.


Updated webrev:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v01/

Best regards,
Joe

On 11/14/18, 2:32 AM, Daniel Fuchs wrote:

Hi Joe,

I do not see where the new DTDHandler::endDTD methos is called.
Is that an oversight or is there more magic?

best regards,

-- daniel

On 14/11/2018 05:18, Joe Wang wrote:

Hi,

Please review a patch to bring the small footprint parser for 
java.util.Properties compliant with the specification.


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

Thanks,
Joe

Re: RFR (JDK 12/java.base) 8213325: (props) Properties.loadFromXML does not fully comply with the spec

[naoto . sato]

Hi Joe,

Those changes in the resource bundles look fine to me.

Naoto

On 12/4/18 1:45 PM, Joe Wang wrote:

Hi Naoto and all,

Could you please take a look at the changes to the resource files for 
tests of JDK-8172365 & JDK-8210408?


Current webrevs:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v04/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/

Thanks,
Joe

On 11/16/18, 2:27 PM, Joe Wang wrote:

Hi Daniel,

On 11/16/18, 10:32 AM, Daniel Fuchs wrote:

Hi Joe,

1. It looks as if the parser will not complain if the root element
   is seen twice - and it looks as if it is a supported feature
   since the previous implementation also had a counter.

   Should this be tested?
   If this is a feature and multiple roots are supported
   then the new impl might have an issue with allowing only
   one comment (maybe).


Removed "properties" from the ALLOWED_ELEMENTS so that it will report 
an error on properties inside properties, and added a test 
(IllegalElement.xml).


2. The rootElm qName is now set at two locations. Is the overridden
   method below still needed? Could it have been used instead of
   introducing a new startDTD method?

 212 @Override
 213 public void notationDecl(String name, String publicId, 
String systemId) throws SAXException {

 214 rootElm = name;
 215 }


Removed the method override. startDTD is the correct place to 
accomplish this.


Updated webrev:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/

Best regards,
Joe



best regards,

-- daniel

On 15/11/2018 20:26, Joe Wang wrote:

Hi Daniel,

I deleted the endDTD method. It could have been used to signal the 
end of DTD parsing and therefore serve as a validation point. 
However, the parser would have thrown Exceptions if a DTD parsing 
wasn't completed properly, that seems to make an endDTD check 
unnecessary, at least as the current parser impl goes.


The small footprint parser has its problems, for example, not 
providing specific error messages when things go wrong. But since 
this is a Properties usage only, it serves the purpose well enough.


Updated webrev:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v01/

Best regards,
Joe

On 11/14/18, 2:32 AM, Daniel Fuchs wrote:

Hi Joe,

I do not see where the new DTDHandler::endDTD methos is called.
Is that an oversight or is there more magic?

best regards,

-- daniel

On 14/11/2018 05:18, Joe Wang wrote:

Hi,

Please review a patch to bring the small footprint parser for 
java.util.Properties compliant with the specification.


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

Thanks,
Joe

Re: RFR (JDK 12/java.base) 8213325: (props) Properties.loadFromXML does not fully comply with the spec

[Joe Wang]

Hi Naoto and all,

Could you please take a look at the changes to the resource files for 
tests of JDK-8172365 & JDK-8210408?


Current webrevs:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v04/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/

Thanks,
Joe

On 11/16/18, 2:27 PM, Joe Wang wrote:

Hi Daniel,

On 11/16/18, 10:32 AM, Daniel Fuchs wrote:

Hi Joe,

1. It looks as if the parser will not complain if the root element
   is seen twice - and it looks as if it is a supported feature
   since the previous implementation also had a counter.

   Should this be tested?
   If this is a feature and multiple roots are supported
   then the new impl might have an issue with allowing only
   one comment (maybe).


Removed "properties" from the ALLOWED_ELEMENTS so that it will report 
an error on properties inside properties, and added a test 
(IllegalElement.xml).


2. The rootElm qName is now set at two locations. Is the overridden
   method below still needed? Could it have been used instead of
   introducing a new startDTD method?

 212 @Override
 213 public void notationDecl(String name, String publicId, 
String systemId) throws SAXException {

 214 rootElm = name;
 215 }


Removed the method override. startDTD is the correct place to 
accomplish this.


Updated webrev:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/

Best regards,
Joe



best regards,

-- daniel

On 15/11/2018 20:26, Joe Wang wrote:

Hi Daniel,

I deleted the endDTD method. It could have been used to signal the 
end of DTD parsing and therefore serve as a validation point. 
However, the parser would have thrown Exceptions if a DTD parsing 
wasn't completed properly, that seems to make an endDTD check 
unnecessary, at least as the current parser impl goes.


The small footprint parser has its problems, for example, not 
providing specific error messages when things go wrong. But since 
this is a Properties usage only, it serves the purpose well enough.


Updated webrev:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/

Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v01/

Best regards,
Joe

On 11/14/18, 2:32 AM, Daniel Fuchs wrote:

Hi Joe,

I do not see where the new DTDHandler::endDTD methos is called.
Is that an oversight or is there more magic?

best regards,

-- daniel

On 14/11/2018 05:18, Joe Wang wrote:

Hi,

Please review a patch to bring the small footprint parser for 
java.util.Properties compliant with the specification.


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

Thanks,
Joe

Re: RFR 8214794 : java.specification.version should be only the major version number

[Martin Buchholz]

>
> LGTM

Re: RFR 8214794 : java.specification.version should be only the major version number

[Erik Joelsson]

Looks good.

/Erik

On 2018-12-04 11:32, Roger Riggs wrote:

Hi Mandy, Martin,

The new test is unnecessary, the case is covered by 
java/lang/System/Versions test

and uses the stronger comparison for the version numbers.

It would not detect the problem unless the version included more than 
the major version.


Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/

Thanks, Roger

On 12/04/2018 01:41 PM, Mandy Chung wrote:



On 12/4/18 8:16 AM, Roger Riggs wrote:

Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/



Looks okay.   I agree with Martin to go with a stronger assertion 
without converting into a number. 
test/jdk/java/lang/System/Versions.java looks like also covering this 
test case.  At some point it'd be good to consolidate these two tests.


Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc 
are a relevant group.   VERSION_SPECIFICATION can be moved to group 
with VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.


Mandy

Re: RFR 8214794 : java.specification.version should be only the major version number

[Roger Riggs]

Hi Mandy, Martin,

The new test is unnecessary, the case is covered by 
java/lang/System/Versions test

and uses the stronger comparison for the version numbers.

It would not detect the problem unless the version included more than 
the major version.


Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/

Thanks, Roger

On 12/04/2018 01:41 PM, Mandy Chung wrote:



On 12/4/18 8:16 AM, Roger Riggs wrote:

Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/



Looks okay.   I agree with Martin to go with a stronger assertion 
without converting into a number. 
test/jdk/java/lang/System/Versions.java looks like also covering this 
test case.  At some point it'd be good to consolidate these two tests.


Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc 
are a relevant group.   VERSION_SPECIFICATION can be moved to group 
with VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.


Mandy

Re: RFR 8214794 : java.specification.version should be only the major version number

[Mandy Chung]

The revised webrev looks okay.

Mandy

On 12/4/18 11:32 AM, Roger Riggs wrote:

Hi Mandy, Martin,

The new test is unnecessary, the case is covered by 
java/lang/System/Versions test

and uses the stronger comparison for the version numbers.

It would not detect the problem unless the version included more than 
the major version.


Webrev: http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-3/

Thanks, Roger

On 12/04/2018 01:41 PM, Mandy Chung wrote:



On 12/4/18 8:16 AM, Roger Riggs wrote:

Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/



Looks okay.   I agree with Martin to go with a stronger assertion 
without converting into a number. 
test/jdk/java/lang/System/Versions.java looks like also covering this 
test case.  At some point it'd be good to consolidate these two tests.


Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc 
are a relevant group.   VERSION_SPECIFICATION can be moved to group 
with VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.


Mandy

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stephen Colebourne]

Thank you for following up - we all know the core-libs team has a busy
workload, and naming topics are always tricky.

I'm personally unconvinced that `transform()` is the best name out there.
While transform is OK for String and maybe Optional, it is poor for List
and Stream. In addition, I'm still not overly convinced that this is an
appropriate stylistic direction for Java to go more generally (though I
fully agree with point #2 on language change below).

However, since the core-libs team is clearly indicating a desire to close
the topic, I have no personal intention of objecting further..
thanks
Stephen


On Tue, 4 Dec 2018 at 18:38, Stuart Marks  wrote:

> Hi everybody,
>
> I've finally caught up with all of this. I see that several people are
> surprised
> by the way this has turned out. As the first-named reviewer on this
> changeset, I
> felt I should try to figure out what happened.
>
> While this API point stands on its own, this is really part of Jim's RSL
> work
> [1] which includes several API additions to String, and which will likely
> have a
> significant effect on how String literals are used in Java code.
>
> Jim started code review [2] and CSR review [3] for this proposal back in
> September. There was a gap of several weeks, followed by a revised
> proposal on
> 12 Nov [4][5] that changed the method's name from "transform" to "map".
>
> There was further discussion over the next couple days; in particular
> there were
> some objections to the method name "map". On 26 Nov Jim pushed the
> changeset
> with the name changed back to "transform".
>
>  From reading just the messages on the mailing list, I can certainly see
> why
> people were surprised. It looked like there was an issue open for
> discussion,
> yet Jim went ahead and pushed the revised change anyway. What happened?
>
> Given that the primary open issue was about naming, and such mailing list
> discussions can go on for an arbitrarily long time, Jim asked Brian
> off-list for
> a decision on this. The results were:
>
> 1) the name for this method on String should be "transform";
>
> 2) adding library methods is a reasonable way for the platform to provide
> this
> capability (as opposed to various language enhancements that might be
> contemplated); and
>
> 3) the intent is to colonize the name "transform" for this operation on
> this and
> other classes, such as List, Optional, etc.  (It may well be the case that
> there
> is no name that is a good fit for both Stream and for everything else,
> given
> Stream's highly stylized API; we can consider the tradeoffs between
> consistency
> and clarity when we get to that particular one.)
>
> The primary thing that went wrong here was that Brian and Jim neglected to
> circle back to the list to record this decision. This was an oversight.
>
> There could and should have been better communication about this. Brian,
> Jim, or
> I should have documented the results of this decision and followed up on
> the
> mailing list. None of us did. Sorry about that.
>
> What else should be done here?
>
> One thing is that we (I) can make a better effort to keep up on emails,
> though
> the volume -- on a wide variety of topics -- is significant.
>
> Another point is that issues that are raised on the mailing list are often
> discussed and resolved off the mailing list. While we make significant and
> ongoing efforts to write up relevant offline discussions for public
> consumption
> (see, for example, see the recent writeup on nullable values [6]),
> sometimes
> things will fall through the cracks.
>
> Finally, we should also record that this particular decision sets a
> precedent
> for the use of the name "transform" for methods that do similar things on
> other
> classes. To this end, I've updated this bug with a note about "transform":
>
>JDK-8140283 [7] add method to Stream that facilitates fluent chaining
> of
> other methods
>
> and I've also filed the following RFE:
>
>JDK-8214753 [8] (opt) add Optional::transform, allowing an arbitrary
> operation on an Optional
>
> s'marks
>
> [1] http://openjdk.java.net/jeps/326
>
> [2]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055532.html
>
> [3]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055533.html
>
> [4]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056574.html
>
> [5]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html
>
> [6]
>
> http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2018-November/000784.html
>
> [7] https://bugs.openjdk.java.net/browse/JDK-8140283
>
> [8] https://bugs.openjdk.java.net/browse/JDK-8214753
>
>

Re: RFR 8214794 : java.specification.version should be only the major version number

[Brian Burkhalter]

Hi Roger,

Looks fine.

Brian

> On Dec 4, 2018, at 8:23 AM, Roger Riggs  wrote:
> 
> Including build-dev for the change to GensrcMisc.gmk.
> 
> thx.
> 
> On 12/04/2018 11:16 AM, Roger Riggs wrote:
>> Please review correctly setting the java.specification.version property
>> with only the major version number.  A test is added to ensure the
>> java spec version agrees with the major version.
>> 
>> The symptoms are that jtreg would fail with a full version number.
>> 
>> Webrev:
>>   http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2

Re: [12] RFR: 8214770: java/time/test/java/time/format/TestNonIsoFormatter.java failed in non-english locales.

[Brian Burkhalter]

+1

Brian

> On Dec 4, 2018, at 10:59 AM, Lance Andersen  wrote:
> 
> Looks OK Naoto

Re: [12] RFR: 8214770: java/time/test/java/time/format/TestNonIsoFormatter.java failed in non-english locales.

[Lance Andersen]

Looks OK Naoto
> On Dec 4, 2018, at 1:52 PM, naoto.s...@oracle.com wrote:
> 
> Hello,
> 
> Please review this simple fix to the following issue:
> 
> https://bugs.openjdk.java.net/browse/JDK-8214770
> 
> The proposed fix is located at:
> 
> http://cr.openjdk.java.net/~naoto/8214770/webrev.00/
> 
> It is simply specifying Locale.ROOT to expect the locale independent era 
> names.
> 
> Naoto

 
  

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

[12] RFR: 8214770: java/time/test/java/time/format/TestNonIsoFormatter.java failed in non-english locales.

[naoto . sato]

Hello,

Please review this simple fix to the following issue:

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

The proposed fix is located at:

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

It is simply specifying Locale.ROOT to expect the locale independent era 
names.


Naoto

Re: RFR 8214794 : java.specification.version should be only the major version number

[Mandy Chung]

On 12/4/18 8:16 AM, Roger Riggs wrote:

Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/



Looks okay.   I agree with Martin to go with a stronger assertion 
without converting into a number. 
test/jdk/java/lang/System/Versions.java looks like also covering this 
test case.  At some point it'd be good to consolidate these two tests.


Nit:  in GensrcMisc.gmk, I think VERSION_NUMBER and VERSION_PRE etc are 
a relevant group.   VERSION_SPECIFICATION can be moved to group with 
VERSION_CLASSFILE_MAJOR and MINOR.  Magnus may have an opinion.


Mandy

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stuart Marks]

Hi everybody,

I've finally caught up with all of this. I see that several people are surprised 
by the way this has turned out. As the first-named reviewer on this changeset, I 
felt I should try to figure out what happened.


While this API point stands on its own, this is really part of Jim's RSL work 
[1] which includes several API additions to String, and which will likely have a 
significant effect on how String literals are used in Java code.


Jim started code review [2] and CSR review [3] for this proposal back in 
September. There was a gap of several weeks, followed by a revised proposal on 
12 Nov [4][5] that changed the method's name from "transform" to "map".


There was further discussion over the next couple days; in particular there were 
some objections to the method name "map". On 26 Nov Jim pushed the changeset 
with the name changed back to "transform".


From reading just the messages on the mailing list, I can certainly see why 
people were surprised. It looked like there was an issue open for discussion, 
yet Jim went ahead and pushed the revised change anyway. What happened?


Given that the primary open issue was about naming, and such mailing list 
discussions can go on for an arbitrarily long time, Jim asked Brian off-list for 
a decision on this. The results were:


1) the name for this method on String should be "transform";

2) adding library methods is a reasonable way for the platform to provide this 
capability (as opposed to various language enhancements that might be 
contemplated); and


3) the intent is to colonize the name "transform" for this operation on this and 
other classes, such as List, Optional, etc.  (It may well be the case that there 
is no name that is a good fit for both Stream and for everything else, given 
Stream's highly stylized API; we can consider the tradeoffs between consistency 
and clarity when we get to that particular one.)


The primary thing that went wrong here was that Brian and Jim neglected to 
circle back to the list to record this decision. This was an oversight.


There could and should have been better communication about this. Brian, Jim, or 
I should have documented the results of this decision and followed up on the 
mailing list. None of us did. Sorry about that.


What else should be done here?

One thing is that we (I) can make a better effort to keep up on emails, though 
the volume -- on a wide variety of topics -- is significant.


Another point is that issues that are raised on the mailing list are often 
discussed and resolved off the mailing list. While we make significant and 
ongoing efforts to write up relevant offline discussions for public consumption 
(see, for example, see the recent writeup on nullable values [6]), sometimes 
things will fall through the cracks.


Finally, we should also record that this particular decision sets a precedent 
for the use of the name "transform" for methods that do similar things on other 
classes. To this end, I've updated this bug with a note about "transform":


  JDK-8140283 [7] add method to Stream that facilitates fluent chaining of 
other methods


and I've also filed the following RFE:

  JDK-8214753 [8] (opt) add Optional::transform, allowing an arbitrary 
operation on an Optional


s'marks

[1] http://openjdk.java.net/jeps/326

[2] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055532.html

[3] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055533.html

[4] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056574.html

[5] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html

[6] 
http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2018-November/000784.html


[7] https://bugs.openjdk.java.net/browse/JDK-8140283

[8] https://bugs.openjdk.java.net/browse/JDK-8214753

Re: RFR 8177552: Compact Number Formatting support

[Roger Riggs]

Hi Nishit,

Thanks for the update.  Looks fine.
I have no more comments.

Roger

On 12/04/2018 10:50 AM, Nishit Jain wrote:

Thanks Roger,

Updated Webrev: 
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.06/


Regards,
Nishit Jain
On 04-12-2018 00:58, Roger Riggs wrote:

Hi Nishit,

Thanks for the updates and cleanup.

CompactNumberFormat.java:

- 827: To locate a single character use:
    if (pattern.indexOf(QUOTE) < 0) { ... }

OK.


- 1488:  Since infinite returns do not depend on any of the code 
after line 1454,

   the 1488- 1494 could be moved to 1454. (It is correct where it is).
The computeParseMultiplier decides whether parse string is positive or 
negative based on the prefix and suffix, so the 
status[STATUS_POSITIVE] can be also be used to return positive or 
negative infinity.

ok


Other minor changes in parse():

- Taken out "int numPosition" (line 1444 in webrev.05) and used 
earlier defined variable "position" instead, as "position" previous 
value is not used after that statement.
- Moved the variable "Number multiplier;" (line 1447 in webrev.05) to 
the place where it is assigned the value.

- Moved "Number cnfResult;" (line 1496 in webrev.05) inside else block.



 - in API design, I would have put the position argument immediately 
after text since the position
   is closely related to the text argument (in matchAffix and 
matchPrefixAndSuffix methods).

   Its probably not worth the change in these private methods.
Yes, it is better to move it next to "text". Updated both "matchAffix" 
and "matchPrefixAndSuffix" methods.


Regards,
Nishit Jain


comments below...

On 12/03/2018 07:22 AM, Nishit Jain wrote:

Thanks Roger,

Updated the webrev based on thebelow suggested changes and some 
clean up.


http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/

On 01-12-2018 01:03, Roger Riggs wrote:

Hi Nishit,

Some comments, a couple of possible bugs and several performance 
related issues
could be deferred. Both formatting and parsing seem to be quite 
heavyweight

due to looping and combinatorics.


CompactNumberFormat.java


661, 727, 1507: Is there a reason to convert the divisor to a 
string and then re-parse it to create a new BigDecimal?
    (IntelliJ observes that divide is called without a rounding 
argument.)
    Given that the divisors are all powers of 10 and the digit 
list is a base 10 set of digits
    it seems more efficient to just move the decimal point then 
to do the math.
    BTW, the divisor.toString() is preferred over concat with 
"".  (looks like a hack).


    It would be more efficient to write two methods that would 
pass the Number
    and return a BigInteger or BigDecimal by dispatching on the 
concrete type and

    using the appropriate constructor.
Changed concatenation with toString() and added a rounding 
parameter, but not getting the benefit of having two methods and 
returning respective concrete type using constructors.

I didn't get the point of having two methods. Can you please elaborate?
The would the same function but different return types (BigInteger vs 
BigDecimal).

The code is ok as is.


781:  @param prefix - the parameter name is suffix

804: move the int start = inside the if.

826:  expandAffix can be more efficient if tests the pattern for 
the presence of QUOTE
    and returns the pattern if the QUOTE is not present.  That 
would be the most common case.


914: Reduce the number of compares by reordering to:
    if number > currentValue; multiply and continue
    if number < currentValue break;
    else ==; assign matched index and break;

    In the normal case, there will be only one compare in the 
loop until it is to exit.


1109:  IntelliJ observes that within the case QUOTE, the if (ch == 
QUOTE) is always true

  so the if is redundant.

OK.


1205:  It looks odd to prepend two characters '- to the prefix. Is 
the single quote correct?

  Where's the closing quote if so.
It is to specify that the minus sign is a special character, which 
needs to be replaced with its localized equivalent at the time of 
formatting.


Internally, there is a difference between a "minus sign prefix with 
a single quote" and a "minus sign" (it depends on how user specify 
the pattern), because the later one is considered as a literal and 
should be used exactly same in the formatted output, but the one 
prefixed with a single quote is replaced with its localized symbol 
using DecimalFormatSymbols.getMinusSign().

thanks for the explanation.


1394: matchedPosPrefix.length() is compared to 
negativePrefix.length().
  That's an unusual mixing of positive and negative! Please 
re-check.

Yes, it was a mistake.


1363:  Can there be an early exit from this loop if one of the 
prefixes is identified?
  The pattern of comparing for a prefix/suffix and the length 
might warrant
  creating a private method to reduce the repetitive parts of 
the code.
I had 

Re: RFR 8171426 : java/lang/ProcessBuilder/Basic.java failed with Stream closed

[Brian Burkhalter]

Hi Roger,

> On Dec 4, 2018, at 8:58 AM, Roger Riggs  wrote:
> 
> Thanks for the reviews.

You’re welcome.

> I simplify the description, the full details are in the bug report 
> and are more complete.
> 
> Updated webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-basic-8171426-1/ 
> 
Looks fine.

Thanks,

Brian

Re: RFR 8171426 : java/lang/ProcessBuilder/Basic.java failed with Stream closed

[James Laskey]

I wasn’t as fussy about the original comment but this one is also fine. 

Sent from my iPhone

> On Dec 4, 2018, at 12:58 PM, Roger Riggs  wrote:
> 
> Hi Brian, Jim,
> 
> Thanks for the reviews.
> 
> I simplify the description, the full details are in the bug report
> and are more complete.
> 
> Updated webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-basic-8171426-1/
> 
> Thanks, Roger
> 
> 
>> On 12/03/2018 04:49 PM, Brian Burkhalter wrote:
>> Hi Roger,
>> 
>> L2:copyright year, of course
>> L2119: The verbiage seems a little obtuse.
>> 
>> Otherwise, +1.
>> 
>> Thanks,
>> 
>> Brian
>> 
>>> On 11/27/2018 02:04 PM, Roger Riggs wrote:
>>> Please review a test update to address an intermittent test failure.
>>> The test is modified to accept the current implementation behavior of the 
>>> input streams
>>> from processes that may under race conditions with close throw IOE("Stream 
>>> closed").
>>> 
>>> Details and analysis in the Jira:
>>> https://bugs.openjdk.java.net/browse/JDK-8171426
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~rriggs/webrev-basic-8171426/ 
>>> 
>>> 
>>> Thanks, Roger
>> 
> 

Re: RFR 8171426 : java/lang/ProcessBuilder/Basic.java failed with Stream closed

[Roger Riggs]

Hi Brian, Jim,

Thanks for the reviews.

I simplify the description, the full details are in the bug report
and are more complete.

Updated webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-basic-8171426-1/

Thanks, Roger


On 12/03/2018 04:49 PM, Brian Burkhalter wrote:

Hi Roger,

L2:copyright year, of course
L2119: The verbiage seems a little obtuse.

Otherwise, +1.

Thanks,

Brian

On 11/27/2018 02:04 PM, Roger Riggs wrote:

Please review a test update to address an intermittent test failure.
The test is modified to accept the current implementation behavior of 
the input streams
from processes that may under race conditions with close throw 
IOE("Stream closed").


Details and analysis in the Jira:
https://bugs.openjdk.java.net/browse/JDK-8171426

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-basic-8171426/ 



Thanks, Roger

Re: RFR 8214794 : java.specification.version should be only the major version number

[Martin Buchholz]

>
> I would make a stronger assertion, that Runtime.version().feature()
> converted to a String is equal to specVersion, catching bogus specVersions
> like "+011"

Re: RFR 8214794 : java.specification.version should be only the major version number

[Roger Riggs]

Including build-dev for the change to GensrcMisc.gmk.

thx.

On 12/04/2018 11:16 AM, Roger Riggs wrote:

Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/

Thanks, Roger

RFR 8214794 : java.specification.version should be only the major version number

[Roger Riggs]

Please review correctly setting the java.specification.version property
with only the major version number.  A test is added to ensure the
java spec version agrees with the major version.

The symptoms are that jtreg would fail with a full version number.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-spec-ver-8214700-2/

Thanks, Roger

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Roger Riggs]

Hi Ichiroh,

Looks fine.

I can sponsor that for you.  Tomorrow, though, to allow time for any 
other comments.


Thanks, Roger


On 12/04/2018 10:21 AM, Ichiroh Takiguchi wrote:

Hello Roger.

Thank you for your suggestion.

Could you review the fix again ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.04/

Thanks,
Ichiroh Takiguchi

On 2018-12-04 23:36, Roger Riggs wrote:

Hi Ichiroh,

On 12/03/2018 10:30 PM, Ichiroh Takiguchi wrote:

Hello Roger.

Thank you for your suggestion.


src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

    I think this is no longer needed since it only has imports.
    By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of 
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers 
it.

It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext 
package

I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.

I understand, it is because IBM33722 may or may not be in the same
package as SimpleEUCEncoder.
It is SimpleEUCEncoder that may be in a different package, not IBM33722.



TestIBMBugs:
  - Please use a style consistent with other methods in the class.
    In this case spaces are needed around "{" and "}, and a space
after "," comma.

I'll changed.

226-227:  add a space before "{" to have the same style as line 210.



  - The new method bug8212794, includes a test for x-IBM33722.
    Is that needed since there does not appear to be a change for 
33722?

Yes, it's no need.

Could you review the fix again ?
Bug:    https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/


Thanks, looks fine.

Regards, Roger



Thanks,
Ichiroh Takiguchi

On 2018-12-04 05:50, Roger Riggs wrote:

Hi Ichiroh,

src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

    I think this is no longer needed since it only has imports.
    By the way, the style is to import each specific class and
avoid wild card imports.

TestIBMBugs:
  - Please use a style consistent with other methods in the class.
    In this case spaces are needed around "{" and "}, and a space
after "," comma.

  - The new method bug8212794, includes a test for x-IBM33722.
    Is that needed since there does not appear to be a change for 
33722?


Regards, Roger


On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:



On 2018-11-30 10:49, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix again ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review 
it too.


/Magnus


I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)

On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.

Without fix
$ jshell
|  Welcome to JShell -- Version 12-ea
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964

jshell> /exit
|  Goodbye
$
==

With fix
==
$ jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

On AIX platform
IBM964 is moved to java.base module from jdk.charset module.

==
$ LANG=zh_TW jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

I'd like to obtain a sponsor for this issue.

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-29 22:39, Ichiroh Takiguchi wrote:

Hello Alan & Magnus.

Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.

I think I should not put 2 kind of modification.

For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)

I'll submit code review again.

Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix

I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java

Re: RFR 8177552: Compact Number Formatting support

[Nishit Jain]

Thanks Roger,

Updated Webrev: 
http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.06/


Regards,
Nishit Jain
On 04-12-2018 00:58, Roger Riggs wrote:

Hi Nishit,

Thanks for the updates and cleanup.

CompactNumberFormat.java:

- 827: To locate a single character use:
    if (pattern.indexOf(QUOTE) < 0) { ... }

OK.


- 1488:  Since infinite returns do not depend on any of the code after 
line 1454,

   the 1488- 1494 could be moved to 1454. (It is correct where it is).
The computeParseMultiplier decides whether parse string is positive or 
negative based on the prefix and suffix, so the status[STATUS_POSITIVE] 
can be also be used to return positive or negative infinity.


Other minor changes in parse():

- Taken out "int numPosition" (line 1444 in webrev.05) and used earlier 
defined variable "position" instead, as "position" previous value is not 
used after that statement.
- Moved the variable "Number multiplier;" (line 1447 in webrev.05) to 
the place where it is assigned the value.

- Moved "Number cnfResult;" (line 1496 in webrev.05) inside else block.



 - in API design, I would have put the position argument immediately 
after text since the position
   is closely related to the text argument (in matchAffix and 
matchPrefixAndSuffix methods).

   Its probably not worth the change in these private methods.
Yes, it is better to move it next to "text". Updated both "matchAffix" 
and "matchPrefixAndSuffix" methods.


Regards,
Nishit Jain


comments below...

On 12/03/2018 07:22 AM, Nishit Jain wrote:

Thanks Roger,

Updated the webrev based on thebelow suggested changes and some clean 
up.


http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.05/

On 01-12-2018 01:03, Roger Riggs wrote:

Hi Nishit,

Some comments, a couple of possible bugs and several performance 
related issues
could be deferred. Both formatting and parsing seem to be quite 
heavyweight

due to looping and combinatorics.


CompactNumberFormat.java


661, 727, 1507: Is there a reason to convert the divisor to a string 
and then re-parse it to create a new BigDecimal?
    (IntelliJ observes that divide is called without a rounding 
argument.)
    Given that the divisors are all powers of 10 and the digit 
list is a base 10 set of digits
    it seems more efficient to just move the decimal point then 
to do the math.
    BTW, the divisor.toString() is preferred over concat with 
"".  (looks like a hack).


    It would be more efficient to write two methods that would 
pass the Number
    and return a BigInteger or BigDecimal by dispatching on the 
concrete type and

    using the appropriate constructor.
Changed concatenation with toString() and added a rounding parameter, 
but not getting the benefit of having two methods and returning 
respective concrete type using constructors.

I didn't get the point of having two methods. Can you please elaborate?
The would the same function but different return types (BigInteger vs 
BigDecimal).

The code is ok as is.


781:  @param prefix - the parameter name is suffix

804: move the int start = inside the if.

826:  expandAffix can be more efficient if tests the pattern for the 
presence of QUOTE
    and returns the pattern if the QUOTE is not present. That 
would be the most common case.


914: Reduce the number of compares by reordering to:
    if number > currentValue; multiply and continue
    if number < currentValue break;
    else ==; assign matched index and break;

    In the normal case, there will be only one compare in the 
loop until it is to exit.


1109:  IntelliJ observes that within the case QUOTE, the if (ch == 
QUOTE) is always true

  so the if is redundant.

OK.


1205:  It looks odd to prepend two characters '- to the prefix. Is 
the single quote correct?

  Where's the closing quote if so.
It is to specify that the minus sign is a special character, which 
needs to be replaced with its localized equivalent at the time of 
formatting.


Internally, there is a difference between a "minus sign prefix with a 
single quote" and a "minus sign" (it depends on how user specify the 
pattern), because the later one is considered as a literal and should 
be used exactly same in the formatted output, but the one prefixed 
with a single quote is replaced with its localized symbol using 
DecimalFormatSymbols.getMinusSign().

thanks for the explanation.



1394: matchedPosPrefix.length() is compared to negativePrefix.length().
  That's an unusual mixing of positive and negative! Please 
re-check.

Yes, it was a mistake.


1363:  Can there be an early exit from this loop if one of the 
prefixes is identified?
  The pattern of comparing for a prefix/suffix and the length 
might warrant
  creating a private method to reduce the repetitive parts of 
the code.
I had thought about it, but it was difficult unless the whole list of 
affixes are traversed, because there is always a chance to get longer 

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Ichiroh Takiguchi]

Hello Roger.

Thank you for your suggestion.

Could you review the fix again ?

Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.04/

Thanks,
Ichiroh Takiguchi

On 2018-12-04 23:36, Roger Riggs wrote:

Hi Ichiroh,

On 12/03/2018 10:30 PM, Ichiroh Takiguchi wrote:

Hello Roger.

Thank you for your suggestion.


src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

    I think this is no longer needed since it only has imports.
    By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of 
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers 
it.

It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext 
package

I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.

I understand, it is because IBM33722 may or may not be in the same
package as SimpleEUCEncoder.
It is SimpleEUCEncoder that may be in a different package, not 
IBM33722.



TestIBMBugs:
  - Please use a style consistent with other methods in the class.
    In this case spaces are needed around "{" and "}, and a space
after "," comma.

I'll changed.

226-227:  add a space before "{" to have the same style as line 210.



  - The new method bug8212794, includes a test for x-IBM33722.
    Is that needed since there does not appear to be a change for 
33722?

Yes, it's no need.

Could you review the fix again ?
Bug:    https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/


Thanks, looks fine.

Regards, Roger



Thanks,
Ichiroh Takiguchi

On 2018-12-04 05:50, Roger Riggs wrote:

Hi Ichiroh,

src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

    I think this is no longer needed since it only has imports.
    By the way, the style is to import each specific class and
avoid wild card imports.

TestIBMBugs:
  - Please use a style consistent with other methods in the class.
    In this case spaces are needed around "{" and "}, and a space
after "," comma.

  - The new method bug8212794, includes a test for x-IBM33722.
    Is that needed since there does not appear to be a change for 
33722?


Regards, Roger


On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:



On 2018-11-30 10:49, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix again ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review 
it too.


/Magnus


I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)

On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.

Without fix
$ jshell
|  Welcome to JShell -- Version 12-ea
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964

jshell> /exit
|  Goodbye
$
==

With fix
==
$ jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

On AIX platform
IBM964 is moved to java.base module from jdk.charset module.

==
$ LANG=zh_TW jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

I'd like to obtain a sponsor for this issue.

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-29 22:39, Ichiroh Takiguchi wrote:

Hello Alan & Magnus.

Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.

I think I should not put 2 kind of modification.

For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)

I'll submit code review again.

Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix

I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java

My build machine is not so fast, after test is done.
I'll post new code.

Thanks,
Ichiroh Takiguchi

On 2018-11-28 19:10, Magnus Ihse Bursie 

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Roger Riggs]

Hi Ichiroh,

On 12/03/2018 10:30 PM, Ichiroh Takiguchi wrote:

Hello Roger.

Thank you for your suggestion.


src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

    I think this is no longer needed since it only has imports.
    By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of 
SimpleEUCEncoder.java.template.

SimpleEUCEncoder.java.template has conversion loop and IBM964 refers it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext 
package

I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I understand, it is because IBM33722 may or may not be in the same 
package as SimpleEUCEncoder.

It is SimpleEUCEncoder that may be in a different package, not IBM33722.



TestIBMBugs:
  - Please use a style consistent with other methods in the class.
    In this case spaces are needed around "{" and "}, and a space
after "," comma.

I'll changed.

226-227:  add a space before "{" to have the same style as line 210.



  - The new method bug8212794, includes a test for x-IBM33722.
    Is that needed since there does not appear to be a change for 33722?

Yes, it's no need.

Could you review the fix again ?
Bug:    https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/


Thanks, looks fine.

Regards, Roger



Thanks,
Ichiroh Takiguchi

On 2018-12-04 05:50, Roger Riggs wrote:

Hi Ichiroh,

src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

    I think this is no longer needed since it only has imports.
    By the way, the style is to import each specific class and
avoid wild card imports.

TestIBMBugs:
  - Please use a style consistent with other methods in the class.
    In this case spaces are needed around "{" and "}, and a space
after "," comma.

  - The new method bug8212794, includes a test for x-IBM33722.
    Is that needed since there does not appear to be a change for 33722?

Regards, Roger


On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:



On 2018-11-30 10:49, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix again ?

Bug:    https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review 
it too.


/Magnus


I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)

On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.

Without fix
$ jshell
|  Welcome to JShell -- Version 12-ea
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964

jshell> /exit
|  Goodbye
$
==

With fix
==
$ jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

On AIX platform
IBM964 is moved to java.base module from jdk.charset module.

==
$ LANG=zh_TW jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

I'd like to obtain a sponsor for this issue.

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-29 22:39, Ichiroh Takiguchi wrote:

Hello Alan & Magnus.

Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.

I think I should not put 2 kind of modification.

For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)

I'll submit code review again.

Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix

I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java

My build machine is not so fast, after test is done.
I'll post new code.

Thanks,
Ichiroh Takiguchi

On 2018-11-28 19:10, Magnus Ihse Bursie wrote:

On 2018-11-28 10:36, Alan Bateman wrote:

On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of character 
sets in the build in general. :-( I'd really like to modernize 
it. I have a, slightly fuzzy, laundry list of things I 

Re: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Thomas Stüfe]

Thanks Roger!
On Tue, Dec 4, 2018 at 3:14 PM Roger Riggs  wrote:
>
> Hi Thomas,
>
> No change was intended.
>
> I'll file a bug and fix it shortly.
>
> Roger
>
>
> On 12/04/2018 08:07 AM, Thomas Stüfe wrote:
>
> Hi Roger,
>
> There is a difference now as to how java.specification.version is set.
>
> -PUTPROP(props, "java.specification.version",
> -VERSION_SPECIFICATION);
>
> +props.setProperty("java.specification.version", VERSION_NUMBER);
>
> Was that intended?
>
> It seems to cause errors in our JTreg tests because the jtreg runner
> is expecting only the full version number without dots.
>
> Cheers, Thomas
>
>
>
> On Thu, Nov 15, 2018 at 2:12 AM Roger Riggs  wrote:
>
> Hi Brent,
>
>
> On 11/14/2018 06:54 PM, Brent Christian wrote:
>
> Hi, Roger
>
> * I like Mandy's idea of not combining the cli/VM props and the
> well-known props into a single array.  Maybe we could then avoid
> copying between arrays (System.c L166).
>
> yes, it would avoid the copy.
>
> * The name "Raw" doesn't really speak to me.  It's OK as an inner
> class, though I wonder if everything could be done in SystemProps
> directly.
>
> Raw to make it clear that these are not system properties, just data to
> be used to
> create the system properties.
>
> In some other prototyping, there were quite a few other changes in
> SystemProps
> so keeping the primitive data separate was useful, possibly the class
> could be unloaded
> since its use-once code.
>
> A few other comments:
>
> java.base/share/native/libjava/System.c:
>
>  112  * The first FIXED_LENGTH entries are the platform defined
> property values, no names.
>  113  * The remaining array indices are alternating key/value pairs
>  114  * supplied by the VM including those defined on the command line
>  115  * using -Dkey=value that may override the platform defined value.
>
> Some of this would also be useful information SystemProps.java, maybe
> in the comment for Raw.initProps.  Or refer the reader to
> Java_jdk_internal_util_SystemProps_00024Raw_getRawProperties for a
> description of the layout of the array.  Or may become moot if we use
> two separate arrays.
>
> Agreed, a more complete description would be good in the declaration of
> the native method.
>
> --
>
> file.encoding used to be set from sprops->encoding on Mac, and
> sprops->sun_jnu_encoding otherwise:
>
>  382 #ifdef MACOSX
>  383 /*
>  384  * Since sun_jnu_encoding is now hard-coded to UTF-8 on
> Mac, we don't
>  385  * want to use it to overwrite file.encoding
>  386  */
>  387 PUTPROP(props, "file.encoding", sprops->encoding);
>  388 #else
>  389 PUTPROP(props, "file.encoding", sprops->sun_jnu_encoding);
>  390 #endif
>
> There is no longer an ifdef for this.  But I guess this is OK, as
> SystemProps.initProperties() will only put a value for "file.encoding"
> if there isn't one, and only uses sun.jnu.encoding if there is no
> file.encoding:
>
>   59 putIfAbsent(props, "file.encoding",
>   60 ((raw.propDefault(Raw._file_encoding_NDX) == null)
>   61 ? raw.propDefault(Raw._sun_jnu_encoding_NDX)
>   62 : raw.propDefault(Raw._file_encoding_NDX)));
>
> Yes, I spent some time ensuring that the result was the same.
> Thanks for confirming the logic.
>
> --
>
>  313 * Command line overrides with -D are copied above from
> JVM_getProperties.
>
> s/JVM_get/JVM_Get/
>
> right, fixed, Java naming conventions bleeding into native.
>
> java.base/share/classes/jdk/internal/util/SystemProps.java:
>
>  126 /**
>  127  * Puts the property if it is non-null
>  128  * @param props the Properties
>  129  * @param key the key
>  130  * @param value the value
>  131  */
>  132 private static void put(Properties props, String key, String
> value) {
>
> I notice that this method is only called once.
>
> Yes, at the moment, that is the only property that must not be
> overridden on the command line.
> There are others that probably should not be overridable but for
> compatibility...
>
> --
>
>  175 cmdProps.put(disp, dispValue);
> ...
>  186 cmdProps.put(fmt, fmtValue);
> ) {
>
> } else if (
> Aren't L175 & L186 just putting the property that is already there ?
>
> Oops,  that's a carryover from a different implementation that had
> separate maps.
> Those can be removed since an I18N property on the command is always
> retained
> only defined if the platform value is supplied and different than the base.
>
> Thanks for the review and suggestions
>
> Roger
>
> Thanks,
> -Brent
>
> On 11/13/18 7:59 AM, Roger Riggs wrote:
>
> Please review a re-implementation of the initialization of System
> properties
> moving some functions from native to Java.
>
> The upcalls from native to java for each property are replaced by a
> mechanism
> to gather the platform, VM and command line properties and return them
> from a single JNI call.  The 

Re: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Roger Riggs]

Hi Thomas,

No change was intended.

I'll file a bug and fix it shortly.

Roger


On 12/04/2018 08:07 AM, Thomas Stüfe wrote:

Hi Roger,

There is a difference now as to how java.specification.version is set.

-PUTPROP(props, "java.specification.version",
-VERSION_SPECIFICATION);

+props.setProperty("java.specification.version", VERSION_NUMBER);

Was that intended?

It seems to cause errors in our JTreg tests because the jtreg runner
is expecting only the full version number without dots.

Cheers, Thomas



On Thu, Nov 15, 2018 at 2:12 AM Roger Riggs  wrote:

Hi Brent,


On 11/14/2018 06:54 PM, Brent Christian wrote:

Hi, Roger

* I like Mandy's idea of not combining the cli/VM props and the
well-known props into a single array.  Maybe we could then avoid
copying between arrays (System.c L166).

yes, it would avoid the copy.

* The name "Raw" doesn't really speak to me.  It's OK as an inner
class, though I wonder if everything could be done in SystemProps
directly.

Raw to make it clear that these are not system properties, just data to
be used to
create the system properties.

In some other prototyping, there were quite a few other changes in
SystemProps
so keeping the primitive data separate was useful, possibly the class
could be unloaded
since its use-once code.

A few other comments:

java.base/share/native/libjava/System.c:

  112  * The first FIXED_LENGTH entries are the platform defined
property values, no names.
  113  * The remaining array indices are alternating key/value pairs
  114  * supplied by the VM including those defined on the command line
  115  * using -Dkey=value that may override the platform defined value.

Some of this would also be useful information SystemProps.java, maybe
in the comment for Raw.initProps.  Or refer the reader to
Java_jdk_internal_util_SystemProps_00024Raw_getRawProperties for a
description of the layout of the array.  Or may become moot if we use
two separate arrays.

Agreed, a more complete description would be good in the declaration of
the native method.

--

file.encoding used to be set from sprops->encoding on Mac, and
sprops->sun_jnu_encoding otherwise:

  382 #ifdef MACOSX
  383 /*
  384  * Since sun_jnu_encoding is now hard-coded to UTF-8 on
Mac, we don't
  385  * want to use it to overwrite file.encoding
  386  */
  387 PUTPROP(props, "file.encoding", sprops->encoding);
  388 #else
  389 PUTPROP(props, "file.encoding", sprops->sun_jnu_encoding);
  390 #endif

There is no longer an ifdef for this.  But I guess this is OK, as
SystemProps.initProperties() will only put a value for "file.encoding"
if there isn't one, and only uses sun.jnu.encoding if there is no
file.encoding:

   59 putIfAbsent(props, "file.encoding",
   60 ((raw.propDefault(Raw._file_encoding_NDX) == null)
   61 ? raw.propDefault(Raw._sun_jnu_encoding_NDX)
   62 : raw.propDefault(Raw._file_encoding_NDX)));


Yes, I spent some time ensuring that the result was the same.
Thanks for confirming the logic.

--

  313 * Command line overrides with -D are copied above from
JVM_getProperties.

s/JVM_get/JVM_Get/

right, fixed, Java naming conventions bleeding into native.


java.base/share/classes/jdk/internal/util/SystemProps.java:

  126 /**
  127  * Puts the property if it is non-null
  128  * @param props the Properties
  129  * @param key the key
  130  * @param value the value
  131  */
  132 private static void put(Properties props, String key, String
value) {

I notice that this method is only called once.

Yes, at the moment, that is the only property that must not be
overridden on the command line.
There are others that probably should not be overridable but for
compatibility...

--

  175 cmdProps.put(disp, dispValue);
...
  186 cmdProps.put(fmt, fmtValue);
) {

 } else if (
Aren't L175 & L186 just putting the property that is already there ?

Oops,  that's a carryover from a different implementation that had
separate maps.
Those can be removed since an I18N property on the command is always
retained
only defined if the platform value is supplied and different than the base.

Thanks for the review and suggestions

Roger


Thanks,
-Brent

On 11/13/18 7:59 AM, Roger Riggs wrote:

Please review a re-implementation of the initialization of System
properties
moving some functions from native to Java.

The upcalls from native to java for each property are replaced by a
mechanism
to gather the platform, VM and command line properties and return them
from a single JNI call.  The creation of the Properties instance and
applying
command line overrides is handled in Java instead of native.

The JVM_initProperties interface in Hotspot is replaced by
JVM_getProperties.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-props-only-raw/

Issue:

Re: RFR 8171426 : java/lang/ProcessBuilder/Basic.java failed with Stream closed

[Jim Laskey]

+1

> On Dec 3, 2018, at 5:45 PM, Roger Riggs  wrote:
> 
> Ping?
> 
> On 11/27/2018 02:04 PM, Roger Riggs wrote:
>> Please review a test update to address an intermittent test failure.
>> The test is modified to accept the current implementation behavior of the 
>> input streams
>> from processes that may under race conditions with close throw IOE("Stream 
>> closed").
>> 
>> Details and analysis in the Jira:
>>   https://bugs.openjdk.java.net/browse/JDK-8171426
>> 
>> Webrev:
>>   http://cr.openjdk.java.net/~rriggs/webrev-basic-8171426/
>> 
>> Thanks, Roger
>> 
> 

Re: RFR(S)JDK-8214074: Ghash optimization using AVX instructions

[Dmitry Chuyko]

Hello,

AArch64 aes test runs pass in -Dmode=GCM.

-Dmitry

On 12/2/18 3:37 AM, Vladimir Kozlov wrote:

I am fine with Hotspot changes.

But we need to verify changes on all platforms.
I see that aarch64 also supports it in addition to SPARC.

I will run compiler/codegen/aes/ test to make sure it pass on SPARC 
but we don't test aarch64.

I let you know testing results when they are done.

Thanks,
Vladimir


On 11/20/18 6:45 PM, Kamath, Smita wrote:

Hi Tony,

Thanks for your comments.

1)  This intrinsic is also used with solaris-sparc, has there been a 
sanity check by anyone to make sure this does not break the sparc 
intrinsic? It may work as the code is now since the sparc intrinsic 
will only use the first two longs in the subkeyHtbl.
Would it be possible to help with this sanity check?  I don't have 
the required set-up to test this scenario.


2) I have changed the code so that subkeyH corresponds to the first 
two entries of subkeyHtbl.  Please find the updated webrev link.


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

Webrev link: http://cr.openjdk.java.net/~svkamath/ghash/webrev01/

Thanks and Regards,
Smita



-Original Message-
From: Anthony Scarpino [mailto:anthony.scarp...@oracle.com]
Sent: Tuesday, November 20, 2018 2:05 PM
To: Kamath, Smita ; 'Vladimir Kozlov' 

Cc: Viswanathan, Sandhya ; 
core-libs-dev@openjdk.java.net; hotspot compiler 

Subject: Re: RFR(S)JDK-8214074: Ghash optimization using AVX 
instructions


On 11/19/18 12:50 PM, Kamath, Smita wrote:

Hi Vladimir,

I'd like to contribute an optimization for GHASH Algorithm using AVX
Instructions. I have tested this optimization on SKX x86_64 platform
and it shows ~20-30% performance improvement for larger message sizes
(for example 8k).

I, smita.kam...@intel.com  , Shay
Gueuron, (shay.gue...@intel.com )and
Regev Shemy (regev.sh...@intel.com ) are
contributors to this code.

Link to Bug: https://bugs.openjdk.java.net/browse/JDK-8214074

Link to webrev: http://cr.openjdk.java.net/~svkamath/ghash/webrev/

For testing the implementation, I have executed TestAESMain.java. I
have executed Jtreg tests and tested this code on 64 bit Windows and
Linux platforms.

Please review and let me know if you have any comments.

Thanks and Regards,

Smita



Hi,

This intrinsic is also used with solaris-sparc, has there been a 
sanity check by anyone to make sure this does not break the sparc 
intrinsic?
It may work as the code is now since the sparc intrinsic will only 
use the first two longs in the subkeyHtbl.


In that same idea, have you consider combining the subkeyH to be the 
first two of subkeyHtbl for the non-avx operations?  It would 
eliminate an extra two longs per instance.


Tony

Re: RFR 4947890 : Minimize JNI upcalls in system-properties initialization

[Thomas Stüfe]

Hi Roger,

There is a difference now as to how java.specification.version is set.

-PUTPROP(props, "java.specification.version",
-VERSION_SPECIFICATION);

+props.setProperty("java.specification.version", VERSION_NUMBER);

Was that intended?

It seems to cause errors in our JTreg tests because the jtreg runner
is expecting only the full version number without dots.

Cheers, Thomas



On Thu, Nov 15, 2018 at 2:12 AM Roger Riggs  wrote:
>
> Hi Brent,
>
>
> On 11/14/2018 06:54 PM, Brent Christian wrote:
> > Hi, Roger
> >
> > * I like Mandy's idea of not combining the cli/VM props and the
> > well-known props into a single array.  Maybe we could then avoid
> > copying between arrays (System.c L166).
> yes, it would avoid the copy.
> >
> > * The name "Raw" doesn't really speak to me.  It's OK as an inner
> > class, though I wonder if everything could be done in SystemProps
> > directly.
> Raw to make it clear that these are not system properties, just data to
> be used to
> create the system properties.
>
> In some other prototyping, there were quite a few other changes in
> SystemProps
> so keeping the primitive data separate was useful, possibly the class
> could be unloaded
> since its use-once code.
> >
> > A few other comments:
> >
> > java.base/share/native/libjava/System.c:
> >
> >  112  * The first FIXED_LENGTH entries are the platform defined
> > property values, no names.
> >  113  * The remaining array indices are alternating key/value pairs
> >  114  * supplied by the VM including those defined on the command line
> >  115  * using -Dkey=value that may override the platform defined value.
> >
> > Some of this would also be useful information SystemProps.java, maybe
> > in the comment for Raw.initProps.  Or refer the reader to
> > Java_jdk_internal_util_SystemProps_00024Raw_getRawProperties for a
> > description of the layout of the array.  Or may become moot if we use
> > two separate arrays.
> Agreed, a more complete description would be good in the declaration of
> the native method.
> >
> > --
> >
> > file.encoding used to be set from sprops->encoding on Mac, and
> > sprops->sun_jnu_encoding otherwise:
> >
> >  382 #ifdef MACOSX
> >  383 /*
> >  384  * Since sun_jnu_encoding is now hard-coded to UTF-8 on
> > Mac, we don't
> >  385  * want to use it to overwrite file.encoding
> >  386  */
> >  387 PUTPROP(props, "file.encoding", sprops->encoding);
> >  388 #else
> >  389 PUTPROP(props, "file.encoding", sprops->sun_jnu_encoding);
> >  390 #endif
> >
> > There is no longer an ifdef for this.  But I guess this is OK, as
> > SystemProps.initProperties() will only put a value for "file.encoding"
> > if there isn't one, and only uses sun.jnu.encoding if there is no
> > file.encoding:
> >
> >   59 putIfAbsent(props, "file.encoding",
> >   60 ((raw.propDefault(Raw._file_encoding_NDX) == null)
> >   61 ? raw.propDefault(Raw._sun_jnu_encoding_NDX)
> >   62 : raw.propDefault(Raw._file_encoding_NDX)));
> >
> Yes, I spent some time ensuring that the result was the same.
> Thanks for confirming the logic.
> > --
> >
> >  313 * Command line overrides with -D are copied above from
> > JVM_getProperties.
> >
> > s/JVM_get/JVM_Get/
> right, fixed, Java naming conventions bleeding into native.
> >
> >
> > java.base/share/classes/jdk/internal/util/SystemProps.java:
> >
> >  126 /**
> >  127  * Puts the property if it is non-null
> >  128  * @param props the Properties
> >  129  * @param key the key
> >  130  * @param value the value
> >  131  */
> >  132 private static void put(Properties props, String key, String
> > value) {
> >
> > I notice that this method is only called once.
> Yes, at the moment, that is the only property that must not be
> overridden on the command line.
> There are others that probably should not be overridable but for
> compatibility...
> >
> > --
> >
> >  175 cmdProps.put(disp, dispValue);
> > ...
> >  186 cmdProps.put(fmt, fmtValue);
> > ) {
> >
> > } else if (
> > Aren't L175 & L186 just putting the property that is already there ?
> Oops,  that's a carryover from a different implementation that had
> separate maps.
> Those can be removed since an I18N property on the command is always
> retained
> only defined if the platform value is supplied and different than the base.
>
> Thanks for the review and suggestions
>
> Roger
>
> >
> > Thanks,
> > -Brent
> >
> > On 11/13/18 7:59 AM, Roger Riggs wrote:
> >> Please review a re-implementation of the initialization of System
> >> properties
> >> moving some functions from native to Java.
> >>
> >> The upcalls from native to java for each property are replaced by a
> >> mechanism
> >> to gather the platform, VM and command line properties and return them
> >> from a single JNI call.  The creation of the Properties instance and
> >> applying

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Ichiroh Takiguchi]

Hello Magnus.

IBM33722 should be in sun.nio.cs.ext package on jdk.charset module
Because it's not used for default encoding on AIX platform.

In case of template file, build tool checks 
make/data/charsetmapping/stdcs-aix file for this case.

If class name is there, it will be migrated to sun.nio.cs package.
About IBM33722,
If IBM33722 is moved to sun.nio.cs package also,
"import sun.nio.cs.*;" is no need on IBM33722.java
If IBM33722 is not in stdcs-aix,
"import sun.nio.cs.*;" is still required on IBM33722.java

Thanks,
Ichiroh Takiguchi

On 2018-12-04 19:42, Magnus Ihse Bursie wrote:

On 2018-12-04 04:30, Ichiroh Takiguchi wrote:

Hello Roger.

Thank you for your suggestion.


src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of 
SimpleEUCEncoder.java.template.
SimpleEUCEncoder.java.template has conversion loop and IBM964 refers 
it.

It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext 
package

I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.

I'm not sure I'm fully following the template intricacies here, but
would this not be solved if IBM33722.java was made a template as well?
Then you could do
import $PACKAGE$. SimpleEUCEncoder
Or so I'd think.

/Magnus



TestIBMBugs:
  - Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.

I'll changed.


  - The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 
33722?

Yes, it's no need.

Could you review the fix again ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/

Thanks,
Ichiroh Takiguchi

On 2018-12-04 05:50, Roger Riggs wrote:

Hi Ichiroh,

src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.

TestIBMBugs:
  - Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.

  - The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 
33722?


Regards, Roger


On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:



On 2018-11-30 10:49, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix again ?

Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review 
it too.


/Magnus


I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)

On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.

Without fix
$ jshell
|  Welcome to JShell -- Version 12-ea
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964

jshell> /exit
|  Goodbye
$
==

With fix
==
$ jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

On AIX platform
IBM964 is moved to java.base module from jdk.charset module.

==
$ LANG=zh_TW jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

I'd like to obtain a sponsor for this issue.

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-29 22:39, Ichiroh Takiguchi wrote:

Hello Alan & Magnus.

Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.

I think I should not put 2 kind of modification.

For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)

I'll submit code review again.

Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix

I'll not touch

Re: RFR: 8212794 IBM-964 is required for AIX default charset

[Magnus Ihse Bursie]

On 2018-12-04 04:30, Ichiroh Takiguchi wrote:

Hello Roger.

Thank you for your suggestion.


src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.
"import sun.nio.cs.*;" is required because of 
SimpleEUCEncoder.java.template.

SimpleEUCEncoder.java.template has conversion loop and IBM964 refers it.
It means that,
* On AIX platform, SimpleEUCEncoder should be in sun.nio.cs package
* On non-AIX platform, SimpleEUCEncoder should be in sun.nio.cs.ext 
package

I could not determine which package has SimpleEUCEncoder.
And same kind code is available on ISO2022_JP.java.
Please let me know if you know another way.
I'm not sure I'm fully following the template intricacies here, but 
would this not be solved if IBM33722.java was made a template as well? 
Then you could do

import $PACKAGE$. SimpleEUCEncoder
Or so I'd think.

/Magnus



TestIBMBugs:
  - Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.

I'll changed.


  - The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 33722?

Yes, it's no need.

Could you review the fix again ?
Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.03/

Thanks,
Ichiroh Takiguchi

On 2018-12-04 05:50, Roger Riggs wrote:

Hi Ichiroh,

src/jdk.charsets/share/classes/sun/nio/cs/ext/IBM33722.java:

I think this is no longer needed since it only has imports.
By the way, the style is to import each specific class and
avoid wild card imports.

TestIBMBugs:
  - Please use a style consistent with other methods in the class.
In this case spaces are needed around "{" and "}, and a space
after "," comma.

  - The new method bug8212794, includes a test for x-IBM33722.
Is that needed since there does not appear to be a change for 33722?

Regards, Roger


On 11/30/2018 09:58 AM, Magnus Ihse Bursie wrote:



On 2018-11-30 10:49, Ichiroh Takiguchi wrote:

Hello.

Could you review the fix again ?

Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.02/
I think it looks good but please let someone from core-libs review 
it too.


/Magnus


I just fixed only IBM964 for JDK-8212794.
(IBM29626C fix is not included)

On non AIX platform (Linux),
ibm-euctw alias is added for IBM964.

Without fix
$ jshell
|  Welcome to JShell -- Version 12-ea
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm964
964

jshell> /exit
|  Goodbye
$
==

With fix
==
$ jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.forName("IBM964")
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.ext.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

On AIX platform
IBM964 is moved to java.base module from jdk.charset module.

==
$ LANG=zh_TW jshell
|  Welcome to JShell -- Version 12-internal
|  For an introduction type: /help intro

jshell> var cs = java.nio.charset.Charset.defaultCharset()
cs ==> x-IBM964

jshell> cs.getClass().getName()
$2 ==> "sun.nio.cs.IBM964"

jshell> System.out.println(String.join("\n", cs.aliases()))
ibm-964
cp964
ibm-euctw
ibm964
964

jshell> /exit
|  Goodbye
$
==

I'd like to obtain a sponsor for this issue.

Thanks,
Ichiroh Takiguchi
IBM Japan, Ltd.

On 2018-11-29 22:39, Ichiroh Takiguchi wrote:

Hello Alan & Magnus.

Sorry for you confusion.
I did many copy actions and rename actions.
So you may see, I added unexpected code into non AIX platform.

I think I should not put 2 kind of modification.

For this bug id, I'll handle IBM964 and IBM33722.
(also SimpleEUCEncoder.java is required)

I'll submit code review again.

Additionally, I'll touch
make/data/charsetmapping/charsets
make/data/charsetmapping/stdcs-aix

I'll not touch
make/jdk/src/classes/build/tools/charsetmapping/Main.java
make/jdk/src/classes/build/tools/charsetmapping/SRC.java

My build machine is not so fast, after test is done.
I'll post new code.

Thanks,
Ichiroh Takiguchi

On 2018-11-28 19:10, Magnus Ihse Bursie wrote:

On 2018-11-28 10:36, Alan Bateman wrote:

On 28/11/2018 09:28, Magnus Ihse Bursie wrote:
I'm quite unsatisfied with the current handling of character 
sets in the build in general. :-( I'd really like to modernize 
it. I have a, slightly fuzzy, laundry list of things I want to 
fix from a build perspective, but I'm not sure of what 
"external" 

Re: RFR: JDK-8066619: String(byte[],int,int,int) in String has been deprecated in Manifest and Attributes

[Philipp Kunz]

Hi Roger,
I'm afraid the previous patch missed the tests, should be included this
time.
The intention of the patch is to solve only bug 8066619 about
deprecation. I sincerely hope the changes are neutral.
The new ValueUtf8Coding test heavily coincides/overlaps with 6202130
which is why I mentioned it. I'm however not satisfied that that test
alone also completely solves 6202130 because 6202130 has or might have
implications with breaking characters encoded in UTF-8 with more than
one bytes across a line break onto a continuation line which is not
part of the current patch proposed for 8066619. At some point I
proposed ValueUtf8Coding with only removing the comments from the
implementation in http://mail.openjdk.java.net/pipermail/core-libs-dev/
2018-October/056166.html but I have changed my mind since then and
think now 6202130 should also change the implementation not to break
lines inside of multi-byte characters which is not part of the current
patch and is probably easier after the current patch if necessary at
all. Both 6202130 and the current patch for 8066619 here touch the UTF-
8 coding of manifests and which ever is solved first should add a
corresponding test because no such test exists yet I believe. Worth to
mention are test/jdk/tools/launcher/DiacriticTest.java and
test/jdk/tools/launcher/UnicodeTest.java both of which test the JVM
launch and have a somewhat different purpose. I haven't found any other
test for the specifically changed lines of code apart from probably
many tests that use manifests indirectly in some form. 
Regards,Philipp

On Mon, 2018-12-03 at 16:43 -0500, Roger Riggs wrote:
> Hi Phillip,
> 
> The amount detail obscures the general purpose.
> And there appears to be more than 1.
> The Jira issue IDs mentioned are 8066619 and 6202130.
> 
> Is this functionally neutral and only fixes the deprecations?
> 
> There is a mention that a test is needed for multi-byte chars, but a
> test
> is not included.  Is there an existing test for that?
> 
> Its probably best to identify the main functional improvement (multi-
> byte)
> and fix the deprecation as a side effect.
> 
> Thanks for digging through the issues and the explanations;
> it will take a bit of study to unravel and understand everything in
> this 
> changeset.
> 
> Regards, Roger
> 
> 
> On 12/01/2018 06:49 AM, Philipp Kunz wrote:
> > Find the proposed patch attached. Some comments and explanations,
> > here:
> > 
> > There is a quite interesting implementation in Manifest and
> > Attributes
> > worth quite some explanation. The way it used to work before was:
> > 
> > 1. put manifest header name, colon and space into a StringBuffer
> > -> the buffer now contains a string of characters each high-byte of
> > which is zero as explained later why this is important. the high-
> > bytes
> > are zero because the set of allowed characters is very limited to
> > ":",
> > " ", "a" - "z", "A" - "Z", "0" - "9", "_", and "-" according to
> > Attributes.Name#hash(String) so far with only the name and the
> > separator and yet without the values.
> > 
> > 2. if the value is not null, encode it in UTF-8 into a byte array
> > and
> > instantiate a String with it using deprecated
> > String#String(byte[],int,int,int) resulting in a String with the
> > same
> > length as the byte array before holding one byte in each
> > character's
> > low-byte. This makes a difference for characters encoded with more
> > than
> > one byte in UTF-8. The new String is potentially longer than the
> > original value.
> > 
> > 3. if the value is not null, append value to buffer. The one UTF-8
> > encoded byte per character from the appended string is preserved
> > also
> > in the buffer along with the previous buffer contents.
> > 
> > 3alt. if the value is null, add "null" to the buffer. See
> > java.lang.AbstractStringBuilder#append(String). Neither of the
> > characters of "null" has a non-zero high-byte encoded as UTF-16
> > chars.
> > 
> > 4. make72Safe inserts line breaks with continuation spaces. Note
> > that
> > the buffer here contains only one byte per character because all
> > high-
> > bytes are still zero so that line.length() and line.insert(index,
> > ...)
> > effectively operate with byte offsets and not characters.
> > 
> > 5. buffer.toString()
> > 
> > 6. DataOutputStream#writeBytes(String). First of all read the
> > JavaDoc
> > comment for it, which explains it all:
> >  Writes out the string to the underlying output stream as a
> >  sequence of bytes. Each character in the string is written
> > out, in
> >  sequence, by discarding its high eight bits. If no exception
> > is
> >  thrown, the counter written is incremented by the
> >  length of s
> > This restores the earlier UTF-8 encoding correctly.
> > 
> > The topic has been discussed and mentioned already in
> > http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/05294
> > 6.ht
> > ml
> > https://bugs.openjdk.java.net/browse/JDK-6202130
> > 
> > 

Re: RFR: 8214712: Archive Attributes$Name.KNOWN_NAMES

[Claes Redestad]

On 2018-12-04 08:42, Alan Bateman wrote:

On 03/12/2018 16:50, Claes Redestad wrote:

:

The extra Names added to KNOWN_NAMES was my doing, and it was based 
on commonly used attributes in Jar file manifests scanned from a set 
commonly used applications as an alternative to building up a Name 
cache dynamically (which is frought with other perils). The chose 
extras gave a marginal but measurable improvement in startup to a 
wide array of applications as a result, and their inclusion is 
strictly an internal implementation detail. If you insist on having 
them removed I won't object, but I believe it's an harmless 
optimization.
There are several non-standard and tool/library specify attribute name 
in this list, many should not be interesting to libraries on either 
the class path or module path. So I think we should look at pruning 
that list. Ideally it would like just standard and JDK-specific 
attributes as they are the only attributes that the JDK knows about.


These non-standard and tool/library specific names appear in the 
manifest of a great many jar files, for example on maven central, and 
including the most common non-standard manifest attributes significantly 
reduce allocation churn reading such manifests. Especially true for ones 
that were oft repeated within the same manifest, e.g.,"Name" and 
"SHA1-Digest" for signed entries.


Was it a crucial optimization? Probably not, a few percent on some 
startup applications.


Ideals vs trade-offs... Hard-coding a bunch of arbitrarily chosen 
external values isn't exactly pretty, sure, but I do think the JDK 
should try to optimize for typical behavior, and the fact that many 
(most?) libraries on maven central have OSGi attributes ("Bundle-*") in 
their manifests is one such typical behavior, and this optimization was 
very low cost and net beneficial on most. With 8214712 the overhead cost 
is even less (almost zero in fact).


/Claes

Re: RFR: 8214712: Archive Attributes$Name.KNOWN_NAMES

[Claes Redestad]

Hi David,

unless my reading of JEP 334 (and JEP 303) is completely off then custom 
constants (say, Attributes$Name) would effectively be turned into 
condys, implying a bootstrap method call for them to be constructed. So 
would carry similar construction overhead as the current implementation.


And even if I'm missing some detail of JEP 334 that would allow these 
Name objects to be loaded in without any runtime BSM call overhead 
whatsoever, we'd still need to create the KNOWN_NAMES Map of these 
things. So maybe create the entire map as a constant? I'm not sure we 
even could describe something like a Map as a loadable 
constant, but it'd be an interesting prospect... still, I believe the 
creation cost would end up similar to the status quo.


So: while they may share some common goals (move computation out of 
runtime), I think there are some things heap archiving will keep doing 
better. Facilitating better sharing between JVMs is another benefit that 
comes from more extensive heap archiving.


/Claes

On 2018-12-04 06:09, David Holmes wrote:i

Hi Claes,

Meta-comment: are these Names candidates for the forthcoming 
compile-time evaluation of constants? Just wondering if these 
optimizations (and even the archiving itself) will be moot in the future?

vev
Thanks,
David

On 4/12/2018 2:02 am, Claes Redestad wrote:

Hi,

initializing java.util.jar.Attributes.Name. executes ~20k 
bytecodes setting up and eagerly calculating case-insensitive hash 
codes for a slew of Name objects.


By archiving the resulting set of Names and initializing public 
constants from the archived map, we reduce time spent starting up 
(Name. drops to 368 executed bytecodes) and improve the 
footprint sharing effect of using CDS:


http://cr.openjdk.java.net/~redestad/8214712/jdk.00/

Testing: tier1-2 running

Verified a 1-2.5ms startup improvement on java -jar Hello.jar
- significant and stable reduction in instruction count, branches and 
branch misses

- only adds ~1.1Kb to the dumped CDS archive

Thanks!

/Claes