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: use shared, static observables to reduce allocations #1289

Merged
merged 2 commits into from Feb 24, 2017

Conversation

Projects
None yet
3 participants
@kentcb
Copy link
Contributor

kentcb commented Feb 24, 2017

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Performance improvement.

What is the current behavior? (You can also link to an open issue here)

#1156

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

Other information:

Let's allocate less!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 24, 2017

Coverage Status

Coverage increased (+0.06%) to 66.782% when pulling edddd94 on kentcb:observables into 67f10b0 on reactiveui:develop.

@kentcb kentcb added this to the vNext milestone Feb 24, 2017

@kentcb kentcb changed the title Use static observables to reduce allocations perf: use static observables to reduce allocations Feb 24, 2017

@kentcb kentcb changed the title perf: use static observables to reduce allocations perf: use shared, static observables to reduce allocations Feb 24, 2017

@@ -72,6 +72,7 @@
<Compile Include="MessageBus.cs" />
<Compile Include="SuspensionHost.cs" />
<Compile Include="ObservableAsPropertyHelper.cs" />
<Compile Include="Observables.cs" />

This comment has been minimized.

@olevett

olevett Feb 24, 2017

Member

This saddens me (the fact that we need to do it everywhere :()

This comment has been minimized.

@kentcb

kentcb Feb 24, 2017

Author Contributor

Totally! Bring on VS17.

This comment has been minimized.

@olevett

olevett Feb 24, 2017

Member

It's doable with shared projects now...probably not worth the faff of doing it twice though!

/// <summary>
/// Provides commonly required, statically-allocated, pre-canned observables.
/// </summary>
public static class Observables

This comment has been minimized.

@olevett

olevett Feb 24, 2017

Member

Should this be internal (like the Observable<T> class)?

This comment has been minimized.

@kentcb

kentcb Feb 24, 2017

Author Contributor

It should! Thanks - I copied this from Genesis.Observables and forgot to change both.

/// </summary>
/// <remarks>
/// <para>
/// This observable is equivalent to <c>Observable&lt;bool&gt;.Default</c>, but is provided for convenience.

This comment has been minimized.

@olevett

olevett Feb 24, 2017

Member

Is there a reason to Default rather than Observable.Return(false) don't think it really matters, but it seems a little odd that it's different for the true/false cases.

This comment has been minimized.

@kentcb

kentcb Feb 24, 2017

Author Contributor

Probably a completely pointless optimization. That is, if it's Observable.Return(false) then someone access Observable<bool>.Default, then we've cached two instead of one. For the sake of readability, I will change this.

This comment has been minimized.

@olevett

olevett Feb 24, 2017

Member

That does make sense, hadn't quite spotted that!

@coveralls

This comment has been minimized.

Copy link

coveralls commented Feb 24, 2017

Coverage Status

Coverage increased (+0.06%) to 66.782% when pulling b3a01d6 on kentcb:observables into 67f10b0 on reactiveui:develop.

@kentcb kentcb merged commit f766ddd into reactiveui:develop Feb 24, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.06%) to 66.782%
Details
@kentcb

This comment has been minimized.

Copy link
Contributor Author

kentcb commented Feb 24, 2017

Thanks for the review @olevett!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment