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

Revert "Implement parallel foreach() for easier multithreading" #79953

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Jul 27, 2023

Reverts #72784. The WTP changes will be reverted until the topic can be discussed in its entirety, with some possibly coming back when there's a stronger agreement.

(CC @myaaaaaaaaa)

@RandomShaper RandomShaper added this to the 4.2 milestone Jul 27, 2023
@RandomShaper RandomShaper requested review from a team as code owners July 27, 2023 11:48
@YuriSizov YuriSizov merged commit 4e22ce8 into godotengine:master Jul 27, 2023
@YuriSizov
Copy link
Contributor

Thanks! Let's discuss this more thoroughly (on the contributors chat, preferably) and find a good solution that we can implement in one go. I think readability improvements from this change were great, but we need to make sure it doesn't come at a cost of confusion or, worse, performance.

@RandomShaper RandomShaper deleted the revert_wtp_for_range branch July 27, 2023 15:14
@myaaaaaaaaa
Copy link
Contributor

...find a good solution that we can implement in one go.

I generally prefer to approach refactors and changes to core in small commits, since it lets each change be tested in isolation, makes things easier for reviewers,1 and makes any potential regressions easier to bisect.

But I see that doing things this way created its own problems by making things overly confusing to keep track of.

In that case, would you be willing to accept a PR with multiple commits (obviously with each individual commit passing CI)? This way, reviewability and bisectability are maintained, while making it easier to keep track of the big picture under one PR.

This is assuming of course that parallel_for() is still a desired feature, since reduz seemed particularly against it.

Footnotes

  1. Huge squashed commits are generally discouraging to review and get delayed a lot in practice. Refactor main.cpp setup #49362 is possibly the biggest victim of this.

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 4, 2023

But I see that doing things this way created its own problems by making things overly confusing to keep track of.

Well, it's more about the snowballing effect. We merge one change that looks okay-ish, and it breaks stuff, and so we patch it, and it potentially breaks more stuff. And each solution becomes progressively more hacky, making the whole thing much less appropriate and much less likely to pass the quality control for the core. Basically, after fixups we end up with a solution that would not be accepted to begin with.

So to avoid that we need to arrive to a well-vetted solution before implementing it.

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.

3 participants