Quantcast

RFR [9] 8177738: Runtime.Version must be a value-based class

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR [9] 8177738: Runtime.Version must be a value-based class

Pavel Rappo
Hello,

Please review the following change:

    http://cr.openjdk.java.net/~prappo/8177738/webrev.00/

This is an attempt to resolve a group of issues related to Version-String API
and its implementation:

    8177738: Runtime.Version must be a value-based class
    8148822: (spec) Regex in Runtime.Version and JEP 223 should match
    8160954: (spec) Runtime.Version regex and $PRE/$OPT issues
    8148877: (spec) Specify when an empty '+' is required in a version string

A good amount of work has been already done here and we just need to finish it.
Let me explain what this patch does starting from the most trivial changes.

- Fixes certain javadoc issues [a] (e.g. dangling html tags, overly qualified
symbol references in @link tags, etc.)

- Refactors Basic.java test, making it a little bit easier to read/modify this
test in the future.

- Addresses [2] in Runtime.java:963,1050. In addition I've rephrased Runtime.java:951
to reflect the fact a version string contains only the mentioned components and
nothing else. In other words, $VSTR *matches* a valid version-string rather than
*finds* itself somewhere in this version-string.

- Makes Runtime.Version a value-based class [1][5]. The class has gotten `final`
modifier, the only constructor has become private and the usual (in this case)
verbiage has been added to the javadoc. Before that, Runtime.Version had ticked
all other the boxes in the value-based class checklist already.

- Improves $VNUM list mechanics in VersionProps.java.template and Runtime.java:1149-1153
by providing immutable list instead of unmodifiable. This change has become
possible due to the fact Runtime.Version constructor has become private and,
thus, all the callers are known and trusted.

- Removes redundant/explicit javadoc declarations in `hashCode`, `equals` and
`compareTo` methods. These methods are documented needlessly explicit. The only
truly justified comment sits inside `compareToIgnoreOptional` method.
Since `compareToIgnoreOptional` and `equalsIgnoreOptional` are not inherited,
I marked the "consistent-with" relationship between them explicitly [c].

- Now a few words about a not-so-pleasant part which addresses [3][4]. In a
nutshell the regex-style format ($VSTR) which is used in the spec to define the
version-string, matches all valid version strings and *some more*.

Here's the format:

    $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)?

If we expand all optional elements we will get the following 8 possibilities:

  ##     $VSTR                      $PRE $BUILD $OPT    EXAMPLE
--------------------------------------------------------------------------------
  1      $VNUM                         0      0    0    9.0.1
  2      $VNUM+-$OPT                   0      0    1    9.0.1+-longcat.dev
  3      $VNUM+$BUILD                  0      1    0    9.0.1+256
  4      $VNUM+$BUILD-$OPT             0      1    1    9.0.1+256-longcat.dev
  5      $VNUM-$PRE                    1      0    0    9.0.1-ea
* 6      $VNUM-$PRE+-$OPT              1      0    1    9.0.1-ea+-longcat.dev
  7      $VNUM-$PRE+$BUILD             1      1    0    9.0.1-ea+256
  8      $VNUM-$PRE+$BUILD-$OPT        1      1    1    9.0.1-ea+256-longcat.dev

Now, ##6 was disallowed by design. In the case when $BUILD is not present, but
both $PRE and $OPT are, the $VSTR will instead look like this:

  6      $VNUM-$PRE-$OPT               1      0    1    9.0.1-ea-longcat.dev

(i.e. without the "+" separator).

This is a problem because it means that in addition to the format it needs a
clear description of exceptional cases which are to be subtracted from the set
of matched version-strings. Such a description has been proposed in [6] (you can
have a look at the webrev that thread refers to):

    * <p> If the build number is unset, then {@code '+'} may only be present
    * when {@code $OPT} is present and {@code $PRE} is not.

It indeed may be a satisfactory solution. However I would argue that it's
probably not the best one. I suspect spec readers will jump straight to the
format completely missing "the special case" paragraph simply because the format
attracts attention and neatly defines the version-string. Neatly but wrongly.

