Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Wed, 19 Jun 2024 12:52:46 GMT, Alan Bateman wrote: >> @AlanBateman -- could you please take a look? Thanks. > >> @AlanBateman -- could you please take a look? Thanks. > > There was a lot of heap analysis done a few years ago that shined a light on > the number of empty collections in a typical heap. I don't recall seeing > COWAL in any of the data but it seems plausible that there are cases where > there are a lot of empty COWAL instances in the heap. > > Main thing for a change like this to make sure it doesn't hurt other cases > and check if there are overridable methods used in the existing + updated > implementations that might get reported as a behavior change by something > that extends and overrides some methods. I don't see anything so I think it's > okay. > > One thing that surprises me is the change to remove(Object) to handle the > case where the last remaining element is removed. Does that help the > application that prompted this change? Part of me wonders if remove(int) > needs to do the same as someone will show up sometime to ask. @AlanBateman can you please confirm remove() is updated as intended, and approve if ready? - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2228637385
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject Thank you! I've expanded coverage to remove(int) as well. > One thing that surprises me is the change to remove(Object) to handle the > case where the last remaining element is removed. Does that help the > application that prompted this change? Part of me wonders if remove(int) > needs to do the same as someone will show up sometime to ask. The application benefits most from the lower footprint of the empty constructors, but I tried to expand the optimization throughout the class. Clearly I missed a spot, thank you for catching it. - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2181366938
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Wed, 19 Jun 2024 11:58:07 GMT, Aleksey Shipilev wrote: > @AlanBateman -- could you please take a look? Thanks. There was a lot of heap analysis done a few years ago that shined a light on the number of empty collections in a typical heap. I don't recall seeing COWAL in any of the data but it seems plausible that there are cases where there are a lot of empty COWAL instances in the heap. Main thing for a change like this to make sure it doesn't hurt other cases and check if there are overridable methods used in the existing + updated implementations that might get reported as a behavior change by something that extends and overrides some methods. I don't see anything so I think it's okay. One thing that surprises me is the change to remove(Object) to handle the case where the last remaining element is removed. Does that help the application that prompted this changed? Part of wonders if remove(int) needs to do the same as someone will show up sometime to ask. - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2178649021
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject @AlanBateman -- could you please take a look? Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2178515650
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject I think this one looks OK now. @AlanBateman might want to provide his take on this. - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2158177971
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 14:29:46 GMT, Aleksey Shipilev wrote: >> `clone()` performs a shallow copy, so there is currently no `Object[]` >> allocated and therefore nothing to optimize. I don't think there's any work >> to do here. >> >> @DougLea @viktorklang-ora can you confirm? > >> clone() performs a shallow copy, so there is currently no Object[] allocated >> and therefore nothing to optimize. > > Yes, I believe so. - PR Review Comment: https://git.openjdk.org/jdk/pull/19527#discussion_r1633127845
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject Yes, I think this is now OK (and maybe barely worth doing). - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2152726032
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Thu, 6 Jun 2024 12:46:36 GMT, jengebr wrote: >> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless >> cloning of Object[0] instances. This cloning is intended to prevent callers >> from changing array contents, but many `CopyOnWriteArrayList`s are allocated >> to size zero, or are otherwise maintained empty, so cloning is unnecessary. >> >> Results from the included JMH benchmark: >> Before: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 74.487 ± 1.793 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 27.918 ± 0.759 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.656 ± 0.375 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 15.415 ± 0.489 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.608 ± 0.363 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 15.374 ± 0.260 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 15.688 ± 0.350 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2625.125 ± 71.802 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2607.447 ± 46.400 ns/op >> >> >> After: >> >> BenchmarkMode Cnt >> Score Error Units >> CopyOnWriteArrayListBenchmark.clear avgt5 >> 75.365 ± 2.092 ns/op >> CopyOnWriteArrayListBenchmark.clearEmpty avgt5 >> 20.803 ± 0.539 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 >> 16.808 ± 0.582 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 >> 12.980 ± 0.418 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 >> 21.627 ± 0.173 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 >> 12.864 ± 0.408 ns/op >> CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 >> 12.931 ± 0.255 ns/op >> CopyOnWriteArrayListBenchmark.readInstance avgt 10 >> 2615.500 ± 30.771 ns/op >> CopyOnWriteArrayListBenchmark.readInstanceEmpty avgt 10 >> 2583.892 ± 62.086 ns/op > > jengebr has updated the pull request incrementally with one additional commit > since the last revision: > > Adding benchmarks for readObject I think this is good. I'll let Doug and Viktor to confirm their comments were addressed. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19527#pullrequestreview-2102178280
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Wed, 5 Jun 2024 14:56:26 GMT, jengebr wrote: > clone() performs a shallow copy, so there is currently no Object[] allocated > and therefore nothing to optimize. Yes, I believe so. - PR Review Comment: https://git.openjdk.org/jdk/pull/19527#discussion_r1629666551
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
> Improve `java/util/concurrent/CopyOnWriteArrayList` by eliminating needless > cloning of Object[0] instances. This cloning is intended to prevent callers > from changing array contents, but many `CopyOnWriteArrayList`s are allocated > to size zero, or are otherwise maintained empty, so cloning is unnecessary. > > Results from the included JMH benchmark: > Before: > > BenchmarkMode Cnt > Score Error Units > CopyOnWriteArrayListBenchmark.clear avgt5 > 74.487 ± 1.793 ns/op > CopyOnWriteArrayListBenchmark.clearEmpty avgt5 > 27.918 ± 0.759 ns/op > CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 > 16.656 ± 0.375 ns/op > CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 > 15.415 ± 0.489 ns/op > CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 > 21.608 ± 0.363 ns/op > CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 > 15.374 ± 0.260 ns/op > CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 > 15.688 ± 0.350 ns/op > > > After: > > BenchmarkMode Cnt > Score Error Units > CopyOnWriteArrayListBenchmark.clear avgt5 > 75.365 ± 2.092 ns/op > CopyOnWriteArrayListBenchmark.clearEmpty avgt5 > 20.803 ± 0.539 ns/op > CopyOnWriteArrayListBenchmark.createInstanceArrayavgt5 > 16.808 ± 0.582 ns/op > CopyOnWriteArrayListBenchmark.createInstanceArrayEmpty avgt5 > 12.980 ± 0.418 ns/op > CopyOnWriteArrayListBenchmark.createInstanceCollection avgt5 > 21.627 ± 0.173 ns/op > CopyOnWriteArrayListBenchmark.createInstanceCollectionEmpty avgt5 > 12.864 ± 0.408 ns/op > CopyOnWriteArrayListBenchmark.createInstanceDefault avgt5 > 12.931 ± 0.255 ns/op jengebr has updated the pull request incrementally with one additional commit since the last revision: Adding benchmarks for readObject - Changes: - all: https://git.openjdk.org/jdk/pull/19527/files - new: https://git.openjdk.org/jdk/pull/19527/files/9ab83c9d..b1920f7a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19527=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19527=01-02 Stats: 43 lines in 1 file changed: 41 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19527.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19527/head:pull/19527 PR: https://git.openjdk.org/jdk/pull/19527
