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

Fix the performance degradation due to different schedule execution and #2912

Merged

Conversation

akarnokd
Copy link
Member

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.

@akarnokd
Copy link
Member Author

Benchmark      (size)        2912   2912 error       1.0.9  1.0.9 error
observeOn           1  198785,487    59066,518  203658,175     2162,208
observeOn          10  153990,002    79263,702  178839,814    17425,416
observeOn         100  150333,481    20749,482  148706,965    15169,371
observeOn        1000   25568,884    24718,878   16911,025     7937,752
observeOn        2000   14914,879    17179,009   10022,748    12460,449
observeOn        3000   11238,118     2400,207   10359,226     2868,300
observeOn        4000    8172,019     1370,653    7084,573     1460,569
observeOn       10000    3491,021      606,603    3412,501      527,199
observeOn      100000     398,684       20,607     400,360       14,743
observeOn     1000000      41,865        1,884      43,887        0,746
subscribeOn         1  209443,261     8593,857  169965,326   104264,868
subscribeOn        10  175152,648    79449,091  151055,057   152837,825
subscribeOn       100  150042,891    84189,199  140177,447   128390,130
subscribeOn      1000   72804,521    29222,089   57929,258    19364,337
subscribeOn      2000   61834,990    14148,419   36040,899     8608,854
subscribeOn      3000   44148,878     4653,713   44982,855     3551,646
subscribeOn      4000   35494,660     3338,209   30343,790    24747,587
subscribeOn     10000   14890,534     6229,533   13920,062     1897,708
subscribeOn    100000    1691,786      230,978    1661,187      141,518
subscribeOn   1000000     193,028       22,564     191,835       16,975

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);
Copy link
Member

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:

Copy link
Member Author

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.

Copy link
Member

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!

benjchristensen added a commit that referenced this pull request Apr 30, 2015
Fix the performance degradation due to different schedule execution and
@benjchristensen benjchristensen merged commit 2532484 into ReactiveX:1.x Apr 30, 2015
@benjchristensen benjchristensen mentioned this pull request Apr 30, 2015
@akarnokd akarnokd deleted the FixEventLoopsPerfDegradation branch May 6, 2015 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants