-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
[oneDNN] Added clearing oneDNN cache per executor #32499
[oneDNN] Added clearing oneDNN cache per executor #32499
Conversation
Thanks for your contribution! |
@luotao1 PR-CI-Coverage failed and it is shown that one of lines is not covered. I can see that execution of test (test_analyzer_int8_resnet) is crossing this line so not sure why overage fails to notice that. Could you please advice on this? |
@@ -169,6 +169,9 @@ void Executor::Run(const ProgramDesc& pdesc, Scope* scope, int block_id, | |||
bool force_disable_gc, bool keep_kid_scopes) { | |||
platform::RecordBlock b(block_id); | |||
if (FLAGS_use_mkldnn) EnableMKLDNN(pdesc); | |||
#ifdef PADDLE_WITH_MKLDNN | |||
platform::AttachPointerHashToMKLDNNKey(this, place_); | |||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about ?
#ifdef PADDLE_WITH_MKLDNN
if (FLAGS_use_mkldnn) {
EnableMKLDNN(pdesc);
platform::AttachPointerHashToMKLDNNKey(this, place_);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luotao1 We cannot do that. as FLAGS_use_mkldnn = True makes an iteration through ops to set attributes "use_mkldnn=true" , but you can set this attribute on your own without using FLAGS_use_mkldnn. And situation that op is already having use_mkldnn=true set happens when for example we create an operator inside unit tests (python UTs). So then FLAGS_use_mkldnn is set to false , but we oneDNN execution happens.
Could you give the details how |
@luotao1 So test_analyzer_int8_resnet is being run during PR-CI-Coverage as it can be found in its log. This test is strictly oneDNN test without any conditional paths. I have run this test locally and stopped execution at the line that coverage listed as not being crossed through (mkldnn_quantizer.cc:415). Below is a backtrace (captured in GDB) showing that mentioned line
If you need more data then let me know. |
@jczaja PR_CI_Coverage is remitted now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
please cherry-pick to release 2.1
PR types
Bug fixes
PR changes
Others
Describe
This is second fix to #31992 . Problem was that when executor( naive, regular) we clear oneDNN cache which is bad behaviour when we have multiple executors used from multiple threads as it may happen that one thread is doign execution and halfway cache is cleared.