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: Improve Amb #512

Merged
merged 1 commit into from
May 26, 2018
Merged

4.x: Improve Amb #512

merged 1 commit into from
May 26, 2018

Conversation

akarnokd
Copy link
Collaborator

This PR improves the Amb operator by reducing allocations and making it lock-free.

Note that the algorithm can be extended to Amb any number of sources.

@danielcweber
Copy link
Collaborator

Will that work with my changes in #493?

@akarnokd
Copy link
Collaborator Author

Nope. This is a self-contained change and #493 is just simply too widespread. #493 will conflict with any and all PRs targeting individual operators. So the problem isn't the number of commits per PR but the number of files affected.

@danielcweber
Copy link
Collaborator

So what would you suggest how to proceed?

@akarnokd
Copy link
Collaborator Author

It depends on who's PR gets merged first. #493 requires more extensive review work than this PR.

@danielcweber
Copy link
Collaborator

So we're making that a competition now?

@akarnokd
Copy link
Collaborator Author

I'd say, without active and frequent review/merge persons, it is a risk to do widespread changes.

It is also risky to have multiple PRs open because in the worst case, merging one of them requires the rebasing or even redoing of all the rest, again and again.

If I were you, I would've done the fundamental sink changes with minimal surface API changes and have each operator retrofitted in its own PR.

@danielcweber
Copy link
Collaborator

The Sink changes are not fundamental. You had a look into it, they mainly reduce repetitive code. The widespread changes are due to that reduction of repetition. There is no re-implementation of anything whatsoever. There is no change in surface API.

@danielcweber
Copy link
Collaborator

danielcweber commented May 17, 2018

Why you would choose to possibly conflict with PRs that cleanup, clarify, reduce repetitive code in favor of complete reimplementations, new concepts (like 'Coordinator') at the risk of redoing it all at this point in time is beyond me, especially since you reviewed my PR thoroughly. Maybe you want to "collaborate" here completely on your own?

@akarnokd
Copy link
Collaborator Author

How do you think collaboration would work if there is an unmerged large PR that prevents any other PR from getting through because of conflicts? In my previous PRs, I've tried to skirt around #493 as much as possible to avoid merge conflicts, but there is a point where given one contributor's available time, holding back on changes may never get those changes in code form as the contributor simply gives up on the project.

@danielcweber
Copy link
Collaborator

Talking about a contributor's available time, now, as it is, at least somebody's time and effort will be partially wasted either or. I would completely agree if it was really the size or complexity of the PR that would block it. Instead, we're just in a period of transition and it will happen, I'm sure. Meanwhile, we could collabratively work out how to bring this forward without clashing, I think there's plenty of ways, some of them I already even proposed to you.

@clairernovotny
Copy link
Member

I'm merging in everything I can now.... #493 will require some review and conflict resolution now. Happy to get that in once ready. There's quite a few changes here that can justify a 4.1 fairly soon.

@clairernovotny clairernovotny merged commit c961295 into dotnet:master May 26, 2018
@akarnokd akarnokd deleted the AmbImprovements branch May 26, 2018 14:58
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.

3 participants