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