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

[Paul Sandoz]

> On 27 Jun 2016, at 16:36, Remi Forax <forax at univ-mlv.fr> wrote:
> 
> Hi Claes,
> 
> Change looks good to me :)
> 
> just a minor nit
>  Optional<Integer> buildNumber = build.isPresent() ?
>    Optional.of(Integer.valueOf(build.get())) :
>    Optional.empty();
> can be re-written
>  Optional<Integer> buildNumber = build.map(Integer::parseInt);
> 

I presumed lambda’s were off limits for this section of code.

Paul.

> BTW, i don't know why you're using Integerr.parseInt() when for the build numbers in Version.parse() but Integer.valueOf() in version().
> 
> regards,
> Rémi
> 
> 
> ----- Mail original -----
>> De: "Claes Redestad" <claes.redestad at oracle.com>
>> À: "Paul Sandoz" <paul.sandoz at oracle.com>
>> Cc: core-libs-dev at openjdk.java.net
>> Envoyé: Lundi 27 Juin 2016 16:16:45
>> Objet: Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
>> 
>> 
>> 
>> 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.
>>> 
>>