Re: RFR: 8321206: Make Locale related system properties static properties
On Wed, 6 Dec 2023 08:14:05 GMT, Alan Bateman 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. > > src/java.base/share/classes/java/util/Locale.java line 1099: > >> 1097: StaticProperty.userCountry(category.ordinal() + 1), >> 1098: StaticProperty.userVariant(category.ordinal() + 1), >> 1099: >> getDefaultExtensions(StaticProperty.userExtensions(category.ordinal() + 1)) > > I suspect this would be more readable, and maintainable, if you add > non-public languageKey/scriptKey/countryKey/variantKey methods to > Locale.Category. That would change this to > getInstance(category.languageKey(), ...) so no sight of the ordinal at the > use-site. Right. Will change it in the next iteration - PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417769586
Re: RFR: 8321206: Make Locale related system properties static properties
On Wed, 6 Dec 2023 02:14:13 GMT, David Holmes wrote: > 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. ??? I simply followed the fix to https://bugs.openjdk.org/browse/JDK-8295295 where the last time I added a `StaticProperty`, it broke the CDS test. Since I am not familiar in this area, I am happy to have the reasoning corrected. I would appreciate suggestions. > You also have a large number of test failures with this PR. Are you referring to GHA tests? I am not sure they are relevant to this fix, as my run for mach5 did not cause any regression test failures. > Can I also suggest that you change the bug and PR synopsis to say > StaticProperty rather than static properties as I found it quite confusing to > understand the issue. Modified. >> Making them static properties is safer than relying on the class >> initialization timing > "class initialization" refers to the static initialization of a class. I > assume that is not what you mean here but the creation of the instance of the > class? Even StaticProperty still initializes these things during class > initialization, so I'm unclear exactly what this is trying to say. Modified it to specifically saying `Locale` class loading time. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1843406352
Re: RFR: 8321206: Make Locale related system properties static properties
On Tue, 5 Dec 2023 23:04:55 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 class initialization
> timing, which could potentially be changed depending on the implementation.
src/java.base/share/classes/java/util/Locale.java line 1061:
> 1059: if (sm != null) {
> 1060: sm.checkPropertiesAccess();
> 1061: }
This SM check is no longer needed; the use of privilegedGetProperties made
access unconditional.
The static properties are always accessible to the locale implementation.
src/java.base/share/classes/jdk/internal/util/StaticProperty.java line 113:
> 111: USER_EXTENSIONS_DISPLAY = getProperty(props,
> "user.extensions.display", USER_EXTENSIONS);
> 112: USER_EXTENSIONS_FORMAT = getProperty(props,
> "user.extensions.format", USER_EXTENSIONS);
> 113: USER_REGION = getProperty(props, "user.region", "");
Computing the defaults for these properties should be in the Locale class,
close to the initialization of the default locale. Splitting the responsibility
across files makes it harder to follow what happens where/when.
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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417495473
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417510556
PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1417485546
Re: RFR: 8321206: Make Locale related system properties static properties
On Tue, 5 Dec 2023 23:04:55 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 class initialization > timing, which could potentially be changed depending on the implementation. src/java.base/share/classes/java/util/Locale.java line 1099: > 1097: StaticProperty.userCountry(category.ordinal() + 1), > 1098: StaticProperty.userVariant(category.ordinal() + 1), > 1099: > getDefaultExtensions(StaticProperty.userExtensions(category.ordinal() + 1)) I suspect this would be more readable, and maintainable, if you add non-public languageKey/scriptKey/countryKey/variantKey methods to Locale.Category. That would change this to getInstance(category.languageKey(), ...) so no sight of the ordinal at the use-site. - PR Review Comment: https://git.openjdk.org/jdk/pull/16986#discussion_r1416885432
Re: RFR: 8321206: Make Locale related system properties static properties
On Tue, 5 Dec 2023 23:04:55 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 class initialization > timing, which could potentially be changed depending on the implementation. Can I also suggest that you change the bug and PR synopsis to say `StaticProperty` rather than `static properties` as I found it quite confusing to understand the issue. > Making them static properties is safer than relying on the class > initialization timing "class initialization" refers to the static initialization of a class. I assume that is not what you mean here but the creation of the instance of the class? Even `StaticProperty` still initializes these things during class initialization, so I'm unclear exactly what this is trying to say. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1841978376
Re: RFR: 8321206: Make Locale related system properties static properties
On Tue, 5 Dec 2023 23:04:55 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 class initialization > timing, which could potentially be changed depending on the implementation. 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. ??? You also have a large number of test failures with this PR. - PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1841972901 PR Comment: https://git.openjdk.org/jdk/pull/16986#issuecomment-1841973970
RFR: 8321206: Make Locale related system properties static properties
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 class initialization timing, which could potentially be changed depending on the implementation. - Commit messages: - Add exclusions in cdsHeapVerifier for new StaticProperties - initial commit Changes: https://git.openjdk.org/jdk/pull/16986/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16986&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8321206 Stats: 183 lines in 3 files changed: 163 ins; 6 del; 14 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