Another option would be to split the $VSTR format into a minimal number of
sub-cases (e.g. Runtime:1032-1034). This will require no extra "special case"
paragraph and will draw reader's attention to all the subtleties on the spot.

  #                                       ##
--------------------------------------------------------------------------------
  1      $VNUM(-$PRE)?\+$BUILD(-$OPT)?    3, 4, 7, 8
  2      $VNUM-$PRE(-$OPT)?               5, 6
  3      $VNUM(+-$OPT)?                   1, 2

One more option would be to require "+" to be there in the case where any
component to the right from $PRE is present (i.e. $BUILD or $OPT or both).
This will greatly simplify the rules and allow the $VSTR to stay as it is today.

This option would affect only the case ##6:

  6      $VNUM-$PRE+-$OPT              1      0    1    9.0.1-ea+-longcat.dev
                   ^^
Yes, it looks a bit ugly, but we already have this ugliness in case 2 (and we
can't get rid of it because of [3]):

  2      $VNUM+-$OPT                   0      0    1    9.0.1+-longcat.dev
              ^^
I suspect both 2 and 6 will be pretty rare anyway as it would mean optional
build information is there, but the build number is not.
Sounds like a corner case to me.

Thanks,
-Pavel

--------------------------------------------------------------------------------
[1] https://bugs.openjdk.java.net/browse/JDK-8177738
[2] https://bugs.openjdk.java.net/browse/JDK-8148822
[3] https://bugs.openjdk.java.net/browse/JDK-8148877
[4] https://bugs.openjdk.java.net/browse/JDK-8160954
[5] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046813.html
[6] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/044277.html

[a] If you build locally or use promoted builds, please keep in mind there's a
    known issue with rendering documentation for Runtime.Version in
    some browsers: https://bugs.openjdk.java.net/browse/JDK-8177727
[b] Startup time/memory footprint might benefit as well (it will definitely
    require some benchmarking to determine if it is the case)
[c] Even though I'm fully aware I do not have the full knowledge the design
    decisions were based upon, I must say I fail to see why equalsIgnoreOptional
    and compareToIgnoreOptional methods have been introduced from the day 1.
    It looks like something that could be neatly solved by providing a custom
    Comparator if needed.

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9] 8177738: Runtime.Version must be a value-based class

Paul Sandoz
Hi,

962 * [1-9][0-9]*((\.0)*\.[1-9][0-9]*)*

You removed the initial “^” which is still mentioned in JDK-8148822

< `^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$`. The sequence may be of arbitrary
---
> `^[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*$`. The sequence may be of arbitrary

JEP 223 is still using the former. Which is correct?

Since Version is now value based i think you need to check where it’s identity is used and replace with equals (which means ensuring that equals is efficient and could use identity under the covers). See usages in JarFile.



Separately i wish we could avoid using List<Integer> as the internal representation, it’s very inefficient, int[] is much better, the cost is then on the version() method but it’s easy to create an immutable List wrapping the array. (I logged an issue for that, but cannot remember the number.)

You patch gets us closer to that, especially with the change to parseVersionNumbers method.

Paul.



