perf(bufferCount): optimize bufferCount operator

[martinsik]

The bufferCount() operator can be used as bufferCount(5, 3) or bufferCount(5) (= bufferCount(5, 5).

My assumption is that the second use-case bufferCount(5) is very common (maybe even more common that the first one) and can be handled in an simpler way. This means instead of keeping an array of buffers T[][] we can have just a single buffer T[]. This simplifies the subscriber class significantly and runs faster.

For this reason I split the original subscriber into two classes BufferSkipCountSubscriber and BufferCountSubscriber each handling one use-case.

Performance of bufferCount(5):

Before:

$ node perf/micro buffercount 
                                         |                     RxJS 4.1.0 |                     RxJS 5.1.0 |          factor |      % improved
---------------------------------------------------------------------------------------------------------------------------------------------------
                 buffercount - immediate |                13,277 (±1.39%) |               308,516 (±2.25%) |          23.24x |        2,223.6%
                             buffercount |                 9,046 (±2.52%) |               143,636 (±2.19%) |          15.88x |        1,487.9%

After:

$ node perf/micro buffercount 
                                         |                     RxJS 4.1.0 |                     RxJS 5.1.0 |          factor |      % improved
---------------------------------------------------------------------------------------------------------------------------------------------------
                 buffercount - immediate |                13,205 (±1.02%) |               525,996 (±1.84%) |          39.83x |        3,883.2%
                             buffercount |                 9,330 (±2.55%) |               204,174 (±2.12%) |          21.88x |        2,088.4%

This improves performance on the buffercount - immediate script roughly from 23.24x to 39.83x and buffercount from 15.88x to 21.88x.

Performance of bufferCount(5, 3) remains the same. I could just remove one additional condition that wasn't necessary any more when we have two separate classes now.

I also tried to make an implementations using only fixed size arrays for both use-cases but these always turned out to be less efficient than the current implementation. It required calculating indices on every next(), at least one additional if condition and calling Array.split() to clone the buffer when calling destination.next(...). Using Array.push() without Array.split() was always faster.

[coveralls]

Coverage increased (+0.006%) to 97.695% when pulling dfd75dd on martinsik:optimize-buffercount into 31dfc73 on ReactiveX:master.

[trxcllnt]

Yay! Great work, thanks a lot @martinsik

[jayphelps]

Minor nits, otherwise looking good!

[jayphelps]

destination lookup used to be cached above, so it doesn't need to happen every time. Seemed like a simple win for micro perf?

[martinsik]

@jayphelps I removed the const because this.destination is used just once in this method. Maybe I don't understand what you mean.

[jayphelps]

@martinsik this is inside a loop, is it not possible for the branch inside to call destination.next() multiple times? Seems like it would be.

[benlesh]

@jayphelps is correct here, you'll want to access the property outside of the loop and put in in a const

[martinsik]

@Blesh @jayphelps Ahaaaa, I see it now :).

[jayphelps]

I would prefer if we don't increment inside a conditional. I understand it just fine, but I've seen it be unnecessarily confusing for many. Can we increment before we enter the conditional?

[martinsik]

@jayphelps done!

[coveralls]

Coverage decreased (-0.004%) to 97.686% when pulling 9233ec4 on martinsik:optimize-buffercount into 31dfc73 on ReactiveX:master.

[benlesh]

move property accessor outside of the loop.

[benlesh]

@jayphelps is correct here, you'll want to access the property outside of the loop and put in in a const

[coveralls]

Coverage increased (+0.005%) to 97.694% when pulling a35a723 on martinsik:optimize-buffercount into 31dfc73 on ReactiveX:master.

Same as my concerns... seem to have been met.

[benlesh]

Thank you @martinsik!

[lock]

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.