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.san...@oracle.com> > Cc: "core-libs-dev" <core-libs-dev@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 <fo...@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.redes...@oracle.com> > >> À: "Paul Sandoz" <paul.san...@oracle.com> > >> Cc: core-libs-dev@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.redes...@oracle.com> > >>>> wrote: > >>>> > >>>> Hi Paul, > >>>> > >>>> On 2016-06-27 10:07, Paul Sandoz wrote: > >>>>> > >>>>>> On 26 Jun 2016, at 21:55, Claes Redestad <claes.redes...@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. > >>> > >> > >
