[15] RFR 8236878: Use atomic instruction to update StringDedupTable's entries and entries_removed counters

[Zhengyu Gu]

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



>