[15] RFR 8236878: Use atomic instruction to update StringDedupTable's entries and entries_removed counters
Hi, Please review this small change that uses atomic operations to update StringDedupTable's entries and entries_removed counter. This is *not* a correctness fix or performance enhancement, but for Shenandoah GC to move StringDedupTable cleanup task into concurrent phase, while holding StringDedupTable_lock. Bug: https://bugs.openjdk.java.net/browse/JDK-8236878 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.00/index.html Test: hotspot_gc (fastdebug and release) on x86_64 Linux Submit test in progress. Thanks, -Zhengyu
OK. Thanks, Roman
Please review this small change that uses atomic operations to update StringDedupTable's entries and entries_removed counter.
This is *not* a correctness fix or performance enhancement, but for Shenandoah GC to move StringDedupTable cleanup task into concurrent phase, while holding StringDedupTable_lock.
Bug: https://bugs.openjdk.java.net/browse/JDK-8236878 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.00/index.html
Test: hotspot_gc (fastdebug and release) on x86_64 Linux Submit test in progress.
Thanks,
-Zhengyu
Thanks, Roman. -Zhengyu On 1/13/20 9:06 AM, Roman Kennke wrote:
OK.
Thanks, Roman
Please review this small change that uses atomic operations to update StringDedupTable's entries and entries_removed counter.
This is *not* a correctness fix or performance enhancement, but for Shenandoah GC to move StringDedupTable cleanup task into concurrent phase, while holding StringDedupTable_lock.
Bug: https://bugs.openjdk.java.net/browse/JDK-8236878 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.00/index.html
Test: hotspot_gc (fastdebug and release) on x86_64 Linux Submit test in progress.
Thanks,
-Zhengyu
Submit test also passed. May I get a second review? Thanks, -Zhengyu On 1/13/20 9:06 AM, Roman Kennke wrote:
OK.
Thanks, Roman
Please review this small change that uses atomic operations to update StringDedupTable's entries and entries_removed counter.
This is *not* a correctness fix or performance enhancement, but for Shenandoah GC to move StringDedupTable cleanup task into concurrent phase, while holding StringDedupTable_lock.
Bug: https://bugs.openjdk.java.net/browse/JDK-8236878 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.00/index.html
Test: hotspot_gc (fastdebug and release) on x86_64 Linux Submit test in progress.
Thanks,
-Zhengyu
On 1/14/20 6:19 PM, Zhengyu Gu wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8236878 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.00/index.html
It is odd to mix the atomic update and locked update. We can lose locked updates that do not expect anyone to modify the field when lock is held. It is probably fine for _entries_removed, as it is used for statistics. It seems riskier to do for _table->_entries: are we sure nothing in the String dedup table relies on that being very accurate? Can you explain a little bit why we cannot block on StringDedupTable_lock here? Is this a reentrancy issue? -- Thanks, -Aleksey
On 1/15/20 2:17 PM, Aleksey Shipilev wrote:
On 1/14/20 6:19 PM, Zhengyu Gu wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8236878 Webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.00/index.html
It is odd to mix the atomic update and locked update. We can lose locked updates that do not expect anyone to modify the field when lock is held. It is probably fine for _entries_removed, as it is used for statistics. It seems riskier to do for _table->_entries: are we sure nothing in the String dedup table relies on that being very accurate?
Atomic update and locked update do not overlap. The counter updates only happens at a safepoint (before this patch) or with StringDedupTable_lock held (with this patch). Added following patch to make it more obvious. diff -r 7ce7d01e68ec src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp --- a/src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp Wed Jan 15 14:37:34 2020 -0500 +++ b/src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp Wed Jan 15 14:48:21 2020 -0500 @@ -479,6 +479,7 @@ // Delayed update to avoid contention on the table lock if (removed > 0) { + assert_locked_or_safepoint_weak(StringDedupTable_lock); Atomic::sub(&_table->_entries, removed); Atomic::add(&_entries_removed, removed); }
Can you explain a little bit why we cannot block on StringDedupTable_lock here? Is this a reentrancy issue?
Table rehashing is part of cleanup, and it utilizes workers to perform parallel rehashing, therefore, it needs to use lock or atomic operation to update entry counters from each worker. Concurrent string dedup cleaning task need to take StringDedupTable_lock to avoid modification to the table from mutators, so that, workers can not acquire the lock. Otherwise, deadlock. As far as I know, _table->_entries is only used to make rehashing decision, so there is no blocking requirement. Updated webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.01/index.html Test: Reran hotspot_gc test. Thanks, -Zhengyu
On 1/15/20 10:29 PM, Zhengyu Gu wrote:
Updated webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.01/index.html
OK, thanks for explaining. I guess that makes sense. This comment is outdated then: 480 // Delayed update to avoid contention on the table lock I'd suggest to rewrite it to: // Do atomic update here instead of taking StringDedupTable_lock. This allows concurrent // cleanup when multiple workers are cleaning up the table, while the mutators are blocked // on StringDedupTable_lock. ...or some such. -- Thanks, -Aleksey
On 1/16/20 3:51 AM, Aleksey Shipilev wrote:
On 1/15/20 10:29 PM, Zhengyu Gu wrote:
Updated webrev: http://cr.openjdk.java.net/~zgu/JDK-8236878/webrev.01/index.html
OK, thanks for explaining. I guess that makes sense.
This comment is outdated then: 480 // Delayed update to avoid contention on the table lock
I'd suggest to rewrite it to: // Do atomic update here instead of taking StringDedupTable_lock. This allows concurrent // cleanup when multiple workers are cleaning up the table, while the mutators are blocked // on StringDedupTable_lock.
Updated as you suggested and pushed. Thanks, -Zhengyu
...or some such.
participants (3)
-
Aleksey Shipilev
-
Roman Kennke
-
Zhengyu Gu