Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 18:32:42 GMT, Chen Liang wrote:
>> Claes Redestad has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove redundant constructor
>
> src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 84:
>
>> 82: */
>> 83: public static Supplier, ReferenceKey>>
>> concurrentHashMapSupplier() {
>> 84: return new Supplier<>() {
>
> Can this just `return ReferencedKeyMap.concurrentHashMapSupplier();`?
It compiles, so maybe. :-) Running some tests.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594703469
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote:
>> A rather large startup regression was introduced in 23-b13 from
>> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has
>> been dealt with as enhancements such as
>> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802),
>> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and
>> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide
>> both point fixes and reduce initialization overhead of certain constructs
>> more generally. The remaining issues stem from a set of lambdas added in
>> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early
>> bootstrapping of the lambda infrastructure and a bit of class generation.
>>
>> While the remaining overheads are relatively small and borderline acceptable
>> (< 2-3ms), I think it's still worth acting on them in this particular case
>> since the amount of added bootstrapping overhead is dependent on which
>> locale the system runs under, which complicates testing and comparisons due
>> to relatively large differences in paths taken on different systems.
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove redundant constructor
src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 84:
> 82: */
> 83: public static Supplier, ReferenceKey>>
> concurrentHashMapSupplier() {
> 84: return new Supplier<>() {
Can this just `return ReferencedKeyMap.concurrentHashMapSupplier();`?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594474286
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant constructor LGTM - Marked as reviewed by naoto (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19140#pullrequestreview-2046346164
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:57:22 GMT, Naoto Sato wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove redundant constructor > > src/java.base/share/classes/sun/util/locale/BaseLocale.java line 96: > >> 94: // Interned BaseLocale cache >> 95: private static final ReferencedKeySet CACHE = >> 96: ReferencedKeySet.create(true, >> ReferencedKeySet.concurrentHashMapSupplier()); > > Should this supplier be in `BaseLocale` class? Otherwise `ReferencedKeySet` > may end up with static suppliers for each map type? Maybe, though I think most potential uses of `ReferenceKeySet/-Map` will want to be using a plain `CHM` as the backing store, so keeping these next to their respective `create` methods will encourage sharing. - PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r159407
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant constructor Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19140#pullrequestreview-2046311563
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
On Wed, 8 May 2024 17:30:22 GMT, Claes Redestad wrote: >> A rather large startup regression was introduced in 23-b13 from >> [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has >> been dealt with as enhancements such as >> [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), >> [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and >> [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide >> both point fixes and reduce initialization overhead of certain constructs >> more generally. The remaining issues stem from a set of lambdas added in >> code for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early >> bootstrapping of the lambda infrastructure and a bit of class generation. >> >> While the remaining overheads are relatively small and borderline acceptable >> (< 2-3ms), I think it's still worth acting on them in this particular case >> since the amount of added bootstrapping overhead is dependent on which >> locale the system runs under, which complicates testing and comparisons due >> to relatively large differences in paths taken on different systems. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Remove redundant constructor Thanks for fixing this! src/java.base/share/classes/sun/util/locale/BaseLocale.java line 96: > 94: // Interned BaseLocale cache > 95: private static final ReferencedKeySet CACHE = > 96: ReferencedKeySet.create(true, > ReferencedKeySet.concurrentHashMapSupplier()); Should this supplier be in `BaseLocale` class? Otherwise `ReferencedKeySet` may end up with static suppliers for each map type? - PR Review: https://git.openjdk.org/jdk/pull/19140#pullrequestreview-2046313019 PR Review Comment: https://git.openjdk.org/jdk/pull/19140#discussion_r1594429353
Re: RFR: 8331932: Startup regressions in 23-b13 [v4]
> A rather large startup regression was introduced in 23-b13 from > [JDK-8309622](https://bugs.openjdk.org/browse/JDK-8309622). Some of that has > been dealt with as enhancements such as > [JDK-8330802](https://bugs.openjdk.org/browse/JDK-8330802), > [JDK-8330595](https://bugs.openjdk.org/browse/JDK-8330595) and > [JDK-8330681](https://bugs.openjdk.org/browse/JDK-8330681), which provide > both point fixes and reduce initialization overhead of certain constructs > more generally. The remaining issues stem from a set of lambdas added in code > for `java.util.Locale` and `jdk.internal.util.BaseLocale` causing early > bootstrapping of the lambda infrastructure and a bit of class generation. > > While the remaining overheads are relatively small and borderline acceptable > (< 2-3ms), I think it's still worth acting on them in this particular case > since the amount of added bootstrapping overhead is dependent on which locale > the system runs under, which complicates testing and comparisons due to > relatively large differences in paths taken on different systems. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Remove redundant constructor - Changes: - all: https://git.openjdk.org/jdk/pull/19140/files - new: https://git.openjdk.org/jdk/pull/19140/files/d8594b31..819e3d47 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19140=03 - incr: https://webrevs.openjdk.org/?repo=jdk=19140=02-03 Stats: 6 lines in 1 file changed: 0 ins; 6 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19140.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19140/head:pull/19140 PR: https://git.openjdk.org/jdk/pull/19140
