RFR: 8253037: G1: Improve check for string dedup

[Thomas Schatzl]

On Sun, 1 Nov 2020 07:04:29 GMT, Kim Barrett <kbarrett 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.

src/hotspot/share/gc/g1/g1ParScanThreadState.hpp line 88:

> 86:   PartialArrayTaskStepper _partial_array_stepper;
> 87:   // Used to check whether string dedup should be applied to an object.
> 88:   Klass* _string_klass_or_null;

Maybe add a note that we replicate java_lang_String::is_instance_inlined() here for performance reasons (skipping the NULL check), so that when it changes, maybe a grep will find this here?

src/hotspot/share/gc/g1/g1StringDedup.hpp line 78:

> 76:   // thread. Before enqueuing, these functions apply the appropriate candidate
> 77:   // selection policy to filters out non-candidates.
> 78:   static void enqueue_from_mark(oop obj, uint worker_id);

As Albert mentioned, it would be nice if the caller of this would also get this optimization as it seems straightforward to implement there as well for consistency.

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

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