Skip to content

8368875: [lworld] UseParallelGC fails null narrow klass assertion failure #1655

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

8368875: [lworld] UseParallelGC fails null narrow klass assertion failure #1655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a5e36b
8368875: [lworld] UseParallelGC fails null narrow klass assertion fai…
coleenp Oct 3, 2025
2b1b721
default_prototype_header isn't needed. MemAllocator::finish better ha…
coleenp Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/hotspot/share/gc/parallel/psParallelCompact.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2373,11 +2373,8 @@ void MoveAndUpdateClosure::do_addr(HeapWord* addr, size_t words) {
assert(source() != destination(), "inv");
assert(FullGCForwarding::is_forwarded(cast_to_oop(source())), "inv");
assert(FullGCForwarding::forwardee(cast_to_oop(source())) == cast_to_oop(destination()), "inv");
// Read the klass before the copying, since it might destroy the klass (i.e. overlapping copy)
// and if partial copy, the destination klass may not be copied yet
Klass* klass = cast_to_oop(source())->klass();
Copy::aligned_conjoint_words(source(), copy_destination(), words);
cast_to_oop(copy_destination())->set_mark(Klass::default_prototype_header(klass));
cast_to_oop(copy_destination())->reinit_mark();
}

update_state(words);
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/gc/shared/memAllocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,10 +383,10 @@ oop MemAllocator::finish(HeapWord* mem) const {
// object zeroing are visible before setting the klass non-null, for
// concurrent collectors.
if (UseCompactObjectHeaders) {
oopDesc::release_set_mark(mem, Klass::default_prototype_header(_klass));
oopDesc::release_set_mark(mem, _klass->prototype_header());
} else {
if (EnableValhalla) {
oopDesc::set_mark(mem, Klass::default_prototype_header(_klass));
oopDesc::set_mark(mem, _klass->prototype_header());
} else {
oopDesc::set_mark(mem, markWord::prototype());
}
Expand Down
1 change: 0 additions & 1 deletion src/hotspot/share/oops/klass.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,6 @@ class Klass : public Metadata {
inline markWord prototype_header() const;
inline void set_prototype_header(markWord header);
static ByteSize prototype_header_offset() { return in_ByteSize(offset_of(Klass, _prototype_header)); }
static inline markWord default_prototype_header(Klass* k);
inline void set_prototype_header_klass(narrowKlass klass);

JFR_ONLY(DEFINE_TRACE_ID_METHODS;)
Expand Down
5 changes: 0 additions & 5 deletions src/hotspot/share/oops/klass.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,6 @@ inline markWord Klass::prototype_header() const {
return _prototype_header;
}

// May no longer be required (was used to avoid a bootstrapping problem...
inline markWord Klass::default_prototype_header(Klass* k) {
return (k == nullptr) ? markWord::prototype() : k->prototype_header();
}

inline void Klass::set_prototype_header_klass(narrowKlass klass) {
// Merge narrowKlass in existing prototype header.
_prototype_header = _prototype_header.set_narrow_klass(klass);
Expand Down
18 changes: 8 additions & 10 deletions src/hotspot/share/oops/markWord.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,8 @@ class markWord {

// Should this header be preserved during GC?
bool must_be_preserved() const {
return (!is_unlocked() || !has_no_hash() || (EnableValhalla && is_larval_state()));
return (!is_unlocked() || !has_no_hash() ||
(EnableValhalla && (is_larval_state() || is_inline_type() || is_flat_array() || is_null_free_array())));
Copy link
Member

@Arraying Arraying Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't is_inline_type imply is_larval_state? (edit: other way around)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other way around, is_larval_state implies is_inline_type because only value classes instances can be in a larval state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I was decoding bits in my notebook yesterday. Would rather have them spelled out explicitly in case larval_state goes away or the bits change.

Copy link
Member

@Arraying Arraying Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @fparain, I mixed it up when writing it down. I knew that, I promise 😅.

I think it would be good to introduce an is_valhalla_concept (naming may vary) which ORs the larval, inline type, flat array and null free array bit masks into a single mask. That way we can check it in just one call, which is imo easier to read. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can consider this as a further enhancement. If we add valhalla bits (although I think we're out of bits), this would make sense to collect them all. I think all of this is probably optimized to one load and compare since it's inline.

Copy link
Collaborator

Choose a reason for hiding this comment

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

is_larval_state() is a status that is specific to a particular instance, and cannot be retrieved from another source, and this is why it must be preserved. is_inline_type(), is_flat_array() and is_null_free_array() are all properties than can be retrieved from the K-klass. So I'm not sure why they have been added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bug was that in parallelGC it is unsafe to retrieve the K-Klass to find these properties to re-initialize the moved object. The moved object has written over the markWord because it's not preserved. Adding the code that we must preserve the mark word because these bits matter fixes the bug because then the moved object will have these bits.

Copy link
Collaborator

@fparain fparain Oct 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Replying to myself: The answer seems to be in the comment in oop.inline.hpp:98, but it would be nice to know the exact reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, it would be good to have a GC expert explain this @albertnetymk ? Paul can follow up with him next week. I think it's inherent in the parallelCompaction algorithm that the klass is lost (and then found again?) at this point.

}

// WARNING: The following routines are used EXCLUSIVELY by
Expand Down Expand Up @@ -411,26 +412,23 @@ class markWord {
return (mask_bits(value(), larval_mask_in_place) == larval_pattern);
}

#ifdef _LP64 // 64 bit encodings only
bool is_flat_array() const {
#ifdef _LP64 // 64 bit encodings only
return (mask_bits(value(), flat_array_mask_in_place) == null_free_flat_array_pattern)
|| (mask_bits(value(), flat_array_mask_in_place) == nullable_flat_array_pattern);
#else
return false;
#endif
}

bool is_null_free_array() const {
#ifdef _LP64 // 64 bit encodings only
return (mask_bits(value(), null_free_array_mask_in_place) == null_free_array_pattern);
}
#else
bool is_flat_array() const {
fatal("Should not ask this for mark word, ask oopDesc");
return false;
#endif
}

bool is_null_free_array() const {
fatal("Should not ask this for mark word, ask oopDesc");
return false;
}
#endif
inline Klass* klass() const;
inline Klass* klass_or_null() const;
inline Klass* klass_without_asserts() const;
Expand Down
1 change: 1 addition & 0 deletions src/hotspot/share/oops/oop.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ class oopDesc {
// Used only to re-initialize the mark word (e.g., of promoted
// objects during a GC) -- requires a valid klass pointer
inline void init_mark();
inline void reinit_mark(); // special for parallelGC

inline Klass* klass() const;
inline Klass* klass_or_null() const;
Expand Down
11 changes: 11 additions & 0 deletions src/hotspot/share/oops/oop.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,17 @@ void oopDesc::init_mark() {
set_mark(prototype_mark());
}

// This is for parallel gc, which doesn't always have the klass.
// markWord::must_be_preserved preserves the original prototype header bits for EnableValhalla,
// I don't know why serial gc doesn't work the same.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably find out why. I can ask around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! That would be good. The prototype_mark() function above or even init_mark shouldn't have to be conditional on EnableValhalla if the markWord is always preserved. But serial gc is very unhappy with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear I think this should be a further investigation because this should fix a lot of ParallelGC tests that are currently failing.

void oopDesc::reinit_mark() {
if (UseCompactObjectHeaders) {
set_mark(klass()->prototype_header());
} else {
set_mark(markWord::prototype());
}
}

Klass* oopDesc::klass() const {
switch (ObjLayout::klass_mode()) {
case ObjLayout::Compact:
Expand Down