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

A small improvement to the parallel_for when task count > thread count #1839

Merged
merged 2 commits into from
Sep 16, 2019

Conversation

snnn
Copy link
Member

@snnn snnn commented Sep 13, 2019

Description:

A small improvement to the parallel_for when task count > thread count

Before this change: The recommended thread pool size is number of cpu cores minus one.
After this change: The recommended thread pool size is number of cpu cores

The new code is more like the parallelFor in Eigen::ThreadPoolDevice. Especially the " if (total > NumThreads()) " logic is the same.

Motivation and Context

  • Why is this change required? What problem does it solve?

If you have 4 cores, and 4 tasks, that's fine.
But if you have 4 cores, but 8 tasks, if one task need 1 unit time, the old code need 3 unit time, but the new code only need 2.

  • If it fixes an open issue, please link to the issue here.

Related PR:
#947
#1646

@snnn snnn requested a review from a team as a code owner September 13, 2019 22:37
@snnn snnn merged commit 166b1f8 into master Sep 16, 2019
@snnn snnn deleted the snnn/tp8 branch September 16, 2019 17:44
snnn added a commit that referenced this pull request Sep 17, 2019
@snnn snnn mentioned this pull request Sep 17, 2019
snnn added a commit that referenced this pull request Sep 24, 2019
snnn added a commit that referenced this pull request Sep 24, 2019
snnn added a commit that referenced this pull request Mar 12, 2020
1. Do not reuse the main thread.
2. Do not plus one when mlas calculate the number of tasks to schedule. (It was me put the plus one there)

This is the second try of #1839

It's known that this change has negative performance impact on some of the models.
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