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

perf(ReplaySubject): slightly improved performance #2677

Merged
merged 5 commits into from
Mar 30, 2018

Conversation

martinsik
Copy link
Contributor

I was going through the ReplaySubject class and I think in some situations it can be simplified. Most importantly when I don't set the timeWindow parameter the buffer trimming method can be avoided completely and I can store just the values without wrapping them with ReplayEvent.

Also, I tried some performance tests to see whether splice() or shift() would be better. It seems that running shift() in a for loop is faster than splice() when removing < 25 items. However, splice() is more consistent even when removing a large portion of an array. See https://jsperf.com/splice-vs-shift-2

There were no benchmarks for Subjects in perf directory so I made a few myself but I'm not sure I'm testing it the right way. It shows that RxJS 4 implementation was faster than RxJS 5...

Before:

                                         |                     RxJS 4.1.0 |                     RxJS 5.4.1 |          factor |      % improved
---------------------------------------------------------------------------------------------------------------------------------------------------
    replaysubject_windowtime - immediate |                68,591 (±1.36%) |                17,151 (±2.57%) |           0.25x |          -75.0%
               replaysubject - immediate |                72,951 (±1.08%) |                17,099 (±2.69%) |           0.23x |          -76.6%
                replaysubject_windowtime |                39,104 (±0.85%) |                15,393 (±2.85%) |           0.39x |          -60.6%
                           replaysubject |                34,721 (±1.13%) |                15,393 (±2.30%) |           0.44x |          -55.7%

After:

                                        |                     RxJS 4.1.0 |                     RxJS 5.4.1 |          factor |      % improved
---------------------------------------------------------------------------------------------------------------------------------------------------
    replaysubject_windowtime - immediate |                63,306 (±1.14%) |                19,745 (±2.68%) |           0.31x |          -68.8%
               replaysubject - immediate |                63,870 (±3.35%) |                28,659 (±2.49%) |           0.45x |          -55.1%
                replaysubject_windowtime |                36,607 (±1.78%) |                16,646 (±2.83%) |           0.45x |          -54.5%
                           replaysubject |                36,058 (±1.29%) |                22,130 (±2.86%) |           0.61x |          -38.6%

@rxjs-bot
Copy link

rxjs-bot commented Jun 19, 2017

Messages
📖

CJS: 1362.3KB, global: 729.4KB (gzipped: 117.9KB), min: 140.9KB (gzipped: 30.7KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 97.723% when pulling a4e146e on martinsik:perf-replaySubject into 4694a27 on ReactiveX:master.

@kwonoj
Copy link
Member

kwonoj commented Jun 19, 2017

lol I need to update size diff output

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

Please verify that the loop with shift is faster than the splice

if (spliceCount > 0) {
_events.splice(0, spliceCount);
for (let i = 0; i < spliceCount; i++) {
_events.shift();
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually shocked this would be faster. this section was O(1) now it's O(n).

Copy link
Member

Choose a reason for hiding this comment

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

After reviewing perf, i think we should return it to splice because of it's consistent performance, even if shift is faster on smaller arrays.

@benlesh
Copy link
Member

benlesh commented Jun 19, 2017

@martinsik the change looks fantastic, just one thing I'd like you to verify if possible.

I suspect that shift() is faster for 1 or 2 items, but splice is faster for more than that. If that's the case, we just need to accommodate the 80% case and move on (with a comment to note why it's a loop and not a splice)

@martinsik
Copy link
Contributor Author

@benlesh I made this test to see which one is faster: https://jsperf.com/splice-vs-shift-2
It seems like even this:

for (var i = 0; i < 25; i++) {
    arr.shift();
}

is faster than this:

arr.splice(0, 25);

I don't know if there're any other aspects to consider. I guess size of the array shouldn't matter but maybe there are some specific cases when it does?

@benlesh
Copy link
Member

benlesh commented Jun 20, 2017

Yeah, it looks like what I suspected. Which is that for small arrays shift() will be more efficient, but for larger arrays splice() wins out (the 50 vs 50 tests show a 2x perf increase for splice)

While there's some serious wins for the smaller arrays, I think they die off quickly enough that it's worth looking at the more consistent (and better for a larger range of sizes) performance of splice.

I think we should move it back to splice to keep performance consistent.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 97.723% when pulling 43f8bac on martinsik:perf-replaySubject into 4694a27 on ReactiveX:master.

@coveralls
Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage decreased (-0.01%) to 97.723% when pulling a4e146e on martinsik:perf-replaySubject into 4694a27 on ReactiveX:master.

@martinsik
Copy link
Contributor Author

@benlesh I understand, I switched back to splice().

I'm not sure about the performance test I made. It shows that RxJS 4 is faster than RxJS 5 but is this even possible?

@martinsik
Copy link
Contributor Author

@benlesh I just want to ask if this PR is still relevant or it needs updates for 5.5 or 6.0 :).

@benlesh benlesh merged commit 9fea36d into ReactiveX:master Mar 30, 2018
@lock
Copy link

lock bot commented Jun 5, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants