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

8276801: gc/stress/CriticalNativeStress.java fails intermittently with Shenandoah #6316

Closed
wants to merge 7 commits into from
Closed
Changes from 3 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
@@ -111,17 +111,17 @@ void ShenandoahCodeRoots::initialize() {
}

void ShenandoahCodeRoots::register_nmethod(nmethod* nm) {
assert_locked_or_safepoint(CodeCache_lock);
assert(CodeCache_lock->owned_by_self(), "Must have CodeCache_lock held");
_nmethod_table->register_nmethod(nm);
}

void ShenandoahCodeRoots::unregister_nmethod(nmethod* nm) {
assert_locked_or_safepoint(CodeCache_lock);
assert(CodeCache_lock->owned_by_self(), "Must have CodeCache_lock held");
zhengyu123 marked this conversation as resolved.
Show resolved Hide resolved
_nmethod_table->unregister_nmethod(nm);
}

void ShenandoahCodeRoots::flush_nmethod(nmethod* nm) {
assert_locked_or_safepoint(CodeCache_lock);
assert(CodeCache_lock->owned_by_self(), "Must have CodeCache_lock held");
_nmethod_table->flush_nmethod(nm);
}

@@ -271,13 +271,17 @@ void ShenandoahNMethodTable::register_nmethod(nmethod* nm) {
assert(_index >= 0 && _index <= _list->size(), "Sanity");

ShenandoahNMethod* data = ShenandoahNMethod::gc_data(nm);
ShenandoahReentrantLocker data_locker(data != NULL ? data->lock() : NULL);

if (data != NULL) {
assert(contain(nm), "Must have been registered");
assert(nm == data->nm(), "Must be same nmethod");
// Prevent updating a nmethod while concurrent iteration is in progress.
wait_until_concurrent_iteration_done();
ShenandoahReentrantLocker data_locker(data->lock());
data->update();
} else {
// For a new nmethod, we can safely append it to the list, cause
Copy link
Contributor

@shipilev shipilev Nov 9, 2021

Choose a reason for hiding this comment

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

Suggested change
// For a new nmethod, we can safely append it to the list, cause
// For a new nmethod, we can safely append it to the list, because

Copy link
Contributor Author

@zhengyu123 zhengyu123 Nov 9, 2021

Choose a reason for hiding this comment

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

Fixed

// concurrent iteration will not touch it.
data = ShenandoahNMethod::for_nmethod(nm);
assert(data != NULL, "Sanity");
ShenandoahNMethod::attach_gc_data(nm, data);
@@ -290,7 +294,7 @@ void ShenandoahNMethodTable::register_nmethod(nmethod* nm) {
}

void ShenandoahNMethodTable::unregister_nmethod(nmethod* nm) {
assert_locked_or_safepoint(CodeCache_lock);
assert(CodeCache_lock->owned_by_self(), "Lock must be held");

ShenandoahNMethod* data = ShenandoahNMethod::gc_data(nm);
assert(data != NULL, "Sanity");
@@ -382,11 +386,13 @@ void ShenandoahNMethodTable::rebuild(int size) {
}

ShenandoahNMethodTableSnapshot* ShenandoahNMethodTable::snapshot_for_iteration() {
assert(CodeCache_lock->owned_by_self(), "Must have CodeCache_lock held");
_itr_cnt++;
return new ShenandoahNMethodTableSnapshot(this);
}

void ShenandoahNMethodTable::finish_iteration(ShenandoahNMethodTableSnapshot* snapshot) {
assert(CodeCache_lock->owned_by_self(), "Must have CodeCache_lock held");
assert(iteration_in_progress(), "Why we here?");
assert(snapshot != NULL, "No snapshot");
_itr_cnt--;