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