> On 30 Mar 2017, at 10:04, Pavel Rappo <[hidden email]> wrote:
>
> Hello,
>
> Please review the following change:
>
>    http://cr.openjdk.java.net/~prappo/8177738/webrev.00/
>
> This is an attempt to resolve a group of issues related to Version-String API
> and its implementation:
>
>    8177738: Runtime.Version must be a value-based class
>    8148822: (spec) Regex in Runtime.Version and JEP 223 should match
>    8160954: (spec) Runtime.Version regex and $PRE/$OPT issues
>    8148877: (spec) Specify when an empty '+' is required in a version string
>
> A good amount of work has been already done here and we just need to finish it.
> Let me explain what this patch does starting from the most trivial changes.
>
> - Fixes certain javadoc issues [a] (e.g. dangling html tags, overly qualified
> symbol references in @link tags, etc.)
>
> - Refactors Basic.java test, making it a little bit easier to read/modify this
> test in the future.
>
> - Addresses [2] in Runtime.java:963,1050. In addition I've rephrased Runtime.java:951
> to reflect the fact a version string contains only the mentioned components and
> nothing else. In other words, $VSTR *matches* a valid version-string rather than
> *finds* itself somewhere in this version-string.
>
> - Makes Runtime.Version a value-based class [1][5]. The class has gotten `final`
> modifier, the only constructor has become private and the usual (in this case)
> verbiage has been added to the javadoc. Before that, Runtime.Version had ticked
> all other the boxes in the value-based class checklist already.
>
> - Improves $VNUM list mechanics in VersionProps.java.template and Runtime.java:1149-1153
> by providing immutable list instead of unmodifiable. This change has become
> possible due to the fact Runtime.Version constructor has become private and,
> thus, all the callers are known and trusted.
>
> - Removes redundant/explicit javadoc declarations in `hashCode`, `equals` and
> `compareTo` methods. These methods are documented needlessly explicit. The only
> truly justified comment sits inside `compareToIgnoreOptional` method.
> Since `compareToIgnoreOptional` and `equalsIgnoreOptional` are not inherited,
> I marked the "consistent-with" relationship between them explicitly [c].
>
> - Now a few words about a not-so-pleasant part which addresses [3][4]. In a
> nutshell the regex-style format ($VSTR) which is used in the spec to define the
> version-string, matches all valid version strings and *some more*.
>
> Here's the format:
>
>    $VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)?
>
> If we expand all optional elements we will get the following 8 possibilities:
>
>  ##     $VSTR                      $PRE $BUILD $OPT    EXAMPLE
> --------------------------------------------------------------------------------
>  1      $VNUM                         0      0    0    9.0.1
>  2      $VNUM+-$OPT                   0      0    1    9.0.1+-longcat.dev
>  3      $VNUM+$BUILD                  0      1    0    9.0.1+256
>  4      $VNUM+$BUILD-$OPT             0      1    1    9.0.1+256-longcat.dev
>  5      $VNUM-$PRE                    1      0    0    9.0.1-ea
> * 6      $VNUM-$PRE+-$OPT              1      0    1    9.0.1-ea+-longcat.dev
>  7      $VNUM-$PRE+$BUILD             1      1    0    9.0.1-ea+256
>  8      $VNUM-$PRE+$BUILD-$OPT        1      1    1    9.0.1-ea+256-longcat.dev
>
> Now, ##6 was disallowed by design. In the case when $BUILD is not present, but
> both $PRE and $OPT are, the $VSTR will instead look like this:
>
>  6      $VNUM-$PRE-$OPT               1      0    1    9.0.1-ea-longcat.dev
>
> (i.e. without the "+" separator).
>
> This is a problem because it means that in addition to the format it needs a
> clear description of exceptional cases which are to be subtracted from the set
> of matched version-strings. Such a description has been proposed in [6] (you can
> have a look at the webrev that thread refers to):
>
>    * <p> If the build number is unset, then {@code '+'} may only be present
>    * when {@code $OPT} is present and {@code $PRE} is not.
>
> It indeed may be a satisfactory solution. However I would argue that it's
> probably not the best one. I suspect spec readers will jump straight to the
> format completely missing "the special case" paragraph simply because the format
> attracts attention and neatly defines the version-string. Neatly but wrongly.
>
> Another option would be to split the $VSTR format into a minimal number of
> sub-cases (e.g. Runtime:1032-1034). This will require no extra "special case"
> paragraph and will draw reader's attention to all the subtleties on the spot.
>
>  #                                       ##
> --------------------------------------------------------------------------------
>  1      $VNUM(-$PRE)?\+$BUILD(-$OPT)?    3, 4, 7, 8
>  2      $VNUM-$PRE(-$OPT)?               5, 6
>  3      $VNUM(+-$OPT)?                   1, 2
>
> One more option would be to require "+" to be there in the case where any
> component to the right from $PRE is present (i.e. $BUILD or $OPT or both).
> This will greatly simplify the rules and allow the $VSTR to stay as it is today.
>
> This option would affect only the case ##6:
>
>  6      $VNUM-$PRE+-$OPT              1      0    1    9.0.1-ea+-longcat.dev
>                   ^^
> Yes, it looks a bit ugly, but we already have this ugliness in case 2 (and we
> can't get rid of it because of [3]):
>
>  2      $VNUM+-$OPT                   0      0    1    9.0.1+-longcat.dev
>              ^^
> I suspect both 2 and 6 will be pretty rare anyway as it would mean optional
> build information is there, but the build number is not.
> Sounds like a corner case to me.
>
> Thanks,
> -Pavel
>
> --------------------------------------------------------------------------------
> [1] https://bugs.openjdk.java.net/browse/JDK-8177738
> [2] https://bugs.openjdk.java.net/browse/JDK-8148822
> [3] https://bugs.openjdk.java.net/browse/JDK-8148877
> [4] https://bugs.openjdk.java.net/browse/JDK-8160954
> [5] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046813.html
> [6] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/044277.html
>
> [a] If you build locally or use promoted builds, please keep in mind there's a
>    known issue with rendering documentation for Runtime.Version in
>    some browsers: https://bugs.openjdk.java.net/browse/JDK-8177727
> [b] Startup time/memory footprint might benefit as well (it will definitely
>    require some benchmarking to determine if it is the case)
> [c] Even though I'm fully aware I do not have the full knowledge the design
>    decisions were based upon, I must say I fail to see why equalsIgnoreOptional
>    and compareToIgnoreOptional methods have been introduced from the day 1.
>    It looks like something that could be neatly solved by providing a custom
>    Comparator if needed.
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9] 8177738: Runtime.Version must be a value-based class

