RFR: 8253037: G1: Improve check for string dedup

[Kim Barrett]

On Mon, 2 Nov 2020 08:34:37 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Please review this change to G1 evacuation's checking for string
>> deduplication.  The old code would go out of line to the G1StringDedup
>> support code for every object when deduplication is enabled, only to usually
>> discover the object isn't a string.  Instead we now have a simpl inline
>> test for the combination of dedup enabled and object is a string, and only
>> call out to the dedup support code when that's true.  This eliminates some
>> work for every non-string (non-array) object when dedup is enabled.
>> 
>> The performance impact seems to be pretty small and hard to measure, since
>> enabling deduplication has other costs that seem to overwhelm the cost
>> here.  I'm hoping to improve that with JDK-8254598.
>> 
>> Testing:
>> tier1 on Oracle supported platforms.
>> Performance testing with deduplication enabled.
>
> src/hotspot/share/gc/g1/g1ParScanThreadState.cpp line 86:
> 
>> 84:     _obj_alloc_stat(NULL)
>> 85: {
>> 86:   // Verify klass comparison with _string_klass_or_null is sufficient
> 
> Not sure if this is the correct place to check this property: it needs not only hold during evacuation but basically anywhere. Is it possible to change finality of a java class?
> 
> 
> What would be the impact of java.lang.String not passing this check? From what I understand only that these subclasses of j.l.String are not deduplicated. This does not seem to be a breaking (but surprising) problem, only that dedup for all subclasses of j.l.String does not work.
> 
> I would probably move that check into initialization, printing a warning.

I don't believe finality can be changed in a relevant way. (Not counting changing the source code and recompiling, or perhaps patching the jar file.)  If some project were to try to change String, I think they'd very quickly trip this assert and know something needed to be done. Depending on what the change was, it could effectively disable deduplication (for example, if String were changed to be abstract).

I don't see a good way to check this during initialization that doesn't put the check far away from the code that cares, which makes it much less helpful.  I thought about putting the assert at the point of use, but that seemd a little excessive.

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

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