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.

Sign up for GitHub

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

Jump to bottom

Convert simple uses of WorkerThreadPool to parallel for_range() #79490

Closed
wants to merge 1 commit into from
Closed

Convert simple uses of WorkerThreadPool to parallel for_range() #79490

wants to merge 1 commit into from

Conversation

myaaaaaaaaa
Copy link
Contributor

Simplifies threading compared to direct usage of WorkerThreadPool, since there is no longer a need for intermediate functions or structs.

image_compress_cvtt() may experience performance regressions until #72716 is merged.

This has not been tested. Tread with caution.

@myaaaaaaaaa myaaaaaaaaa requested review from a team as code owners July 14, 2023 21:37
@Calinou Calinou added this to the 4.x milestone Jul 14, 2023
@YuriSizov YuriSizov requested a review from RandomShaper July 17, 2023 19:25
@RandomShaper RandomShaper mentioned this pull request Jul 27, 2023
Closed
RandomShaper
RandomShaper reviewed Jul 27, 2023
View reviewed changes
core/object/worker_thread_pool.h
@@ -204,6 +204,10 @@ class WorkerThreadPool : public Object {

template <typename F>
static _FORCE_INLINE_ void for_range(int i_begin, int i_end, bool parallel, String name, F f) {
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but now I've realized the naming convention of arguments code be a bit mroe strongly followed here...

RandomShaper
RandomShaper approved these changes Jul 27, 2023
View reviewed changes
Copy link
Member

@RandomShaper RandomShaper 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 I already said I'm loving all this parallel for refactor.

@RandomShaper
Copy link
Member

Regarding the review, everything looks correct to me. Let's test it in the wild.

clayjohn
clayjohn reviewed Jul 27, 2023
View reviewed changes
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

This PR warrants some discussion with the production team as it strongly encourages the use of lambdas. Currently we discourage the use of lambdas in the codebase except for in a few key places.

@RandomShaper
Copy link
Member

Other for_range PRs have been already merged. Of course, feel free to discuss the matter, but I'm sure you'll come to the conclusion that this is one of those key cases that should be considered. 😁 I myself have used lambdas in other PRs where they avoided the need for the boilerplate of adding a one or two-lined static member functiton because of syntactic needs. That said, this for-range use case is even more natural, as lambdas allow to get closer to first-class support in the language (similar to what OMP makes possible).

@clayjohn
Copy link
Member

for_range

Ah, I see I was mistaking this PR for #72784 which was already merged awhile ago. Oh well, I'll defer to your judgement here

@clayjohn clayjohn modified the milestones: 4.x, 4.2 Jul 27, 2023
@myaaaaaaaaa myaaaaaaaaa mentioned this pull request Jul 27, 2023
Merged
@YuriSizov YuriSizov modified the milestones: 4.2, 4.x Aug 1, 2023
@myaaaaaaaaa myaaaaaaaaa mentioned this pull request Aug 7, 2023
Merged
@myaaaaaaaaa myaaaaaaaaa deleted the forrange-misc branch August 16, 2023 16:23
@AThousandShips AThousandShips removed this from the 4.x milestone Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clayjohn clayjohn clayjohn left review comments

@RandomShaper RandomShaper RandomShaper approved these changes

Assignees
No one assigned
Labels
archived enhancement needs testing topic:core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@myaaaaaaaaa @RandomShaper @clayjohn @Calinou @akien-mga @YuriSizov @AThousandShips