Pavel Rappo

> On 3 Apr 2017, at 20:39, Paul Sandoz <[hidden email]> wrote:
>
> Hi,
>
> 962 * [1-9][0-9]*((\.0)*\.[1-9][0-9]*)*
>
> You removed the initial “^” which is still mentioned in JDK-8148822
>
> < `^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$`. The sequence may be of arbitrary
> ---
>> `^[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*$`. The sequence may be of arbitrary
>
> JEP 223 is still using the former. Which is correct?

The patch from the thread [1] I referred to in my previous email removes ^ and $.
So in this case I simply reused the patch and, once again [2], I do agree this
is an odd place to use boundary matchers, given $VNUM is only a prefix in $VSTR
and not the whole regex.

As for the JEP question, it's more for original designers/authors. If we agree
on it I will update the JEP.

> Since Version is now value based i think you need to check where it’s identity is used and replace with equals (which means ensuring that equals is efficient and could use identity under the covers). See usages in JarFile.

Will do, thanks!

> —
>
> Separately i wish we could avoid using List<Integer> as the internal representation, it’s very inefficient, int[] is much better, the cost is then on the version() method but it’s easy to create an immutable List wrapping the array. (I logged an issue for that, but cannot remember the number.)

What are you concerns specifically?
The startup time?
The amount of simultaneously existing Version objects at runtime?
The use of `Integer` wrapper?

