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