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.

Sign up for GitHub

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

Jump to bottom

Save allocations in Zip #635

Merged
merged 3 commits into from
Jun 25, 2018
Merged

Save allocations in Zip #635

merged 3 commits into from
Jun 25, 2018

Conversation

danielcweber
Copy link
Collaborator

A lot of SingleAssignmentDisposables can be saved in Zip.

@clairernovotny
Copy link
Member

Can we get BenchmarkDotnet wired up for these? There's quite a few perf optimizations and it'd be useful to have a before/after that shows the difference.

@danielcweber
Copy link
Collaborator Author

That would be very cool, I'll have a look into this.

@danielcweber
Copy link
Collaborator Author

Would you prefer some local benchmark results (as some kind of proof) in the PR or rather have the benchmarks part of the CI build ?

@danielcweber
Copy link
Collaborator Author

Here we go. Again, we don't see that big a boost here, but another benefit of less allocations is that GC may kick in later and less often, which is not reflected in this benchmark.

Original:

                       Method |       Mean |     Error |    StdDev | 
----------------------------- |-----------:|----------:|----------:|
            Zip_AllCompleted2 |   8.352 us | 0.1553 us | 0.1377 us |
            Zip_AllCompleted3 |  11.626 us | 0.1709 us | 0.1515 us |
            Zip_AllCompleted4 |  14.901 us | 0.2862 us | 0.2811 us |
            Zip_AllCompleted5 |  18.526 us | 0.3624 us | 0.3026 us |
            Zip_AllCompleted6 |  24.086 us | 0.4448 us | 0.4161 us |
            Zip_AllCompleted7 |  28.698 us | 0.5023 us | 0.4698 us |
            Zip_AllCompleted8 |  34.896 us | 0.6896 us | 0.6113 us |
            Zip_AllCompleted9 |  41.483 us | 0.7713 us | 0.7575 us |
           Zip_AllCompleted10 |  50.294 us | 0.4446 us | 0.3941 us |
           Zip_AllCompleted11 |  57.110 us | 0.7622 us | 0.7130 us |
           Zip_AllCompleted12 |  66.456 us | 1.3266 us | 1.8159 us |
           Zip_AllCompleted13 |  73.969 us | 1.2653 us | 1.1836 us |
           Zip_AllCompleted14 |  86.634 us | 1.3661 us | 1.2778 us |
           Zip_AllCompleted15 |  96.360 us | 1.8158 us | 1.7834 us |
           Zip_AllCompleted16 | 111.756 us | 2.2157 us | 2.1761 us |


Less allocations:
                       Method |       Mean |     Error |    StdDev |
----------------------------- |-----------:|----------:|----------:|
            Zip_AllCompleted2 |   8.069 us | 0.1520 us | 0.1690 us |
            Zip_AllCompleted3 |  11.132 us | 0.1231 us | 0.1028 us |
            Zip_AllCompleted4 |  14.793 us | 0.2242 us | 0.2097 us |
            Zip_AllCompleted5 |  18.540 us | 0.3074 us | 0.2875 us |
            Zip_AllCompleted6 |  23.367 us | 0.4649 us | 0.4774 us |
            Zip_AllCompleted7 |  29.251 us | 0.3931 us | 0.3677 us |
            Zip_AllCompleted8 |  35.450 us | 0.6922 us | 0.6798 us |
            Zip_AllCompleted9 |  42.327 us | 0.8452 us | 1.0061 us |
           Zip_AllCompleted10 |  49.990 us | 0.9474 us | 1.0137 us |
           Zip_AllCompleted11 |  56.294 us | 0.8105 us | 0.6768 us |
           Zip_AllCompleted12 |  66.008 us | 1.3048 us | 1.1567 us |
           Zip_AllCompleted13 |  77.102 us | 0.8779 us | 0.8212 us |
           Zip_AllCompleted14 |  86.705 us | 1.6075 us | 1.4250 us |
           Zip_AllCompleted15 |  95.842 us | 1.5038 us | 1.4067 us |
           Zip_AllCompleted16 | 109.378 us | 1.7171 us | 1.6062 us |

@clairernovotny
Copy link
Member

I'm not sure that benchmarks in a CI build would be really useful given how much it depends on the machine load. I think having it in the PR as a reference is helpful though

@danielcweber
Copy link
Collaborator Author

Agreed, and builds would take forever.

Do you have experience with interpreting the benchmarks? Comparison of the mean is not so impressive but the standard deviation would suggest that the measurements are much less flaky, maybe that's the GC kicking in less frequently?

@clairernovotny
Copy link
Member

I would nerd-snipe @benaadams here

benaadams
benaadams reviewed Jun 21, 2018
View reviewed changes
Rx.NET/Source/benchmarks/Benchmarks.System.Reactive/ZipBenchmark.cs Outdated

namespace Benchmarks.System.Reactive
{
public class ZipBenchmark
Copy link
Member

Choose a reason for hiding this comment

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

Add the attribute [MemoryDiagnoser]

Copy link
Member

Choose a reason for hiding this comment

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

The run again and it should show the allocations

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@benaadams Awesome, thanks!

@danielcweber
Copy link
Collaborator Author

Results with MemoryDiagnoser:

Original:

