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

4.x: Make Scheduler.Recursive more uniform and allocate less #617

Merged
merged 3 commits into from
Jun 20, 2018

Conversation

akarnokd
Copy link
Collaborator

@akarnokd akarnokd commented Jun 19, 2018

This PR unifies the Action-based recursive scheduler helper by capturing all state into an instance and managing the recursive calls from there.

The original also allocated at least the scheduler.Schedule(state2, (scheduler1, state3) => part due to the state capture of all those variables.


recursiveAction(tuple.state);
protected void SetDisposable(long idx, IDisposable d)
Copy link
Collaborator

Choose a reason for hiding this comment

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

That stuff with the long indexes is really ugly IMO. I don't particularly like these solutions that will eventually overflow, even if it's not in our lifetime. Can we keep the CompositeDisposable?

Copy link
Collaborator Author

@akarnokd akarnokd Jun 19, 2018

Choose a reason for hiding this comment

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

There are usually 1-2 entries in the map, the previous and the next schedule's disposable. These can't get further away than 1 so even if the counter wraps, that doesn't matter because they would still go into different slots.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To make it wrap the increment should be unchecked?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really know. To keep using the CompositeDisposable it would take the allocation of a SingleAssignmentDisposable, but aren't we saving a ton here anyway?

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'm for the maximum saving.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get this but I don't really like the approach with those weak identities. I am not convinced it's good code. I see it'll be working for a gazillion of years but still, I have to think about that. I'd rather have something that you can compare by reference.

@danielcweber
Copy link
Collaborator

I'm really sorry if the introduction of SingleAssignmentDisposables is just another case of you 'giving up', and I would not want it to be that way. I have elaborated on my objectives on Slack. I hope you understand.

@akarnokd
Copy link
Collaborator Author

The main problem is that the review creates ambiguities about how to proceed due to being formed as questions and eventually leading to an unconvincable state on the side of the reviewer.

@danielcweber
Copy link
Collaborator

Point taken. That's just me not wanting to tell you what to do.

@akarnokd
Copy link
Collaborator Author

It is a matter of decision and taking responsibility for those decisions and the subsequent changes requested by a reviewer. Being a collaborator with merge rights is an elevated position that requires clear view on what should and shouldn't be done and if necessary, explain why. Posing such responses though as more and more questions that create an illusion that the contributor can change the reviewer's mind will lead to confusion, and perhaps even worse, self-censorship.

@danielcweber
Copy link
Collaborator

Well, as I said, point taken. So let me state my view: I think that the solution with ulongs is too brittle in theory (albeit, as we have already discussed, not in practice) to justify the saving of an allocation, therefore I would prefer the SingleAssignmentDisposable solution.

@danielcweber danielcweber merged commit e2092e1 into dotnet:master Jun 20, 2018
@akarnokd akarnokd deleted the RecursiveScheduleImprovements branch June 21, 2018 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants