-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Fix the performance degradation due to different schedule execution and #2912
Fix the performance degradation due to different schedule execution and #2912
Conversation
SubscriptionList.add() and thread unparking.
There doesn't seem to be any performance degradation, however, small sized measurements are quite hectic for some reason. |
|
||
serial.add(s); | ||
s.addParent(serial); | ||
ScheduledAction s = poolWorker.scheduleActual(action, 0, null, serial); |
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.
If I understand correctly this is just removing work that gets done for us within scheduleActual
. Am I reading this correctly? Referenced code is here:
run.add(f); |
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.
The problem here was that serial.add()
happened after the action was scheduled and executed thus if there was some recursive work, those serial.add()
calls could happen before this initial call and thus the order in the list was reversed. Since the removal is O(n) and we expected the list to be in the same order as the executions finish, any disorder increases the remove time and a concurrent lock attempt run out of its spin time and ended up parking. The performance degradation was this increased park/unpark activity. The scheduleAction
overload just adds the action to the serial
before it schedules it thus making sure any recursive schedule happens after it.
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.
Wow, that's an interesting issue. Thanks for the explanation!
Fix the performance degradation due to different schedule execution and
SubscriptionList.add() and thread unparking.
This PR partially reverts some changes from earlier scheduler optimizations and fixes a case where if multiple concurrent schedule() calls happen, the order in the SubscriptionList might be different from the actual execution order which degrades performance on task termination due to remove() being O(n).
This might be the source of degradation in #2857 as well.
I'll post the
ComputationSchedulerPerf
results later.