Skip to content

[oneDNN] Second fix to #33021 #33471

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

Merged
merged 2 commits into from
Jun 11, 2021
Merged

[oneDNN] Second fix to #33021 #33471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0097cd6
- Second fix
jczaja Jun 8, 2021
c81229e
- fix
jczaja Jun 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions paddle/fluid/inference/api/analysis_predictor.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,6 @@ void AnalysisPredictor::MkldnnPreSet(
platform::MKLDNNDeviceContext::tls().set_cur_mkldnn_session_id(
platform::MKLDNNDeviceContextThreadLocals::
kMKLDNNSessionID_CacheClearing);
platform::MKLDNNDeviceContext::tls().set_cur_input_shape_cache_capacity(
config_.mkldnn_cache_capacity_);
// Set current_input_shape for caching dynamic shape.
std::stringstream ss;
for (size_t i = 0; i < inputs_shape.size(); ++i) {
Expand All @@ -353,6 +351,9 @@ void AnalysisPredictor::MkldnnPreSet(
VLOG(2) << "Set input shape=" << ss.str();
platform::MKLDNNDeviceContext::tls().set_cur_input_shape_str(ss.str());
}
platform::MKLDNNDeviceContext::tls().set_cur_input_shape_cache_capacity(
config_.mkldnn_cache_capacity_);

Copy link
Contributor

@lidanqing-vv lidanqing-vv Jun 10, 2021

Choose a reason for hiding this comment

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

Is there difference to move position of this line of code? What is influenced ?

Copy link
Contributor Author

@jczaja jczaja Jun 10, 2021

Choose a reason for hiding this comment

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

Previously , cache_cpacity was only set when it was non-zero (cache clearing mode) . But as zero value is default it was
not set. But if you have two predictors : one working in restriced cache (clear mode) and other without this limitation
then if you run executor with restricted cache size and then go to execution of seconf executor that is being run on default settings then you would actually use restriced caching again. I mean we were doing reverting to defaults in MKLDNNPostReset() , but I we cannot do it anymore so I had to move this resetting to when execution begins.

Copy link
Contributor

@lidanqing-vv lidanqing-vv Jun 11, 2021

Choose a reason for hiding this comment

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

Oh ok it is now in preset function. Sorry I did not notice. Thanks for your explanation

#endif
}

Expand All @@ -368,10 +369,9 @@ void AnalysisPredictor::MkldnnPostReset() {
CHECK_LE(shape_blob_size,
static_cast<size_t>(config_.mkldnn_cache_capacity_));
}
paddle::platform::MKLDNNDeviceContext::tls().set_cur_mkldnn_session_id(
platform::MKLDNNDeviceContextThreadLocals::kMKLDNNSessionID_Default);
platform::MKLDNNDeviceContext::tls().set_cur_input_shape_cache_capacity(0);
platform::MKLDNNDeviceContext::tls().set_cur_input_shape_str("");
// We cannot reset to the default cache settings
// as there maybe CopyToCPU method used and oneDNN
// primitives are used there so cache would grow
}
#endif
}
Expand Down
13 changes: 13 additions & 0 deletions paddle/fluid/inference/tests/api/analyzer_detect_functional_mkldnn_tester.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,19 @@ void validate_cache_onednn(int cache_capacity = 1) {
file.close();
infer_file.close();

// Pick first output tensor from model
// as internally reorders may be called
// so it will impact cache size
auto output_names = predictor->GetOutputNames();
auto output_t = predictor->GetOutputTensor(output_names[0]);
std::vector<int> output_shape = output_t->shape();
size_t out_num = std::accumulate(output_shape.begin(), output_shape.end(), 1,
std::multiplies<int>());
std::vector<float> out_data;
out_data.resize(out_num);
output_t->CopyToCpu(out_data.data());

// Release predictor (relevant cache should be emptied)
Copy link
Contributor

@lidanqing-vv lidanqing-vv Jun 10, 2021

Choose a reason for hiding this comment

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

"predictor.reset(nullptr);" I feel we don't call often call this in API tests. Usually we just do predictor.Run(), that's all and seems assuming that cache will be release during deconstructor of some object?
What will be the result if we remove this predictor.reset(nullptr) ?

Copy link
Contributor Author

@jczaja jczaja Jun 10, 2021

Choose a reason for hiding this comment

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

predictor.reset(nullptr) This enforces that predictor is released and its cached objects as well. Normally you do not need to do it, as it is released when get out of scope, but I want to check if cache is released so I could not wait till end of scope.

predictor.reset(nullptr);
cache_filling.push_back(GetNumCachedObjects());

Expand Down