Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Reflects review comments > - Merge branch 'master' into JDK-8321206-Locale-static-properties > - Add exclusions in cdsHeapVerifier for new StaticProperties > - initial commit > @iklam or @calvinccheung could either of you take a look at the CDS changes > here please. Thanks. > I'm not following the changes to cdsHeapVerifier.cpp. You've marked the new > entries as C but the definition is: > ``` > // [C] A non-final static string that is assigned a string literal during > class > // initialization; this string is never changed during -Xshare:dump. >``` > and these are final static strings not non-final. ??? Hi David, you're correct that the new entries are not of type "C", as the new strings are `static final`. Anyway, the whole reason of this table is to work around the weakness of cdsHeapVerifier, which can sometimes report false positives. The new entries follow the same pattern of the previous entries (which are also wrong ... i.e., `StaticProperty::FILE_ENCODING`) I have file https://bugs.openjdk.org/browse/JDK-8321940 to improve cdsHeapVerifier. Hopefully I can remove all the entries in cdsHeapVerifier.cpp added by this PR. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1853097367
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Reflects review comments > - Merge branch 'master' into JDK-8321206-Locale-static-properties > - Add exclusions in cdsHeapVerifier for new StaticProperties > - initial commit @iklam or @calvinccheung could either of you take a look at the CDS changes here please. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1852949624
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Reflects review comments > - Merge branch 'master' into JDK-8321206-Locale-static-properties > - Add exclusions in cdsHeapVerifier for new StaticProperties > - initial commit Sorry. I forgot the required reviewers' number differs in the hotspot. Let me know if a JBS issue is needed for further modifying the exclusion reasoning. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1850588085
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Fri, 8 Dec 2023 21:11:57 GMT, Naoto Sato wrote: >> Naoto Sato has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains four additional >> commits since the last revision: >> >> - Reflects review comments >> - Merge branch 'master' into JDK-8321206-Locale-static-properties >> - Add exclusions in cdsHeapVerifier for new StaticProperties >> - initial commit > > Can you elaborate on it more? I don't think it is possible to change the > default locale by modifying those system properties via the `setProperty()` > call in recent JDK releases. @naotoj the hotspot changes were not approved by any Hotspot engineer. Unless trivial, all hotspot code changes require two reviews by hotspot developers. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1849333218
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Fri, 8 Dec 2023 21:11:57 GMT, Naoto Sato wrote: > in recent JDK releases. ok, could be that this has changed before. It does make sense to not allow it if the sequence of initialisation can’t be guaranteed. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1848779128
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Reflects review comments > - Merge branch 'master' into JDK-8321206-Locale-static-properties > - Add exclusions in cdsHeapVerifier for new StaticProperties > - initial commit Can you elaborate on it more? I don't think it is possible to change the default locale by modifying those system properties via the `setProperty()` call in recent JDK releases. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1847848415
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Reflects review comments > - Merge branch 'master' into JDK-8321206-Locale-static-properties > - Add exclusions in cdsHeapVerifier for new StaticProperties > - initial commit This should probably get a release note, i think I remember I saw and even used setProperty in main to set a language locale. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1847767973
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Reflects review comments > - Merge branch 'master' into JDK-8321206-Locale-static-properties > - Add exclusions in cdsHeapVerifier for new StaticProperties > - initial commit Thanks for the reviews. I will ask hotspot folks to rectify the reasonings in `cdsHeapVerifier.cpp` if necessary. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1847661347
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 20:55:48 GMT, Naoto Sato wrote: >> Currently, Locale-related system properties, such as `user.language` or >> `user.country`, are initialized when the `Locale` class is loaded. Making >> them static properties is safer than relying on the `Locale` class loading >> timing, which could potentially be changed depending on the implementation. > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains four additional commits since > the last revision: > > - Reflects review comments > - Merge branch 'master' into JDK-8321206-Locale-static-properties > - Add exclusions in cdsHeapVerifier for new StaticProperties > - initial commit Looks good; simpler and more direct is an improvement. - Marked as reviewed by rriggs (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16986#pullrequestreview-1768704534
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
> Currently, Locale-related system properties, such as `user.language` or > `user.country`, are initialized when the `Locale` class is loaded. Making > them static properties is safer than relying on the `Locale` class loading > timing, which could potentially be changed depending on the implementation. Naoto Sato has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Reflects review comments - Merge branch 'master' into JDK-8321206-Locale-static-properties - Add exclusions in cdsHeapVerifier for new StaticProperties - initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/16986/files - new: https://git.openjdk.org/jdk/pull/16986/files/929f339b..35396106 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16986=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16986=00-01 Stats: 38933 lines in 413 files changed: 13835 ins; 23669 del; 1429 mod Patch: https://git.openjdk.org/jdk/pull/16986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16986/head:pull/16986 PR: https://git.openjdk.org/jdk/pull/16986
Re: RFR: 8321206: Make Locale related system properties `StaticProperty` [v2]
On Wed, 6 Dec 2023 18:10:05 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/jdk/internal/util/StaticProperty.java line 415:
>>
>>> 413: public static String userRegion() {
>>> 414: return USER_REGION;
>>> 415: }
>>
>> Using methods to retrieve these makes is more complicated.
>> The bleeding of the enum values outside of Locale is undesirable.
>> Since the property values are final strings, I suggest just making the
>> fields public and keep the mapping local to the Locale class.
>
> As Alan commented, I will not use `Locale.Category.ordinal()` but instead use
> the properties keys. Would that address your suggestion?
Ended up replacing public methods with public fields
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417964470
