RFR: 8160000: Runtime.version() cause startup regressions in 9+119

[Paul Sandoz]

> On 27 Jun 2016, at 10:39, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> Hi Paul,
> 
> On 2016-06-27 10:07, Paul Sandoz wrote:
>> 
>>> On 26 Jun 2016, at 21:55, Claes Redestad <claes.redestad at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> 9+119 changed java.util.regex to initialize java.lang.invoke early, causing a number of easily reproducible startup regressions.
>>> 
>>> This patch uses the fact that we already maintain the version string constituents during build time to simplify creation of the java.lang.Runtime.version().
>>> 
>>> Webrev: http://cr.openjdk.java.net/~redestad/8160000/webrev.3/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8160000
>>> 
>>> Since getting Runtime.version() now does not have to touch java.util.regex classes we end up slightly ahead of previous builds for applications which does not use regular expressions.
>>> 
>>> Thanks!
>>> 
>> 
>> Looks good.
> 
> Thanks for reviewing this!
> 
>> 
>> - Perhaps it’s worth pre-sizing the array list exactly by counting the ‘.’ before processing? or is 4 always pre-sized exactly?
> 
> I figured saving a few bytes in the best case by adding more bytecode and an extra scan of the string would just shift costs around.
> 

Ok. I was hoping consolidation of Optional production would compensate.


>> 
>> - add an assert to check Version produced by version() is the same as that produced the previous way parsing the sys prop
> 
> I actually had that in an earlier version of the patch but found that that is already tested by test/java/lang/Runtime/Version/Basic.java - testVersion and was thus redundant. Do you agree?
> 

Yes, that is ok.


>> 
>> -
>>  957             if (!VersionProps.VERSION_PRE.isEmpty()) {
>>  958                 pre = Optional.of(VersionProps.VERSION_PRE);
>>  959             } else {
>>  960                 pre = Optional.empty();
>>  961             }
>> 
>> Encapsulate that and the other two into a separate method e.g. optionalOfEmpty then do:
>> 
>>   version = new Version(…
>>      optionalOfEmpty(VersionProps.VERSION_PRE),
>>      …
>>      );
> 
> Yes, it'd have been neater if not for the fact that VERSION_BUILD is to be parsed as an Integer, which complicates it a bit. Maybe it is still an improvement.
> 

Drat, i missed the Integer.valueOf. I suppose one could change the types in VersionProps to be Optional<String> values, then the semantics would be a little clearer.

Paul.