                       Method |       Mean |     Error |    StdDev |   Gen 0 | Allocated |
----------------------------- |-----------:|----------:|----------:|--------:|----------:|
            Zip_AllCompleted2 |   7.697 us | 0.0468 us | 0.0437 us | 18.1732 |  37.29 KB |
            Zip_AllCompleted3 |  10.558 us | 0.0739 us | 0.0655 us | 19.5007 |  40.08 KB |
            Zip_AllCompleted4 |  13.607 us | 0.0438 us | 0.0388 us | 20.8282 |  42.76 KB |
            Zip_AllCompleted5 |  16.860 us | 0.1704 us | 0.1594 us | 22.2168 |  45.77 KB |
            Zip_AllCompleted6 |  21.847 us | 0.1523 us | 0.1424 us | 23.8953 |  49.17 KB |
            Zip_AllCompleted7 |  26.215 us | 0.1332 us | 0.1246 us | 25.6348 |  52.89 KB |
            Zip_AllCompleted8 |  32.573 us | 0.4329 us | 0.3615 us | 27.7710 |  56.95 KB |
            Zip_AllCompleted9 |  38.065 us | 0.2881 us | 0.2406 us | 29.8462 |  61.35 KB |
           Zip_AllCompleted10 |  45.841 us | 0.1505 us | 0.1334 us | 32.2266 |  66.13 KB |
           Zip_AllCompleted11 |  51.955 us | 0.3339 us | 0.2960 us | 34.7290 |  71.28 KB |
           Zip_AllCompleted12 |  59.543 us | 0.3348 us | 0.3132 us | 37.3535 |  76.71 KB |
           Zip_AllCompleted13 |  68.253 us | 0.8254 us | 0.7317 us | 39.9170 |  82.49 KB |
           Zip_AllCompleted14 |  79.642 us | 0.6238 us | 0.5835 us | 43.0908 |  88.61 KB |
           Zip_AllCompleted15 |  87.353 us | 0.4121 us | 0.3653 us | 46.2646 |  95.05 KB |
           Zip_AllCompleted16 | 101.971 us | 0.5110 us | 0.4530 us | 49.5605 | 101.81 KB |

Less allocations:

                       Method |       Mean |     Error |    StdDev |   Gen 0 | Allocated |
----------------------------- |-----------:|----------:|----------:|--------:|----------:|
            Zip_AllCompleted2 |   7.513 us | 0.0460 us | 0.0407 us | 18.1732 |  37.29 KB |
            Zip_AllCompleted3 |  10.813 us | 0.0502 us | 0.0445 us | 19.4092 |   39.9 KB |
            Zip_AllCompleted4 |  13.804 us | 0.0682 us | 0.0638 us | 20.7214 |  42.57 KB |
            Zip_AllCompleted5 |  17.298 us | 0.0894 us | 0.0793 us | 22.2168 |  45.56 KB |
            Zip_AllCompleted6 |  22.050 us | 0.1599 us | 0.1495 us | 23.8037 |  48.95 KB |
            Zip_AllCompleted7 |  26.669 us | 0.1567 us | 0.1466 us | 25.6348 |  52.72 KB |
            Zip_AllCompleted8 |  33.237 us | 0.2590 us | 0.2423 us | 27.5269 |  56.66 KB |
            Zip_AllCompleted9 |  39.601 us | 0.4323 us | 0.3832 us | 29.7241 |  61.02 KB |
           Zip_AllCompleted10 |  46.347 us | 0.1650 us | 0.1378 us | 32.0435 |  65.81 KB |
           Zip_AllCompleted11 |  52.834 us | 0.2213 us | 0.1962 us | 34.4238 |   70.9 KB |
           Zip_AllCompleted12 |  60.607 us | 0.2722 us | 0.2273 us | 36.9873 |  76.33 KB |
           Zip_AllCompleted13 |  70.993 us | 0.3774 us | 0.3346 us | 39.9170 |  82.24 KB |
           Zip_AllCompleted14 |  79.750 us | 0.3760 us | 0.3333 us | 42.9688 |   88.2 KB |
           Zip_AllCompleted15 |  90.132 us | 0.5021 us | 0.4696 us | 46.1426 |  94.59 KB |
           Zip_AllCompleted16 | 101.322 us | 0.5323 us | 0.4979 us | 49.3164 | 101.33 KB |

Allocation performance has improved, time performance has not.

@danielcweber
Copy link
Collaborator Author

@cabauman thx for review and welcome!

@cabauman
Copy link
Contributor

Thanks @danielcweber! I haven't posted much in the rxnet slack room but I've been observing from a distance for quite some time. Love what you guys are doing. At the very least, I'll make it a point to do some more reviews.

@danielcweber
Let ZipObserver inherit from SafeObserver which inlines the SingleAss…
b1461b3 
…ignmentDisposables currently used. We save n allocations for the n-ary Zip.
@danielcweber
Override Dispose in ZipObserver so it can clear its own queue, that s…
96c4cca 
…aves the allocation of an anonymous disposable, a closure and a delegate.
@danielcweber
Merge branch 'master' into LessAllocationsInZip
d441740
@danielcweber danielcweber merged commit 70d2a05 into dotnet:master Jun 25, 2018
@danielcweber danielcweber deleted the LessAllocationsInZip branch June 25, 2018 09:59
@akarnokd akarnokd mentioned this pull request Jul 24, 2018
Closed
@quinmars quinmars mentioned this pull request Oct 8, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benaadams benaadams benaadams left review comments

@cabauman cabauman cabauman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@danielcweber @clairernovotny @cabauman @benaadams