Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8272609: Add string deduplication support to SerialGC #5153

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/hotspot/share/gc/serial/defNewGeneration.cpp
Expand Up @@ -26,6 +26,7 @@
#include "gc/serial/defNewGeneration.inline.hpp"
#include "gc/serial/serialGcRefProcProxyTask.hpp"
#include "gc/serial/serialHeap.inline.hpp"
#include "gc/serial/serialStringDedup.inline.hpp"
#include "gc/serial/tenuredGeneration.hpp"
#include "gc/shared/adaptiveSizePolicy.hpp"
#include "gc/shared/ageTable.inline.hpp"
Expand Down Expand Up @@ -142,7 +143,8 @@ DefNewGeneration::DefNewGeneration(ReservedSpace rs,
: Generation(rs, initial_size),
_preserved_marks_set(false /* in_c_heap */),
_promo_failure_drain_in_progress(false),
_should_allocate_from_space(false)
_should_allocate_from_space(false),
_string_dedup_requests()
{
MemRegion cmr((HeapWord*)_virtual_space.low(),
(HeapWord*)_virtual_space.high());
Expand Down Expand Up @@ -601,6 +603,8 @@ void DefNewGeneration::collect(bool full,
// Verify that the usage of keep_alive didn't copy any objects.
assert(heap->no_allocs_since_save_marks(), "save marks have not been newly set.");

_string_dedup_requests.flush();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if placing _string_dedup_requests.flush(); before weak roots processing (WeakProcessor::weak_oops_do) makes more senses. The documentation says flush should be called when marking is done; for Serial young GC, it's equivalent to when evacuation is done, which is right after ref-processing.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That documentation is mistaken; that's a left-over from an earlier development version that I forgot to update. Without special additional care, the lifetime also needs to cover final-reference processing. I have a todo item to file a bug against that comment.


if (!_promotion_failed) {
// Swap the survivor spaces.
eden()->clear(SpaceDecorator::Mangle);
Expand Down Expand Up @@ -705,13 +709,15 @@ oop DefNewGeneration::copy_to_survivor_space(oop old) {
obj = cast_to_oop(to()->allocate(s));
}

bool new_obj_is_tenured = false;
// Otherwise try allocating obj tenured
if (obj == NULL) {
obj = _old_gen->promote(old, s);
if (obj == NULL) {
handle_promotion_failure(old);
return old;
}
new_obj_is_tenured = true;
} else {
// Prefetch beyond obj
const intx interval = PrefetchCopyIntervalInBytes;
Expand All @@ -728,6 +734,11 @@ oop DefNewGeneration::copy_to_survivor_space(oop old) {
// Done, insert forward pointer to obj in this header
old->forward_to(obj);

if (SerialStringDedup::is_candidate_from_evacuation(obj, new_obj_is_tenured)) {
// Record old; request adds a new weak reference, which reference
// processing expects to refer to a from-space object.
_string_dedup_requests.add(old);
}
return obj;
}

Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/gc/serial/defNewGeneration.hpp
Expand Up @@ -31,6 +31,7 @@
#include "gc/shared/generation.hpp"
#include "gc/shared/generationCounters.hpp"
#include "gc/shared/preservedMarks.hpp"
#include "gc/shared/stringdedup/stringDedup.hpp"
#include "gc/shared/tlab_globals.hpp"
#include "utilities/align.hpp"
#include "utilities/stack.hpp"
Expand Down Expand Up @@ -139,6 +140,8 @@ class DefNewGeneration: public Generation {

STWGCTimer* _gc_timer;

StringDedup::Requests _string_dedup_requests;

enum SomeProtectedConstants {
// Generations are GenGrain-aligned and have size that are multiples of
// GenGrain.
Expand Down
2 changes: 2 additions & 0 deletions src/hotspot/share/gc/serial/genMarkSweep.cpp
Expand Up @@ -112,6 +112,8 @@ void GenMarkSweep::invoke_at_safepoint(ReferenceProcessor* rp, bool clear_all_so

deallocate_stacks();

MarkSweep::_string_dedup_requests->flush();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, flush should be called inside mark_sweep_phase1, right after ref-processing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your review and Kim's detailed explanation.

I think putting this invocation here or the position you suggest should have no effect on the result, so I tend not to make this modification.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it's desirable to narrow the request window (btw first add and flush), which offers more prompt string-dedup and memory release (in flush). Additionally, it makes more sense to call flush right after the complete live objects traversal (strong-marking + final-marking as part of ref-processing), since we will not visit any new objects then.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer it where it is. Rather than putting something between phantom
reference processing an oopstorage-based weak processing, I would prefer to
merge those two. I don't remember if there's an RFE for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think flush should be called sooner, e.g. inside mark_sweep_phase1, which is not in conflict with merging phantom-processing and weak root processing. Anyway, this is subjective; feel free to integrate the current version.


// If compaction completely evacuated the young generation then we
// can clear the card table. Otherwise, we must invalidate
// it (consider all cards dirty). In the future, we might consider doing
Expand Down
3 changes: 3 additions & 0 deletions src/hotspot/share/gc/serial/markSweep.cpp
Expand Up @@ -57,6 +57,8 @@ ReferenceProcessor* MarkSweep::_ref_processor = NULL;
STWGCTimer* MarkSweep::_gc_timer = NULL;
SerialOldTracer* MarkSweep::_gc_tracer = NULL;

StringDedup::Requests* MarkSweep::_string_dedup_requests = NULL;

MarkSweep::FollowRootClosure MarkSweep::follow_root_closure;

MarkAndPushClosure MarkSweep::mark_and_push_closure;
Expand Down Expand Up @@ -214,4 +216,5 @@ void MarkSweep::KeepAliveClosure::do_oop(narrowOop* p) { MarkSweep::KeepAliveClo
void MarkSweep::initialize() {
MarkSweep::_gc_timer = new (ResourceObj::C_HEAP, mtGC) STWGCTimer();
MarkSweep::_gc_tracer = new (ResourceObj::C_HEAP, mtGC) SerialOldTracer();
MarkSweep::_string_dedup_requests = new StringDedup::Requests();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why _string_dedup_requests is a pointer. Is it possible to define static StringDedup::Requests _string_dedup_requests; directly? That way, Requests doesn't need to use the inheritance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a non-local variable with a non-trivial destructor. See
https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2021-August/036412.html

}
3 changes: 3 additions & 0 deletions src/hotspot/share/gc/serial/markSweep.hpp
Expand Up @@ -27,6 +27,7 @@

#include "gc/shared/collectedHeap.hpp"
#include "gc/shared/genOopClosures.hpp"
#include "gc/shared/stringdedup/stringDedup.hpp"
#include "gc/shared/taskqueue.hpp"
#include "memory/iterator.hpp"
#include "oops/markWord.hpp"
Expand Down Expand Up @@ -111,6 +112,8 @@ class MarkSweep : AllStatic {
static STWGCTimer* _gc_timer;
static SerialOldTracer* _gc_tracer;

static StringDedup::Requests* _string_dedup_requests;

// Non public closures
static KeepAliveClosure keep_alive;

Expand Down
8 changes: 8 additions & 0 deletions src/hotspot/share/gc/serial/markSweep.inline.hpp
Expand Up @@ -28,6 +28,8 @@
#include "gc/serial/markSweep.hpp"

#include "classfile/classLoaderData.inline.hpp"
#include "classfile/javaClasses.inline.hpp"
#include "gc/serial/serialStringDedup.hpp"
#include "memory/universe.hpp"
#include "oops/markWord.hpp"
#include "oops/access.inline.hpp"
Expand All @@ -37,6 +39,12 @@
#include "utilities/stack.inline.hpp"

inline void MarkSweep::mark_object(oop obj) {
if (StringDedup::is_enabled() &&
java_lang_String::is_instance_inlined(obj) &&
SerialStringDedup::is_candidate_from_mark(obj)) {
_string_dedup_requests->add(obj);
}

// some marks may contain information we need to preserve so we store them away
// and overwrite the mark. We'll restore it at the end of markSweep.
markWord mark = obj->mark();
Expand Down
13 changes: 13 additions & 0 deletions src/hotspot/share/gc/serial/serialHeap.cpp
Expand Up @@ -28,6 +28,7 @@
#include "gc/serial/tenuredGeneration.inline.hpp"
#include "gc/shared/genMemoryPools.hpp"
#include "gc/shared/strongRootsScope.hpp"
#include "gc/shared/suspendibleThreadSet.hpp"
#include "memory/universe.hpp"
#include "services/memoryManager.hpp"

Expand Down Expand Up @@ -99,3 +100,15 @@ void SerialHeap::young_process_roots(OopIterateClosure* root_closure,

old_gen()->younger_refs_iterate(old_gen_closure);
}

void SerialHeap::safepoint_synchronize_begin() {
if (UseStringDeduplication) {
SuspendibleThreadSet::synchronize();
}
}

void SerialHeap::safepoint_synchronize_end() {
if (UseStringDeduplication) {
SuspendibleThreadSet::desynchronize();
}
}
3 changes: 3 additions & 0 deletions src/hotspot/share/gc/serial/serialHeap.hpp
Expand Up @@ -80,6 +80,9 @@ class SerialHeap : public GenCollectedHeap {
void young_process_roots(OopIterateClosure* root_closure,
OopIterateClosure* old_gen_closure,
CLDClosure* cld_closure);

virtual void safepoint_synchronize_begin();
virtual void safepoint_synchronize_end();
};

#endif // SHARE_GC_SERIAL_SERIALHEAP_HPP
36 changes: 36 additions & 0 deletions src/hotspot/share/gc/serial/serialStringDedup.cpp
@@ -0,0 +1,36 @@
/*
* Copyright (c) 2021, Alibaba Group Holding Limited. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

#include "precompiled.hpp"
#include "gc/serial/defNewGeneration.hpp"
#include "gc/serial/serialHeap.hpp"
#include "gc/serial/serialStringDedup.hpp"
#include "oops/oop.inline.hpp"

bool SerialStringDedup::is_candidate_from_mark(oop java_string) {
// Candidate if string is being evacuated from young to old but has not
// reached the deduplication age threshold, i.e. has not previously been a
// candidate during its life in the young generation.
return SerialHeap::heap()->young_gen()->is_in_reserved(java_string) &&
StringDedup::is_below_threshold_age(java_string->age());
}
44 changes: 44 additions & 0 deletions src/hotspot/share/gc/serial/serialStringDedup.hpp
@@ -0,0 +1,44 @@
/*
* Copyright (c) 2021, Alibaba Group Holding Limited. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

#ifndef SHARE_GC_SERIAL_STRINGDEDUP_HPP
#define SHARE_GC_SERIAL_STRINGDEDUP_HPP

#include "memory/allStatic.hpp"
#include "oops/oopsHierarchy.hpp"

class SerialStringDedup : AllStatic {
public:

// Candidate selection policy for full GC, returning true if the given
// String is a candidate for string deduplication.
// precondition: StringDedup::is_enabled()
// precondition: java_string is a Java String
static bool is_candidate_from_mark(oop java_string);

// Candidate selection policy for young during evacuation.
static inline bool is_candidate_from_evacuation(oop obj, bool obj_is_tenured);

};

#endif // SHARE_GC_SERIAL_STRINGDEDUP_HPP
D-D-H marked this conversation as resolved.
Show resolved Hide resolved
41 changes: 41 additions & 0 deletions src/hotspot/share/gc/serial/serialStringDedup.inline.hpp
@@ -0,0 +1,41 @@
/*
* Copyright (c) 2021, Alibaba Group Holding Limited. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

#ifndef SHARE_GC_SERIAL_STRINGDEDUP_INLINE_HPP
#define SHARE_GC_SERIAL_STRINGDEDUP_INLINE_HPP

#include "gc/serial/serialStringDedup.hpp"
D-D-H marked this conversation as resolved.
Show resolved Hide resolved

#include "classfile/javaClasses.inline.hpp"
#include "oops/oop.inline.hpp"

bool SerialStringDedup::is_candidate_from_evacuation(oop obj,
bool obj_is_tenured) {
return StringDedup::is_enabled() &&
java_lang_String::is_instance_inlined(obj) &&
(obj_is_tenured ?
StringDedup::is_below_threshold_age(obj->age()) :
StringDedup::is_threshold_age(obj->age()));
}

#endif // SHARE_GC_SERIAL_STRINGDEDUP_INLINE_HPP
3 changes: 2 additions & 1 deletion src/hotspot/share/gc/shared/stringdedup/stringDedup.hpp
Expand Up @@ -96,6 +96,7 @@
// For additional information on string deduplication, please see JEP 192,
// http://openjdk.java.net/jeps/192

#include "memory/allocation.hpp"
#include "memory/allStatic.hpp"
#include "oops/oopsHierarchy.hpp"
#include "utilities/globalDefinitions.hpp"
Expand Down Expand Up @@ -195,7 +196,7 @@ class StringDedup : public AllStatic {
// Each marking thread should have it's own Requests object. When marking
// is completed the Requests object must be flushed (either explicitly or by
// the destructor).
class StringDedup::Requests {
class StringDedup::Requests : public CHeapObj<mtGC> {
StorageUse* _storage_for_requests;
oop** _buffer;
size_t _index;
Expand Down
Expand Up @@ -116,7 +116,7 @@ size_t StringDedup::Config::desired_table_size(size_t entry_count) {
bool StringDedup::Config::ergo_initialize() {
if (!UseStringDeduplication) {
return true;
} else if (!UseG1GC && !UseShenandoahGC && !UseZGC && !UseParallelGC) {
} else if (!UseG1GC && !UseShenandoahGC && !UseZGC && !UseParallelGC && !UseSerialGC) {
// String deduplication requested but not supported by the selected GC.
// Warn and force disable, but don't error except in debug build with
// incorrect default.
Expand Down
Expand Up @@ -23,6 +23,19 @@

package gc.stringdedup;

/*
* @test TestStringDeduplicationAgeThreshold
* @summary Test string deduplication age threshold
* @bug 8029075
* @requires vm.gc.Serial
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
* @modules java.base/java.lang:open
* java.management
* @run driver gc.stringdedup.TestStringDeduplicationAgeThreshold Serial
*/

/*
* @test TestStringDeduplicationAgeThreshold
* @summary Test string deduplication age threshold
Expand Down
Expand Up @@ -23,6 +23,19 @@

package gc.stringdedup;

/*
* @test TestStringDeduplicationFullGC
* @summary Test string deduplication during full GC
* @bug 8029075
* @requires vm.gc.Serial
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
* @modules java.base/java.lang:open
* java.management
* @run driver gc.stringdedup.TestStringDeduplicationFullGC Serial
*/

/*
* @test TestStringDeduplicationFullGC
* @summary Test string deduplication during full GC
Expand Down
Expand Up @@ -23,6 +23,19 @@

package gc.stringdedup;

/*
* @test TestStringDeduplicationInterned
* @summary Test string deduplication of interned strings
* @bug 8029075
* @requires vm.gc.Serial
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
* @modules java.base/java.lang:open
* java.management
* @run driver gc.stringdedup.TestStringDeduplicationInterned Serial
*/

/*
* @test TestStringDeduplicationInterned
* @summary Test string deduplication of interned strings
Expand Down
Expand Up @@ -23,6 +23,19 @@

package gc.stringdedup;

/*
* @test TestStringDeduplicationPrintOptions
* @summary Test string deduplication print options
* @bug 8029075
* @requires vm.gc.Serial
* @library /test/lib
* @library /
* @modules java.base/jdk.internal.misc:open
* @modules java.base/java.lang:open
* java.management
* @run driver gc.stringdedup.TestStringDeduplicationPrintOptions Serial
*/

/*
* @test TestStringDeduplicationPrintOptions
* @summary Test string deduplication print options
Expand Down