Skip to content

[performance] Avoid O(n^2) in DeadlockDetector.lockAcquired() #1720

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

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Feb 3, 2025

With n conflicting rules. Has been observed as severe hotspot in workspace with n= ~1000 projects

Functionality is tested with
IJobManagerTest.testTransferToJobWaitingOnChildRule()

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

Looks reasonable.

Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

I think I found an issue here.

@laeubi
Copy link
Contributor

laeubi commented Feb 3, 2025

If we talking about performance, it might be that

//only need two passes through all the locks to pick up all conflicting rules
int NUM_PASSES = 2;

could be one pass if no new items where added to the set?

@jukzi jukzi force-pushed the DeadlockDetector branch 2 times, most recently from 03a5505 to a3d776c Compare February 3, 2025 15:08
@jukzi jukzi requested a review from laeubi February 3, 2025 15:09
@jukzi
Copy link
Contributor Author

jukzi commented Feb 3, 2025

@laeubi please accept the code style as is or proof that your optimizations perform faster. To me it looks like adding additional code does not help.

@laeubi
Copy link
Contributor

laeubi commented Feb 3, 2025

@laeubi please accept the code style as is or proof that your optimizations perform faster. To me it looks like adding additional code does not help.

Its hard to tell as you have not given any numbers of the "severe hotspot in workspace with n= ~1000 projects" I just assumed that if you want to avoid loop over, then you want to avoid copy as well as it also requires time but it is of course just a rough idea.

@jukzi
Copy link
Contributor Author

jukzi commented Feb 3, 2025

I only want to avoid n times ArrayList#contains for obvious performance reasons if the set becomes just large enough, especially if all of those n jobs do it.

With n conflicting rules. "conflicting.contains(possible)" has been
observed as severe hotspot in a workspace with n= ~1000 projects,
especially because all n Jobs did the same.

Also avoid second pass if nothing changed.

Functionality is tested with
IJobManagerTest.testTransferToJobWaitingOnChildRule()
OrderedLockTest.testComplex()
DeadlockDetectionTest.testImplicitRules()
Copy link
Contributor

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

Looks good now

Copy link
Contributor

github-actions bot commented Feb 3, 2025

Test Results

 1 743 files   - 12   1 743 suites   - 12   1h 36m 59s ⏱️ + 6m 3s
 4 170 tests ± 0   4 147 ✅ ± 0   23 💤 ±0  0 ❌ ±0 
13 019 runs   - 88  12 853 ✅  - 87  166 💤  - 1  0 ❌ ±0 

Results for commit 8ff13cc. ± Comparison against base commit d3dd4bf.

@jukzi
Copy link
Contributor Author

jukzi commented Feb 4, 2025

Looks good now

@laeubi thanks for all the good findings.

@jukzi
Copy link
Contributor Author

jukzi commented Feb 4, 2025

build #7 failed with Failed to execute goal org.apache.maven.plugins:maven-war-plugin:3.4.0:war (default-war) on project infocenter-app: Error assembling WAR: Problem creating war: Execution exception: Java heap space -> [Help 1], restarted build

@merks
Copy link
Contributor

merks commented Feb 4, 2025

Looks good now

@laeubi thanks for all the good findings.

Good improvements, good suggestions, and good team work guys! 🏆

@jukzi
Copy link
Contributor Author

jukzi commented Feb 4, 2025

ignoring unrelated failure in windows verification: eclipse-platform/eclipse.platform.swt#1801
(but i wonder why does platform start a UI?)

@jukzi jukzi merged commit 31bb835 into eclipse-platform:master Feb 4, 2025
16 of 17 checks passed
@jukzi jukzi deleted the DeadlockDetector branch February 4, 2025 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants