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

Reduce locks #22

Merged
merged 5 commits into from
Oct 22, 2023
Merged

Reduce locks #22

merged 5 commits into from
Oct 22, 2023

Conversation

LilithSilver
Copy link
Collaborator

@LilithSilver LilithSilver commented Oct 19, 2023

Sort-of fixes #14 by scrapping JobPool with Job structs and instead having a light ConcurrentQueue with managed Job classes.

(This can be merged after #21 is merged.)

The new Job class is a class that is thread safe by nature, where all of its operations when used properly can be used without any locks. It maintains a personal lock (so this isn't lock-free) but that should reduce the chance of lock-contention from "almost guaranteed" to "almost never." I think this is as close to a lock-free solution as we can get at the moment, but it should also make implementing a lock-free solution ten million times easier by constraining locks into one API.

The only time lock contention should happen is:

  1. If Complete() is called on a job during its post-execution cleanup phase, or (briefly) on an old job while it was recycled (we expect Complete() to block so this isn't an issue)
  2. If multiple Complete() calls are made from multiple threads on the same job (again, we expect Complete() to block)
  3. If Schedule() updates the dependents of a particular Job during its post-execution cleanup phase
  4. If Schedule() updates the dependencies of a job while some other thread is trying to Complete() it

3 is the only case where lock contention may occur from the scheduler as overhead, and not from random user Complete() calls. But I don't think there's a way to prevent that, and it seems rare.

I expected it would be faster, but it is WAY faster! Just check out the random graph benchmarks:

Old:

Method Threads ConcurrentJobs Waves Degree EdgeChance Mean Error StdDev Allocated
BenchmarkGraph 0 32 512 4 0.05 33.81 ms 0.633 ms 1.157 ms 600 B
BenchmarkGraph 0 32 512 4 0.2 33.50 ms 0.649 ms 0.930 ms 600 B
BenchmarkGraph 0 128 512 4 0.05 170.72 ms 1.367 ms 1.279 ms 600 B
BenchmarkGraph 0 128 512 4 0.2 161.79 ms 1.639 ms 1.533 ms 600 B
BenchmarkGraph 0 256 512 4 0.05 400.76 ms 3.388 ms 3.170 ms 600 B
BenchmarkGraph 0 256 512 4 0.2 395.06 ms 5.017 ms 4.693 ms 600 B

New:

Method Threads ConcurrentJobs Waves Degree EdgeChance Mean Error StdDev Allocated
BenchmarkGraph 0 32 512 4 0.05 18.43 ms 0.191 ms 0.291 ms 600 B
BenchmarkGraph 0 32 512 4 0.2 19.84 ms 0.342 ms 0.690 ms 600 B
BenchmarkGraph 0 128 512 4 0.05 61.10 ms 1.191 ms 1.170 ms 600 B
BenchmarkGraph 0 128 512 4 0.2 59.67 ms 0.737 ms 0.689 ms 600 B
BenchmarkGraph 0 256 512 4 0.05 98.13 ms 1.242 ms 1.162 ms 600 B
BenchmarkGraph 0 256 512 4 0.2 94.43 ms 1.021 ms 0.955 ms 600 B

The rest of the benchmarks have similar gains.

Copy link
Owner

@genaray genaray 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 otherwhise :)

@LilithSilver
Copy link
Collaborator Author

Added one last thing -- removed a failing test that was debug-only since it's no longer relevant (I removed strict is-flushed checking because we can no longer be sure it actually works (flushing a dependency now flushes all dependents as well, and there's no way to crawl up the chain and check without iterating through all jobs). If we really want we can add a MarkFlushed() thing on Job but that's a job for a later PR.

@LilithSilver LilithSilver mentioned this pull request Oct 20, 2023
@genaray genaray merged commit e1ae4c1 into genaray:master Oct 22, 2023
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.

Possible ways to reduce locks?
2 participants