Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
On 2016-06-28 00:11, Iris Clark wrote: HI, Claes. Sorry, uploaded the previous diff again by mistake, updated in-place now. I see the expected changes for unmodifiableList() now. Your changeset is ready to go from my point of view. Thanks! Pushed. /Claes
RE: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
HI, Claes. > Sorry, uploaded the previous diff again by mistake, updated in-place now. I see the expected changes for unmodifiableList() now. Your changeset is ready to go from my point of view. Regards, iris -Original Message- From: Claes Redestad Sent: Monday, June 27, 2016 3:07 PM To: Mandy Chung Cc: Iris Clark; core-libs-dev; build-dev Subject: Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119 Sorry, uploaded the previous diff again by mistake, updated in-place now. On 2016-06-28 00:04, Mandy Chung wrote: > >> On Jun 27, 2016, at 2:43 PM, Claes Redestad >> wrote: >> >> To accommodate your change request w.r.t. unmodifiableList above I took the >> liberty of cleaning up VersionBuilder.parse() a bit, though. Hope you don't >> mind: >> >> http://cr.openjdk.java.net/~redestad/816/webrev.6/ > > Moving Collections.unmodifiableList to the Version constructor is a good > idea. But I don’t see that change in the webrev.6 though - is that updated? > > 940 List versionNumbers = > VersionProps.versionNumbers(); > 941 version = new > Version(Collections.unmodifiableList(versionNumbers), > 942 VersionProps.pre(), > 943 VersionProps.build(), > 944 VersionProps.optional()); > > 1090 this.version = version; > > 1509 version = Collections.unmodifiableList(list); > > Mandy >
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
Sorry, uploaded the previous diff again by mistake, updated in-place now. On 2016-06-28 00:04, Mandy Chung wrote: On Jun 27, 2016, at 2:43 PM, Claes Redestad wrote: To accommodate your change request w.r.t. unmodifiableList above I took the liberty of cleaning up VersionBuilder.parse() a bit, though. Hope you don't mind: http://cr.openjdk.java.net/~redestad/816/webrev.6/ Moving Collections.unmodifiableList to the Version constructor is a good idea. But I don’t see that change in the webrev.6 though - is that updated? 940 List versionNumbers = VersionProps.versionNumbers(); 941 version = new Version(Collections.unmodifiableList(versionNumbers), 942 VersionProps.pre(), 943 VersionProps.build(), 944 VersionProps.optional()); 1090 this.version = version; 1509 version = Collections.unmodifiableList(list); Mandy
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
> On Jun 27, 2016, at 2:43 PM, Claes Redestad wrote: > > To accommodate your change request w.r.t. unmodifiableList above I took the > liberty of cleaning up VersionBuilder.parse() a bit, though. Hope you don't > mind: > > http://cr.openjdk.java.net/~redestad/816/webrev.6/ Moving Collections.unmodifiableList to the Version constructor is a good idea. But I don’t see that change in the webrev.6 though - is that updated? 940 List versionNumbers = VersionProps.versionNumbers(); 941 version = new Version(Collections.unmodifiableList(versionNumbers), 942 VersionProps.pre(), 943 VersionProps.build(), 944 VersionProps.optional()); 1090 this.version = version; 1509 version = Collections.unmodifiableList(list); Mandy
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
On 2016-06-27 22:11, Iris Clark wrote:
Hi, Claes.
[ Sorry for the premature send, my keyboard started interpreting my shortcuts
in unexpected ways. ]
http://cr.openjdk.java.net/~redestad/816/webrev.5/
Nice bugid.
I can hardly believe my luck! :-)
Overall, this change looks good. I just have a few concerns.
Have you built this forcing alternative build numbers using the makefile
"--with-version-<...>" options? For JDK 9 builds, the default version number
is only "9". You should make sure that versions "9.0.X" and "9.0.0.X" for
arbitrary value of X work as expected.
Done.
Runtime.java: I wonder if unmodifiablelist() should be invoked at new line
1090 instead of 941? Depending on the caller to provide an unmodifiable list
without additional verification seems like a potential problem. If you move
the invocation, then you may want to consider reducing the arguments of
Version() at lines 1086-1089 to the single VersionProps.
I expect to address the following outstanding issues in the coming weeks. I
don't think that they will impact your changes, but I leave it for you to
judge:
8156711: Runtime.Version.version should be an int[] instead of a
List
https://bugs.openjdk.java.net/browse/JDK-8156711
This is an implementation change that was suggested during review. The
thought was that using an int[] is more efficient for space and comparisons.
If you believe that there is benefit to adding this change to yours, feel free
to take on the bug. Otherwise, assuming that there is still benefit to making
this change, I'll take care of it after your changes are in a promotion.
Seems trivial enough, but I think the benefit of this is marginal at
best unless we expect Runtime.Version to be used extensively in
performance critical code. For startup it may even be an ever so slight
negative if it means more code to parse the built-in version string.
8156907: Runtime.Version.{major(),security()} return value of 0 may be
ambiguous
https://bugs.openjdk.java.net/browse/JDK-8156907
After careful consideration, this issue will be resolved by changing the
parse() implementation to allow trialing zero elements (now in
VersionBuilder.parse()). As an example of the target behavior,
v = Runtime.Version.parse("9.0.0") will not throw an IAE and v.toString()
will return "9". Specification adjustments will be needed. Since
VersionBuilder does not change the parse() implementation, I believe that
there won't be any problems.
Good.
I don't think there'll be any conflict with this change, and as I was
testing I also made sure that X=0 works well since the build scripts
already handles shortening this to "9".
To accommodate your change request w.r.t. unmodifiableList above I took
the liberty of cleaning up VersionBuilder.parse() a bit, though. Hope
you don't mind:
http://cr.openjdk.java.net/~redestad/816/webrev.6/
Thanks!
/Claes
Thanks,
iris
RE: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
Hi, Claes.
[ Sorry for the premature send, my keyboard started interpreting my shortcuts
in unexpected ways. ]
> http://cr.openjdk.java.net/~redestad/816/webrev.5/
Nice bugid.
Overall, this change looks good. I just have a few concerns.
Have you built this forcing alternative build numbers using the makefile
"--with-version-<...>" options? For JDK 9 builds, the default version number
is only "9". You should make sure that versions "9.0.X" and "9.0.0.X" for
arbitrary value of X work as expected.
Runtime.java: I wonder if unmodifiablelist() should be invoked at new line
1090 instead of 941? Depending on the caller to provide an unmodifiable list
without additional verification seems like a potential problem. If you move
the invocation, then you may want to consider reducing the arguments of
Version() at lines 1086-1089 to the single VersionProps.
I expect to address the following outstanding issues in the coming weeks. I
don't think that they will impact your changes, but I leave it for you to
judge:
8156711: Runtime.Version.version should be an int[] instead of a
List
https://bugs.openjdk.java.net/browse/JDK-8156711
This is an implementation change that was suggested during review. The
thought was that using an int[] is more efficient for space and comparisons.
If you believe that there is benefit to adding this change to yours, feel free
to take on the bug. Otherwise, assuming that there is still benefit to making
this change, I'll take care of it after your changes are in a promotion.
8156907: Runtime.Version.{major(),security()} return value of 0 may be
ambiguous
https://bugs.openjdk.java.net/browse/JDK-8156907
After careful consideration, this issue will be resolved by changing the
parse() implementation to allow trialing zero elements (now in
VersionBuilder.parse()). As an example of the target behavior,
v = Runtime.Version.parse("9.0.0") will not throw an IAE and v.toString()
will return "9". Specification adjustments will be needed. Since
VersionBuilder does not change the parse() implementation, I believe that
there won't be any problems.
Thanks,
iris
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
Hi Iris, I've configured and run with various alternative build numbers, such as --with-version-patch=5, which produces 9.0.0.5, and verified it works. There was a bug in webrev.4 and earlier versions where a + 1 had slipped out of the loop, fixed in the latest webrev[1]. What other concerns...? :-) Thanks! /Claes [1] http://cr.openjdk.java.net/~redestad/816/webrev.5/ On 2016-06-27 20:51, Iris Clark wrote: Hi, Claes. http://cr.openjdk.java.net/~redestad/816/webrev.4/ Overall, this change looks good. I just have a few concerns. Have you built this forcing alternative build numbers? I -Original Message- From: Claes Redestad Sent: Sunday, June 26, 2016 12:56 PM To: core-libs-dev Libs; build-dev Subject: RFR: 8160000: Runtime.version() cause startup regressions in 9+119 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/816/webrev.3/ Bug: https://bugs.openjdk.java.net/browse/JDK-816 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! /Claes
RE: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
Hi, Claes. http://cr.openjdk.java.net/~redestad/816/webrev.4/ Overall, this change looks good. I just have a few concerns. Have you built this forcing alternative build numbers? I -Original Message- From: Claes Redestad Sent: Sunday, June 26, 2016 12:56 PM To: core-libs-dev Libs; build-dev Subject: RFR: 816: Runtime.version() cause startup regressions in 9+119 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/816/webrev.3/ Bug: https://bugs.openjdk.java.net/browse/JDK-816 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! /Claes
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
On 2016-06-27 18:14, Mandy Chung wrote: On Jun 27, 2016, at 9:10 AM, Claes Redestad wrote: On 2016-06-27 17:22, Mandy Chung wrote: On Jun 27, 2016, at 7:16 AM, Claes Redestad wrote: http://cr.openjdk.java.net/~redestad/816/webrev.4/ Looks good in general. Thanks Mandy, I suggest VersionProps::build to return Optional rather than Optional and make the methods in VersionProps package-private. Sure, but not sure public vs package-private matters much since the class is package-private? The class and methods are intended for package private use only. Making them explicit in the declaration to show the clear intention is a plus to the reader. Right, my concern was with introducing an inconsistency with existing code. Cleaned up: http://cr.openjdk.java.net/~redestad/816/webrev.5/ /Claes
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
> On Jun 27, 2016, at 9:10 AM, Claes Redestad wrote: > > > > On 2016-06-27 17:22, Mandy Chung wrote: >> >>> On Jun 27, 2016, at 7:16 AM, Claes Redestad >>> wrote: >>> >>> http://cr.openjdk.java.net/~redestad/816/webrev.4/ >> >> Looks good in general. > > Thanks Mandy, > >> I suggest VersionProps::build to return Optional rather than >> Optional and make the methods in VersionProps package-private. > > Sure, but not sure public vs package-private matters much since the class is > package-private? The class and methods are intended for package private use only. Making them explicit in the declaration to show the clear intention is a plus to the reader. Mandy
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
On 2016-06-27 17:22, Mandy Chung wrote: On Jun 27, 2016, at 7:16 AM, Claes Redestad wrote: http://cr.openjdk.java.net/~redestad/816/webrev.4/ Looks good in general. Thanks Mandy, I suggest VersionProps::build to return Optional rather than Optional and make the methods in VersionProps package-private. Sure, but not sure public vs package-private matters much since the class is package-private? /Claes
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
> On Jun 27, 2016, at 7:16 AM, Claes Redestad wrote: > > http://cr.openjdk.java.net/~redestad/816/webrev.4/ Looks good in general. I suggest VersionProps::build to return Optional rather than Optional and make the methods in VersionProps package-private. Mandy
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
On 2016-06-27 16:36, Remi Forax wrote: BTW, i don't know why you're using Integerr.parseInt() when for the build numbers in Version.parse() but Integer.valueOf() in version(). Oops, but since we're boxing in both cases we end up with the same bytecode. I'll make the arbitrary choice and use parseInt consistently. Thanks! /Claes
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
This is a startup optimization, not a bootstrap issue: the Multi-release
feature touches Runtime.version() the first time a jar file is loaded,
so we'd like to defer lambda usage from this code since a great number
of small applications will take a relatively big hit.
/Claes
On 2016-06-27 16:54, Remi Forax wrote:
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"
Cc: "core-libs-dev"
Envoyé: Lundi 27 Juin 2016 16:41:02
Objet: Re: RFR: 816: Runtime.version() cause startup regressions in 9+119
On 27 Jun 2016, at 16:36, Remi Forax wrote:
Hi Claes,
Change looks good to me :)
just a minor nit
Optional buildNumber = build.isPresent() ?
Optional.of(Integer.valueOf(build.get())) :
Optional.empty();
can be re-written
Optional 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"
À: "Paul Sandoz"
Cc: core-libs-dev@openjdk.java.net
Envoyé: Lundi 27 Juin 2016 16:16:45
Objet: Re: RFR: 816: 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
wrote:
Hi Paul,
On 2016-06-27 10:07, Paul Sandoz wrote:
On 26 Jun 2016, at 21:55, Claes Redestad
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/816/webrev.3/
Bug: https://bugs.openjdk.java.net/browse/JDK-816
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 values, then the semantics would be a
little clearer.
Mandy commented off-list yesterday about moving the conversion from
String to Optional into VersionProps, which I think meshes well
with your suggestion:
http://cr.openjdk.java.net/~redestad/816/webrev.4/
(I still don't think calculating the number of dots is worth our while,
though)
Does this please?
/Claes
Paul.
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
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"
> Cc: "core-libs-dev"
> 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 wrote:
> >
> > Hi Claes,
> >
> > Change looks good to me :)
> >
> > just a minor nit
> > Optional buildNumber = build.isPresent() ?
> >Optional.of(Integer.valueOf(build.get())) :
> >Optional.empty();
> > can be re-written
> > Optional 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"
> >> À: "Paul Sandoz"
> >> Cc: core-libs-dev@openjdk.java.net
> >> Envoyé: Lundi 27 Juin 2016 16:16:45
> >> Objet: Re: RFR: 816: 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
> >>>> wrote:
> >>>>
> >>>> Hi Paul,
> >>>>
> >>>> On 2016-06-27 10:07, Paul Sandoz wrote:
> >>>>>
> >>>>>> On 26 Jun 2016, at 21:55, Claes Redestad
> >>>>>> 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/816/webrev.3/
> >>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-816
> >>>>>>
> >>>>>> 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 values, then the semantics would be a
> >>> little clearer.
> >>
> >> Mandy commented off-list yesterday about moving the conversion from
> >> String to Optional into VersionProps, which I think meshes well
> >> with your suggestion:
> >>
> >> http://cr.openjdk.java.net/~redestad/816/webrev.4/
> >>
> >> (I still don't think calculating the number of dots is worth our while,
> >> though)
> >>
> >> Does this please?
> >>
> >> /Claes
> >>
> >>>
> >>> Paul.
> >>>
> >>
>
>
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
> On 27 Jun 2016, at 16:36, Remi Forax wrote:
>
> Hi Claes,
>
> Change looks good to me :)
>
> just a minor nit
> Optional buildNumber = build.isPresent() ?
>Optional.of(Integer.valueOf(build.get())) :
>Optional.empty();
> can be re-written
> Optional 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"
>> À: "Paul Sandoz"
>> Cc: core-libs-dev@openjdk.java.net
>> Envoyé: Lundi 27 Juin 2016 16:16:45
>> Objet: Re: RFR: 816: 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
>>>> wrote:
>>>>
>>>> Hi Paul,
>>>>
>>>> On 2016-06-27 10:07, Paul Sandoz wrote:
>>>>>
>>>>>> On 26 Jun 2016, at 21:55, Claes Redestad
>>>>>> 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/816/webrev.3/
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-816
>>>>>>
>>>>>> 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 values, then the semantics would be a
>>> little clearer.
>>
>> Mandy commented off-list yesterday about moving the conversion from
>> String to Optional into VersionProps, which I think meshes well
>> with your suggestion:
>>
>> http://cr.openjdk.java.net/~redestad/816/webrev.4/
>>
>> (I still don't think calculating the number of dots is worth our while,
>> though)
>>
>> Does this please?
>>
>> /Claes
>>
>>>
>>> Paul.
>>>
>>
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
Hi Claes,
Change looks good to me :)
just a minor nit
Optional buildNumber = build.isPresent() ?
Optional.of(Integer.valueOf(build.get())) :
Optional.empty();
can be re-written
Optional 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"
> À: "Paul Sandoz"
> Cc: core-libs-dev@openjdk.java.net
> Envoyé: Lundi 27 Juin 2016 16:16:45
> Objet: Re: RFR: 816: 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
> >> wrote:
> >>
> >> Hi Paul,
> >>
> >> On 2016-06-27 10:07, Paul Sandoz wrote:
> >>>
> >>>> On 26 Jun 2016, at 21:55, Claes Redestad
> >>>> 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/816/webrev.3/
> >>>> Bug: https://bugs.openjdk.java.net/browse/JDK-816
> >>>>
> >>>> 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 values, then the semantics would be a
> > little clearer.
>
> Mandy commented off-list yesterday about moving the conversion from
> String to Optional into VersionProps, which I think meshes well
> with your suggestion:
>
> http://cr.openjdk.java.net/~redestad/816/webrev.4/
>
> (I still don't think calculating the number of dots is worth our while,
> though)
>
> Does this please?
>
> /Claes
>
> >
> > Paul.
> >
>
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
> On 27 Jun 2016, at 16:16, Claes Redestad wrote:
>>
-
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 values, then the semantics would be a
>> little clearer.
>
> Mandy commented off-list yesterday about moving the conversion from String to
> Optional into VersionProps, which I think meshes well with your
> suggestion:
>
> http://cr.openjdk.java.net/~redestad/816/webrev.4/
>
> (I still don't think calculating the number of dots is worth our while,
> though)
>
> Does this please?
>
+1
Paul.
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 wrote:
Hi Paul,
On 2016-06-27 10:07, Paul Sandoz wrote:
On 26 Jun 2016, at 21:55, Claes Redestad 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/816/webrev.3/
Bug: https://bugs.openjdk.java.net/browse/JDK-816
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 values, then the semantics would be a
little clearer.
Mandy commented off-list yesterday about moving the conversion from
String to Optional into VersionProps, which I think meshes well
with your suggestion:
http://cr.openjdk.java.net/~redestad/816/webrev.4/
(I still don't think calculating the number of dots is worth our while,
though)
Does this please?
/Claes
Paul.
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
On 2016-06-27 11:43, Erik Joelsson wrote: Build changes look good. Thanks!
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
Build changes look good. /Erik On 2016-06-26 21:55, Claes Redestad 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/816/webrev.3/ Bug: https://bugs.openjdk.java.net/browse/JDK-816 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! /Claes
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
> On 27 Jun 2016, at 10:39, Claes Redestad wrote:
>
> Hi Paul,
>
> On 2016-06-27 10:07, Paul Sandoz wrote:
>>
>>> On 26 Jun 2016, at 21:55, Claes Redestad 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/816/webrev.3/
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-816
>>>
>>> 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 values, then the semantics would be a
little clearer.
Paul.
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
Hi Paul,
On 2016-06-27 10:07, Paul Sandoz wrote:
On 26 Jun 2016, at 21:55, Claes Redestad 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/816/webrev.3/
Bug: https://bugs.openjdk.java.net/browse/JDK-816
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.
- 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?
-
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.
Thanks!
/Claes
Paul.
Re: RFR: 8160000: Runtime.version() cause startup regressions in 9+119
> On 26 Jun 2016, at 21:55, Claes Redestad 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/816/webrev.3/
> Bug: https://bugs.openjdk.java.net/browse/JDK-816
>
> 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.
- Perhaps it’s worth pre-sizing the array list exactly by counting the ‘.’
before processing? or is 4 always pre-sized exactly?
- add an assert to check Version produced by version() is the same as that
produced the previous way parsing the sys prop
-
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),
…
);
Paul.
RFR: 8160000: Runtime.version() cause startup regressions in 9+119
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/816/webrev.3/ Bug: https://bugs.openjdk.java.net/browse/JDK-816 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! /Claes
