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 and optimize gatherUnordered #201

Merged
merged 2 commits into from Aug 8, 2016

Conversation

Projects
None yet
2 participants
@alexandru
Copy link
Member

alexandru commented Aug 7, 2016

Fixes and optimizes gatherUnordered.

The gatherUnordered is not stack-safe in its onSuccess handling. It's really a corner case, but significant in that no Task operation should be stack unsafe.

As for the optimization, adding subscriptions to ourCompositeCancelable one by one is inneficient because needs these two ops:

  • addAll(that: TranversableOnce[Cancelable])
  • removeAll(that: TranversableOnce[Cancelable])

This is because in case we want to add a whole list of cancelables to a composite, adding them one by one generates compareAndSet calls on its internal atomic state for each and every reference added, which is very wasteful. Task.gatherUnordered now uses the new addAll operation.

I also introduced a benchmark. The results are quite revealing:

[info] Benchmark                        Mode  Cnt    Score    Error  Units
[info] TaskGatherBenchmark.gatherA     thrpt   10  107.381 ±  2.937  ops/s
[info] TaskGatherBenchmark.gatherS     thrpt   10   96.838 ±  3.303  ops/s
[info] TaskGatherBenchmark.sequenceA   thrpt   10  220.651 ±  9.420  ops/s
[info] TaskGatherBenchmark.sequenceS   thrpt   10  615.164 ± 14.341  ops/s
[info] TaskGatherBenchmark.unorderedA  thrpt   10  153.785 ± 13.647  ops/s
[info] TaskGatherBenchmark.unorderedS  thrpt   10  383.674 ± 28.970  ops/s

In other words gatherUnordered is dramatically better than gather, having an increase in throughput between 50% and 300%. And sequence is better than both, but that was to be expected for the cheap tasks that we are using.

@alexandru alexandru added this to the 2.0 milestone Aug 7, 2016

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 8, 2016

Current coverage is 83.67% (diff: 89.65%)

Merging #201 into master will decrease coverage by 0.18%

@@             master       #201   diff @@
==========================================
  Files           203        203          
  Lines          7085       7107    +22   
  Methods        5981       6001    +20   
  Messages          0          0          
  Branches       1043       1045     +2   
==========================================
+ Hits           5942       5947     +5   
- Misses         1143       1160    +17   
  Partials          0          0          

Powered by Codecov. Last update 463cc7e...6df3e4b

@alexandru alexandru changed the title Add addAll and removeAll ops to CompositeCancelable Fix and optimize gatherUnordered Aug 8, 2016

@alexandru alexandru self-assigned this Aug 8, 2016

@alexandru alexandru merged commit 61b8abd into master Aug 8, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@alexandru alexandru deleted the composite-changes branch Nov 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.