> On 27 Jun 2016, at 10:39, Claes Redestad <claes.redes...@oracle.com> wrote:
> 
> Hi Paul,
> 
> On 2016-06-27 10:07, Paul Sandoz wrote:
>> 
>>> On 26 Jun 2016, at 21:55, Claes Redestad <claes.redes...@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.

Reply via email to