--------------------------------------------------------------------------------
[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/044277.html
[2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046941.html


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9] 8177738: Runtime.Version must be a value-based class

Paul Sandoz

> On 3 Apr 2017, at 16:40, Pavel Rappo <[hidden email]> wrote:
>
>
>> On 3 Apr 2017, at 20:39, Paul Sandoz <[hidden email]> wrote:
>>
>> Hi,
>>
>> 962 * [1-9][0-9]*((\.0)*\.[1-9][0-9]*)*
>>
>> You removed the initial “^” which is still mentioned in JDK-8148822
>>
>> < `^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$`. The sequence may be of arbitrary
>> ---
>>> `^[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*$`. The sequence may be of arbitrary
>>
>> JEP 223 is still using the former. Which is correct?
>
> The patch from the thread [1] I referred to in my previous email removes ^ and $.
> So in this case I simply reused the patch and, once again [2], I do agree this
> is an odd place to use boundary matchers, given $VNUM is only a prefix in $VSTR
> and not the whole regex.
>
> As for the JEP question, it's more for original designers/authors. If we agree
> on it I will update the JEP.
>

Ok, no objections, i was just unsure as to which was the proposed authoritative answer. Perhaps for the record you should also update the description of JDK-8148822?


>> Since Version is now value based i think you need to check where it’s identity is used and replace with equals (which means ensuring that equals is efficient and could use identity under the covers). See usages in JarFile.
>
> Will do, thanks!
>
>> —
>>
>> Separately i wish we could avoid using List<Integer> as the internal representation, it’s very inefficient, int[] is much better, the cost is then on the version() method but it’s easy to create an immutable List wrapping the array. (I logged an issue for that, but cannot remember the number.)
>
> What are you concerns specifically?

It’s just not particularly efficient to box int to Integer into a List on the heap esp for the expected number of version components, and i suspect the version() method probably not something that will often be called, so taking a hit there with a AbstractList wrapper is fine. It’s quite a simple change, esp. with the the foundation you have set up, so seems worthwhile (separate issue, not this one).

Paul.


>
> The startup time?
> The amount of simultaneously existing Version objects at runtime?
> The use of `Integer` wrapper?
>
> --------------------------------------------------------------------------------
> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/044277.html
> [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046941.html
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR [9] 8177738: Runtime.Version must be a value-based class

roger riggs
Hi Pavel,

These changes look fine.

I think you can update the JEP to match the updated patterns mentioned
below.

Regards, Roger


On 4/3/2017 7:50 PM, Paul Sandoz wrote:

>> On 3 Apr 2017, at 16:40, Pavel Rappo <[hidden email]> wrote:
>>
>>
>>> On 3 Apr 2017, at 20:39, Paul Sandoz <[hidden email]> wrote:
>>>
>>> Hi,
>>>
>>> 962 * [1-9][0-9]*((\.0)*\.[1-9][0-9]*)*
>>>
>>> You removed the initial “^” which is still mentioned in JDK-8148822
>>>
>>> < `^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$`. The sequence may be of arbitrary
>>> ---
>>>> `^[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*$`. The sequence may be of arbitrary
>>> JEP 223 is still using the former. Which is correct?
>> The patch from the thread [1] I referred to in my previous email removes ^ and $.
>> So in this case I simply reused the patch and, once again [2], I do agree this
>> is an odd place to use boundary matchers, given $VNUM is only a prefix in $VSTR
>> and not the whole regex.
>>
>> As for the JEP question, it's more for original designers/authors. If we agree
>> on it I will update the JEP.
>>
> Ok, no objections, i was just unsure as to which was the proposed authoritative answer. Perhaps for the record you should also update the description of JDK-8148822?
>
>
>>> Since Version is now value based i think you need to check where it’s identity is used and replace with equals (which means ensuring that equals is efficient and could use identity under the covers). See usages in JarFile.
>> Will do, thanks!
>>
>>> —
>>>
>>> Separately i wish we could avoid using List<Integer> as the internal representation, it’s very inefficient, int[] is much better, the cost is then on the version() method but it’s easy to create an immutable List wrapping the array. (I logged an issue for that, but cannot remember the number.)
>> What are you concerns specifically?
> It’s just not particularly efficient to box int to Integer into a List on the heap esp for the expected number of version components, and i suspect the version() method probably not something that will often be called, so taking a hit there with a AbstractList wrapper is fine. It’s quite a simple change, esp. with the the foundation you have set up, so seems worthwhile (separate issue, not this one).
>
> Paul.
>
>
>> The startup time?
>> The amount of simultaneously existing Version objects at runtime?
>> The use of `Integer` wrapper?
>>
>> --------------------------------------------------------------------------------
>> [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-October/044277.html
>> [2] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-March/046941.html
>>
>>

Loading...