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.

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

8348960: [leyden] compiler/c1/TestConcurrentPatching.java is stuck #30

Closed
wants to merge 4 commits into from
+19 −2
Closed

8348960: [leyden] compiler/c1/TestConcurrentPatching.java is stuck #30

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
374f2a4
Fix
shipilev Jan 29, 2025
1159730
No iffs, no butts
shipilev Jan 29, 2025
1e1ec54
Avoid recursion in more bullet-proof way
shipilev Jan 30, 2025
f8a10da
Move the fix to CompileTask.get
shipilev Jan 31, 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
21 changes: 19 additions & 2 deletions src/hotspot/share/compiler/compileBroker.cpp
View file
Original file line number Diff line number Diff line change
@@ -417,8 +417,9 @@ static bool process_pending(CompileTask* task) {

void CompileQueue::transfer_pending() {
assert(_lock->owned_by_self(), "must own lock");
while (!_queue.empty()) {
CompileTask* task = _queue.pop();

CompileTask* task;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume these changes aren't strictly needed (_queue is always drained under the lock).

Copy link
Member Author

Choose a reason for hiding this comment

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

While _queue is drained under the lock, it is being added to without a lock, which means we are using empty under concurrent queue modification, which is, per NBQ docs, not guaranteed to work well. (ABA problems.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarification. I missed that NonblockingQueue::empty() is not thread-safe.

while ((task = _queue.pop()) != nullptr) {
bool is_stale = process_pending(task);
if (is_stale) {
task->set_next(_first_stale);
@@ -495,6 +496,22 @@ CompileTask* CompileQueue::get(CompilerThread* thread) {
break;
}

// If we have added stale tasks, there might be waiters that want
// the notification these tasks have failed. Normally, this would
// be done by a compiler thread that would perform the purge at
// the end of some compilation. But, if compile queue is empty,
// there is no guarantee compilers would run and do the purge.
// Do the purge here and now to unblock the waiters.
// Perform this until we run out of stale tasks.
while (_first_stale != nullptr) {
purge_stale_tasks();
}
if (_first != nullptr) {
// Purge stale tasks may have transferred some new tasks,
// so check again.
break;
}

// If there are no compilation tasks and we can compile new jobs
// (i.e., there is enough free space in the code cache) there is
// no need to invoke the GC.