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

[Remi Forax]

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