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

fix omp flag #871

Merged
merged 7 commits into from
Sep 29, 2020
Merged

Conversation

lidanqing-intel
Copy link

@lidanqing-intel lidanqing-intel commented Sep 27, 2020

machine:Linux i9

Compile option: WITH_MKL ON
mkldnn 0
cpu_math_library_num_threads 1

./ocr_system /home/li/repo/PaddleOCR/deploy/cpp_infer/tools/config.txt /home/li/repo/PaddleOCR/doc/imgs/12.jpg

花费了17.8732秒

mkldnn 0
cpu_math_library_num_threads 8

./ocr_system /home/li/repo/PaddleOCR/deploy/cpp_infer/tools/config.txt /home/li/repo/PaddleOCR/doc/imgs/12.jpg

花费了14.1399秒

Windiws machine Please help verify on i7 performance @OliverLPH

@lidanqing-intel
Copy link
Author

lidanqing-intel commented Sep 27, 2020

@wojtuss Please help review, Thanks

@lidanqing-intel
Copy link
Author

@luotao1 Can you help review this?

@lidanqing-intel
Copy link
Author

Solve issue #429

deploy/cpp_infer/CMakeLists.txt Outdated Show resolved Hide resolved
Comment on lines 71 to 77
#ifdef USE_MKL
#pragma omp parallel
for (auto i = 0; i < 10; i++) {
assert(config.cpu_math_library_num_threads == omp_get_num_threads());
}
#endif

Copy link

Choose a reason for hiding this comment

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

What is the purpose of that code?

Copy link
Author

Choose a reason for hiding this comment

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

I think it is better to keep this asset. Because before, MKL was not linked properly, but it does not report any error. Developers and users don't know if it is linked properly. In the code where MKL is used, it is only running on 1 thread, omp_get_num_threads()=1, even config.cpu_math_library_num_threads is set to 8.

Copy link

Choose a reason for hiding this comment

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

I have doubts about whether this check should be an assert here. In my opinion a user should be informed with a warning rather than prevented from running Paddle when the number of threads is not appropriate. It would be good to have this assert in a test, not in this file.
@luotao1 , what do you think about it?
Other than that LGTM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @wojtuss

Copy link
Author

Choose a reason for hiding this comment

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

Done. How is now?

@luotao1
Copy link
Collaborator

luotao1 commented Sep 28, 2020

@OliverLPH Please help test it!

@lidanqing-intel lidanqing-intel force-pushed the develop-fix-omp-flag branch 2 times, most recently from 1b02139 to 2a7b74d Compare September 28, 2020 19:57
@OliverLPH
Copy link
Contributor

@lidanqing-intel
I test on windows platform, the results are shown below.
mkldnn 0
cpu_math_library_num_threads 1
infer cost: 8.55854s

mkldnn 0
cpu_math_library_num_threads 4
infer cost: 6.44267s

mkldnn 0
cpu_math_library_num_threads 6
infer cost: 6.05943s

cpu_math_library_num_threads has significant impact now.

Copy link
Collaborator

@luotao1 luotao1 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@LDOUBLEV LDOUBLEV left a comment

Choose a reason for hiding this comment

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

LGTM

@LDOUBLEV LDOUBLEV merged commit 31eee98 into PaddlePaddle:develop Sep 29, 2020
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.

None yet

5 participants