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

Run isolated tasks last #4004

Merged
merged 4 commits into from
Sep 18, 2024
Merged

Run isolated tasks last #4004

merged 4 commits into from
Sep 18, 2024

Conversation

marcphilipp
Copy link
Member

@marcphilipp marcphilipp commented Sep 18, 2024

  • Fix/suppress warnings
  • Add failing test case
  • Run isolated tasks after all other tasks
  • Add to release notes

Resolves #3928.

@marcphilipp marcphilipp self-assigned this Sep 18, 2024
@marcphilipp marcphilipp enabled auto-merge (squash) September 18, 2024 09:28
Copy link
Collaborator

@leonard84 leonard84 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +160 to +161
Deque<ExclusiveTask> isolatedTasks = new LinkedList<>();
Deque<ExclusiveTask> sameThreadTasks = new LinkedList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 why are we using LinkedList instead of ArrayDeque?
It will probably not really matter performance wise.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was wondering that, too, but kept using LinkedList since it was already in place.

@@ -155,29 +157,34 @@ public void invokeAll(List<? extends TestTask> tasks) {
new ExclusiveTask(tasks.get(0)).execSync();
return;
}
Deque<ExclusiveTask> nonConcurrentTasks = new LinkedList<>();
Deque<ExclusiveTask> isolatedTasks = new LinkedList<>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 The only thing that is a bit weird is that isolated tasks can only be top-level tasks, yet this is invoked on multiple levels. Probably not worth changing.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could have a different class for the top-level task and other tasks. 🤔

@marcphilipp marcphilipp merged commit c8496a2 into main Sep 18, 2024
14 checks passed
@marcphilipp marcphilipp deleted the marc/isolated-last branch September 18, 2024 09:56
marcphilipp added a commit that referenced this pull request Sep 19, 2024
Rather  than executing tasks requiring the global read-write lock
while forked tasks are still being executed and thus causing contention,
such isolated tasks are now executed after all other work is done.

Resolves #3928.

(cherry picked from commit c8496a2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poor thread utilization when using ExclusiveResource.GLOBAL_KEY
2 participants