-
Notifications
You must be signed in to change notification settings - Fork 326
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
Move Cleanup Backfills after main SynchronizerCycle & add workers pool #1334
Conversation
18b26a8
to
e058bb1
Compare
@Laremere, @yeukovichd let's discuss pros and cons of putting cleanup method at the beginning or at the end of a sync cycle. |
Also this is discussed here #1324 in more details. |
The reason to move it to after the cycle is because that's when the synchronizer isn't being waited upon, and is doing little work. The backfill will have been a part of many cycles before the cleanup, so I don't see improving by one cycle to be that helpful. The testing problem can be solved by:
Since backfill cleanup will finish before the next fetch matches can run, running fetch matches a second time guarantees that the cleanup which should clean up the backfill has finished. |
c44f578
to
7b80d15
Compare
/gcbrun |
f144c2c
to
bbde4a0
Compare
comment fix
/gcbrun |
d6708b7
to
ef3f73f
Compare
ef3f73f
to
692e921
Compare
692e921
to
55e48f7
Compare
@Laremere please review this PR again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now CleanupBackfills is performed at then end of the regular Synchronizer cycle. Regular Synchronizer steps do not depend on Cleanup running time, possible bugs in Cleanup, etc.
/gcbrun |
What this PR does / Why we need it:
During the contributor's meeting, we agreed with @Laremere to follow this pattern in the cleanup backfills process.
Move CleanupBackfills after main SynchronizerCycle.
Special notes for your reviewer:
There are some benchmarks:
Using workers pool
workers=5; expired backfills=50
BenchmarkCleanupBackfills-12 40 28685354 ns/op 15493765 B/op 90618 allocs/op
workers=3; expired backfills=50
BenchmarkCleanupBackfills-12 34 29910474 ns/op 15493394 B/op 90614 allocs/op
workers=1; expired backfills=50
BenchmarkCleanupBackfills-12 26 40829001 ns/op 15492384 B/op 90604 allocs/op
workers=5; expired backfills=200
BenchmarkCleanupBackfills-12 9 114774808 ns/op 61991684 B/op 362357 allocs/op
workers=3; expired backfills=200
BenchmarkCleanupBackfills-12 8 130279702 ns/op 61985000 B/op 362315 allocs/op
workers=1; expired backfills=200
BenchmarkCleanupBackfills-12 7 200287135 ns/op 61978501 B/op 362230 allocs/op
Previous straightforward implementation
expired backfills=50
BenchmarkCleanupBackfills-12 25 41920697 ns/op 15491571 B/op 90599 allocs/op
expired backfills=200
BenchmarkCleanupBackfills-12 6 186510753 ns/op 61971938 B/op 362200 allocs/op
As you can see, results with worker=1 are pretty close to not using the worker at all.
Also, there is no big difference between 5 and 3 workers, taking into account that all benchmarks have a margin of error.
Closes #1324 .