RFR: 8272609: Add string deduplication support to SerialGC [v4]

[Denghui Dong]

On Fri, 20 Aug 2021 02:59:01 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Denghui Dong has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fix build error
>
> Some more tidying up.  Sorry for not noticing a couple of these earlier.

@kimbarrett 
Thanks for your careful review!
I have fixed the related problems.

> src/hotspot/share/gc/serial/serialStringDedup.hpp line 27:
> 
>> 25: #define SHARE_GC_SERIAL_STRINGDEDUP_HPP
>> 26: 
>> 27: #include "gc/shared/stringdedup/stringDedup.hpp"
> 
> There is nothing here that requires this header.  Rather, it is needed in the inline.hpp file and should be moved there.

removed

> src/hotspot/share/gc/serial/serialStringDedup.hpp line 42:
> 
>> 40:   // Candidate selection policy for young during evacuation.
>> 41:   // If to is young then age should be the new (survivor's) age.
>> 42:   // if to is old then age should be the age of the copied from object.
> 
> These comments don't match the arguments.  There is no `to` or `age` here.  This comment seems to have been just copied from G1StringDedup.

removed.

> src/hotspot/share/gc/serial/serialStringDedup.hpp line 43:
> 
>> 41:   // If to is young then age should be the new (survivor's) age.
>> 42:   // if to is old then age should be the age of the copied from object.
>> 43:   static inline bool is_candidate_from_evacuation(oop java_string,
> 
> Argument is poorly named.  The value might not be, and indeed is unlikely to be, a String.  Part of this function's job is to check whether the argument is a String.

fixed. Use obj as instead.

> src/hotspot/share/gc/serial/serialStringDedup.hpp line 46:
> 
>> 44:                                                   bool obj_is_tenured);
>> 45: };
>> 46: #endif // SHARE_GC_SERIAL_STRINGDEDUP_HPP
> 
> There's typically a blank line before the final `#endif`.

fixed

> src/hotspot/share/gc/serial/serialStringDedup.inline.hpp line 28:
> 
>> 26: 
>> 27: #include "gc/serial/serialHeap.hpp"
>> 28: #include "gc/serial/serialStringDedup.hpp"
> 
> Inline headers are required to include the associated .hpp first.  That's typically with a blank line between it and the remainder of the includes for emphasis.
> https://github.com/openjdk/jdk/blame/ddcd851c43aa97477c7e406490c0c7c7d71ac629/doc/hotspot-style.md#L141-L144

fixed.

> src/hotspot/share/gc/serial/serialStringDedup.inline.hpp line 36:
> 
>> 34:   // reached the deduplication age threshold, i.e. has not previously been a
>> 35:   // candidate during its life in the young generation.
>> 36:   return SerialHeap::heap()->young_gen()->is_in_reserved(java_string) &&
> 
> This depends on implicit includes since there's nothing here to ensure the type of `young_gen()` is in scope.  I think it's coming from serialHeap.hpp, but that could be changed in the future to forward-declare the generation class rather than include the associated header.  It might be better to have this function in a .cpp file (as it was in an earlier iteration) to limit header exposure in clients.  This function isn't so performance critical, since before calling it we've already checked that deduplication is enabled and the argument is a String, so being inline isn't as important here as for is_candidate_from_evacuation.

Fixed.

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

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