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

[Remi Forax]

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);

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