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

Reduce allocations for anonymous observables #517

Closed
wants to merge 3 commits into from
Closed

Reduce allocations for anonymous observables #517

wants to merge 3 commits into from

Conversation

danielcweber
Copy link
Collaborator

@danielcweber danielcweber commented May 18, 2018

Almost every use of AnonymousObservable captures some variables in a closure and creates a delegate. This PR works around that by passing state.

Note: This is based on #515 and #518. These should be merged before this PR.

Copy link
Collaborator

@akarnokd akarnokd left a comment

Choose a reason for hiding this comment

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

I think more allocations could be saved by dedicated classes instead of delegates.

d.Disposable = m;

m.Disposable = scheduler.Schedule(() =>
return AnonymousObservable<TSource>.CreateStateful(
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about not using a delegate either but extending some base class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All this can be considered, but it would make review and merging at this point even more complicated. Instead I tried to keep replacements straightforward and easy to review.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess in the end, there will be no justification to use AnonymousObservable from inside Rx any more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is possible that this or some other class will be re-optimized in the future, depending on how the project leads decide upon critical components, which would lead to wasted "small step changes" on our parts. Just say'n.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AnonymousX in Rx were a workaround because C# still doesn't support anonymous inner classes like Java does and the devs wasn't apparently very keen on just defining new dedicated classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just say'n.

Don't really know what that's supposed to mean again. Try to encourage people for a change.

…Action<TState> and pass a corresponding state object. The ability to pass state will greatly help to reduce the allocations of closures.
…meter to the subscription function. This will allow the avoidance of closure allocations and enable more delegate caching where AnonymousObservables are created.
… function at various places, resulting in some good savings on closure allocations and delegate caching everywhere!
@danielcweber
Copy link
Collaborator Author

Overridden by #549 if revised and approved.

@danielcweber
Copy link
Collaborator Author

Obsoleted by #549.

@danielcweber danielcweber deleted the ReduceAllocationsForAnonymousObservables branch May 30, 2018 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants