[oneDNN] Second fix to #33021 #33471
Conversation
- fix
|
Thanks for your contribution! |
| @@ -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_); | |||
|
|
|||
There was a problem hiding this comment.
Is there difference to move position of this line of code? What is influenced ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh ok it is now in preset function. Sorry I did not notice. Thanks for your explanation
| out_data.resize(out_num); | ||
| output_t->CopyToCpu(out_data.data()); | ||
|
|
||
| // Release predictor (relevant cache should be emptied) |
There was a problem hiding this comment.
"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) ?
There was a problem hiding this comment.
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.
|
LGTM. |
|
As agreed with @juncaipeng After passing internal review (two reviewers) for PRs specific to oneDNN we are allowed to merge . |
* - Second fix - fix * - fix
PR types
Bug fixes
PR changes
Others
Describe
It is second part of fixes to #33021. Problem addressed here was that Predictor after finishing execution of Run() is restoring default settings of cache. Those default settings are unrestriced size of cache (cache_capacity = 0). but after Run() is completed , There are sometimes Tensor:: CopyToCpu() methods called which are using oneDNN objects and could make cache growing.