On Wed, 12 May 2021 21:13:54 GMT, Albert Mingkun Yang <ay...@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
