> 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.
