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

[Claes Redestad]

On 2016-06-27 11:18, Paul Sandoz wrote:
>
>> 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.

Mandy commented off-list yesterday about moving the conversion from 
String to Optional<String> into VersionProps, which I think meshes well 
with your suggestion:

http://cr.openjdk.java.net/~redestad/8160000/webrev.4/

(I still don't think calculating the number of dots is worth our while, 
though)

Does this please?

/Claes

>
> Paul.
>