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

[Claes Redestad]

This is a startup optimization, not a bootstrap issue: the Multi-release 
feature touches Runtime.version() the first time a jar file is loaded, 
so we'd like to defer lambda usage from this code since a great number 
of small applications will take a relatively big hit.

/Claes

On 2016-06-27 16:54, Remi Forax wrote:
> I'm not sure.
> The lazy initialization code already defers the init of the version to (maybe) a time where lambdas are available.
>
> Rémi
>
> ----- Mail original -----
>> De: "Paul Sandoz" <paul.sandoz at oracle.com>
>> Cc: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>> Envoyé: Lundi 27 Juin 2016 16:41:02
>> Objet: Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
>>
>>
>>> 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.
>>>>>
>>>>
>>
>>