RFR: 8254598: StringDedupTable should use OopStorage [v2]

[Kim Barrett]

On Wed, 12 May 2021 21:13:54 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Kim Barrett has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - more comment improvements
>>  - improve naming and comments around injected String flags
>>  - fix some typos in comments
>
> Just some minor comments.

Following up on an off-line discussion with @albertnetymk , I've done a little refactoring of Requests::add.  I also made a few other small cleanups, noticed while dealing with @albertnetymk comments.  I still haven't dealt with the accumulated merge conflicts.  I'll be doing that next.

> src/hotspot/share/gc/shared/stringdedup/stringDedup.cpp line 171:
> 
>> 169:   // Store the string in the next pre-allocated storage entry.
>> 170:   oop* ref = _buffer[--_index];
>> 171:   _buffer[_index] = nullptr;
> 
> It's not really needed to clear the slot with null, right?

You are right; there used to be some asserts, but no longer. Removed.

> src/hotspot/share/gc/shared/stringdedup/stringDedupProcessor.cpp line 51:
> 
>> 49: }
>> 50: 
>> 51: StringDedup::Processor::~Processor() {}
> 
> Why the empty destructor? Could we just not have it?

Changed to use `= default`.

> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 92:
> 
>> 90:   GrowableArrayCHeap<TableValue, mtStringDedup> _values;
>> 91: 
>> 92:   void adjust_capacity(int new_length);
> 
> Nit: diff arg name in declaration and implementation, `new_length` vs `new_capacity`.

Changed to consistently use `new_capacity`

> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 295:
> 
>> 293: 
>> 294: protected:
>> 295:   CleanupState() {}
> 
> Is it possible to use `= default` here? Just like the destructor.

Changed to use `= default`.

> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 314:
> 
>> 312:   size_t _bucket_index;
>> 313:   size_t _shrink_index;
>> 314:   bool _grow_only;
> 
> Seems unused.

Removed unused `_grow_only`, left over from earlier work.

> src/hotspot/share/memory/universe.cpp line 1153:
> 
>> 1151:     ResolvedMethodTable::verify();
>> 1152:   }
>> 1153:   if (should_verify_subset(Verify_StringDedup) && StringDedup::is_enabled()) {
> 
> Maybe drop the `is_enabled()` check in the `if` to keep the consistency with other `if`s?

Done.  StringDedup::verify already does an is_enabled check.  Also fixed the description comment, which incorrectly said `is_enabled()` was a precondition.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3662