[oneDNN] First fix to #33021 #33174

jczaja · 2021-05-27T14:16:20Z

PR types

Bug fixes

PR changes

Others

Describe

It is a fix to increasing cache size when in cache clearing mode as described in issue #33021.

paddle-bot-old · 2021-05-27T14:16:24Z

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

- compilation fix - Fix - Lint

paddle/fluid/inference/tests/api/analyzer_detect_functional_tester.cc

jczaja · 2021-06-02T12:51:04Z

@jakpiase Please continue your review.

lidanqing-intel

LGTM. Reviewed half

lidanqing-intel · 2021-06-02T13:01:52Z

paddle/fluid/inference/tests/api/analyzer_detect_functional_tester.cc

+  cfg.SetMkldnnCacheCapacity(cache_capacity);
+
+  // we will use two predictors (same model)
+  auto first_predictor = CreatePaddlePredictor<AnalysisConfig>(cfg);


Hi, so there are two predictors? Where is second_predictor? And this validate_cache_onednn means test_cache_onednn ?

Originally there were two predictors, but I removed second one. So the comment here is misleading. I will fix that. thanks

lidanqing-intel · 2021-06-02T13:08:20Z

paddle/fluid/platform/device_context.cc

@@ -739,7 +758,7 @@ void MKLDNNDeviceContext::SetBlob(const std::string& name,
  return;
 }

-unsigned int MKLDNNDeviceContext::GetCachedObjectsNumber(void) {
+unsigned int MKLDNNDeviceContext::GetCachedObjectsNumber(void) const {
  unsigned int num_entries = 0;


The GetCachedObjectsNumber results is num_threads * nums_input_shape ?

Actually structure of cache is of three levels: session IDs , shapes, actual cached objects per shape. So To get quantity of cache we iterate through sessions and their shapes. For every shape we then get amount of cached objects (for shape) and sum it up with other amounts for different shapes.

lidanqing-intel · 2021-06-02T13:09:17Z

paddle/fluid/inference/tests/api/analyzer_detect_functional_tester.cc

+  auto onednn_dev_ctx =
+      dynamic_cast<platform::MKLDNNDeviceContext *>(pool.Get(place));
+  return onednn_dev_ctx->GetCachedObjectsNumber();
+}


Here we got number of all threas cached different input_shapes blobs?

This function just return total number of cached objects regardless the number of threads used. oneDnn cache is one and shared among threads.

jakpiase

LGTM

lidanqing-intel

LGTM. Thank you very much !

lidanqing-intel · 2021-06-07T03:08:50Z

Hi @wzzju Could you please review it? Thanks

arogowie-intel · 2021-06-07T07:15:50Z

paddle/fluid/platform/device_context.h

  using ExecMap = std::unordered_map<
      void*, std::vector<std::pair<BlobPtr_t<KeyBlob>, KeyBlob::iterator>>>;
+  using ExecShape = std::unordered_map<std::string, std::shared_ptr<ExecMap>>;


IMHO those structures deserves some nice documentation. Since every time I look at this code I need to decipher what kind of data are stored here and what are the keys in those maps. Please write detailed explanation. Maybe define some types which will help to make this code more readable. like:

using ExecKey = void*; using BlobMapCacheIterPair = std::pair<BlobPtr_t<KeyBlob>, KeyBlob::iterator>; using ExecBlobMapsVec = std::vector<BlobMapCacheIterPair>; using ExecBlobMap = std::unordered_map<ExecKey, ExecBlobMapsVec>; using ExecShapeMap = std::unordered_map<std::string, std::shared_ptr<ExecBlobMap>>;

arogowie-intel · 2021-06-07T07:32:08Z

paddle/fluid/inference/tests/api/analyzer_detect_functional_tester.cc

+  shape_lines.resize(num_samples);
+
+  // Let's remeber number of cached objects before
+  // exection and after every single execution


Suggested change

// exection and after every single execution

// execution and after every single execution

arogowie-intel · 2021-06-07T07:32:23Z

paddle/fluid/inference/tests/api/analyzer_detect_functional_tester.cc

+  std::vector<int> cache_filling;
+  cache_filling.push_back(GetNumCachedObjects());
+
+  // compute sequenctilly prediction


Suggested change

// compute sequenctilly prediction

// compute sequentially prediction

arogowie-intel · 2021-06-07T08:19:43Z

paddle/fluid/platform/device_context.cc

    }
  } else {
    VLOG(3) << "Prevented Clearing DNNL cache.";
    block_next_cache_clearing_ = false;
  }
 }

+void MKLDNNDeviceContext::RemoveShapeEntriesWithExecutor(void) const {
+  p_exec_items_->erase(p_exec_items_->begin());


Why are you removing the first object in map? How do you know that this is the actual key (shape) you want delete?

Removing shapes works according to FIFO concepts. So I remove objects related to oldest shape

lidanqing-intel · 2021-06-07T09:02:16Z

paddle/fluid/inference/tests/api/analyzer_detect_functional_tester.cc

+
+Record ProcessALine(const std::string &line, const std::string &shape_line) {
+  VLOG(3) << "process a line";
+  std::vector<std::string> columns;


Hi this columns is not used ?

good catch!

lidanqing-intel · 2021-06-07T09:17:15Z

paddle/fluid/inference/tests/api/analyzer_detect_functional_tester.cc

+TEST(Analyzer_detect, validate_cache_onednn) {
+  validate_cache_onednn(2 /*cache_capacity */);
+}
+#endif


Hi if this whole TEST(...) are within #ifdef PADDLE_WITH_MKLDNN #endif, does it mean this file could be renamed as xxx_ mkldnn_tester.cc because otherwise if WITH_MKLDNN OFF this test will not be executed but it looks now it will be executed

Ok. It seems It will be multi-threading functional tests of oneDNN execution to be tested, so I can change a name of file

jczaja · 2021-06-08T07:41:31Z

@juncaipeng could you please start your review?

jczaja · 2021-06-08T12:20:38Z

@wzzju Please start your review. All required CI passed

jakpiase · 2021-06-08T12:45:38Z

LGTM

juncaipeng

LGTM

* - First fix to PaddlePaddle#33021

…y consumption (#33571) * [oneDNN] First fix to #33021 (#33174) * - First fix to #33021 * [oneDNN] Second fix to #33021 (#33471) * use older download_data function Co-authored-by: Jacek Czaja <jacek.czaja@intel.com>

jczaja added the Intel label May 27, 2021

jczaja mentioned this pull request May 27, 2021

config. SetMkldnnCacheCapacity is useless #33021

Closed

jczaja force-pushed the prv-33021-fix branch 2 times, most recently from 431fe82 to a36fd33 Compare May 31, 2021 17:38

jczaja changed the title ~~[oneDNN] Candidate fix to #33021~~ [oneDNN] First fix to #33021 May 31, 2021

jczaja force-pushed the prv-33021-fix branch from 9bcca50 to 15fad22 Compare June 1, 2021 07:41

- First fix to PaddlePaddle#33021

7171132

- compilation fix - Fix - Lint

jczaja force-pushed the prv-33021-fix branch from 15fad22 to 7171132 Compare June 1, 2021 15:47

jczaja requested review from jakpiase, arogowie-intel and lidanqing-intel June 1, 2021 16:10

jakpiase suggested changes Jun 1, 2021

View reviewed changes

- Fixes after review

f243133

lidanqing-intel reviewed Jun 2, 2021

View reviewed changes

- fixes after second round of internal review

9f2b9e0

jakpiase self-requested a review June 2, 2021 14:55

jakpiase previously approved these changes Jun 2, 2021

View reviewed changes

lidanqing-intel previously approved these changes Jun 2, 2021

View reviewed changes

arogowie-intel suggested changes Jun 7, 2021

View reviewed changes

arogowie-intel reviewed Jun 7, 2021

View reviewed changes

lidanqing-intel reviewed Jun 7, 2021

View reviewed changes

- fixes after third round of review

9c30e39

jczaja dismissed stale reviews from lidanqing-intel and jakpiase via 9c30e39 June 7, 2021 12:29

arogowie-intel approved these changes Jun 7, 2021

View reviewed changes

jczaja assigned juncaipeng Jun 8, 2021

jczaja assigned wzzju Jun 8, 2021

juncaipeng approved these changes Jun 9, 2021

View reviewed changes

juncaipeng merged commit 1382cd2 into PaddlePaddle:develop Jun 9, 2021

lidanqing-intel pushed a commit to lidanqing-intel/Paddle that referenced this pull request Jun 15, 2021

[oneDNN] First fix to PaddlePaddle#33021 (PaddlePaddle#33174)

d49a715

* - First fix to PaddlePaddle#33021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[oneDNN] First fix to #33021 #33174

[oneDNN] First fix to #33021 #33174

jczaja commented May 27, 2021 •

edited

Loading

paddle-bot-old bot commented May 27, 2021

jczaja commented Jun 2, 2021

lidanqing-intel left a comment

lidanqing-intel Jun 2, 2021 •

edited

Loading

jczaja Jun 2, 2021

lidanqing-intel Jun 2, 2021

jczaja Jun 2, 2021

lidanqing-intel Jun 2, 2021

jczaja Jun 2, 2021

jakpiase left a comment

lidanqing-intel left a comment

lidanqing-intel commented Jun 7, 2021

arogowie-intel Jun 7, 2021

arogowie-intel Jun 7, 2021

arogowie-intel Jun 7, 2021

arogowie-intel Jun 7, 2021

jczaja Jun 7, 2021

lidanqing-intel Jun 7, 2021

jczaja Jun 7, 2021

lidanqing-intel Jun 7, 2021 •

edited

Loading

jczaja Jun 7, 2021

jczaja commented Jun 8, 2021

jczaja commented Jun 8, 2021 •

edited

Loading

jakpiase commented Jun 8, 2021

juncaipeng left a comment

	// exection and after every single execution
	// execution and after every single execution

	// compute sequenctilly prediction
	// compute sequentially prediction

[oneDNN] First fix to #33021 #33174

[oneDNN] First fix to #33021 #33174

Conversation

jczaja commented May 27, 2021 • edited Loading

PR types

PR changes

Describe

paddle-bot-old bot commented May 27, 2021

jczaja commented Jun 2, 2021

lidanqing-intel left a comment

Choose a reason for hiding this comment

lidanqing-intel Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakpiase left a comment

Choose a reason for hiding this comment

lidanqing-intel left a comment

Choose a reason for hiding this comment

lidanqing-intel commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidanqing-intel Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jczaja commented Jun 8, 2021

jczaja commented Jun 8, 2021 • edited Loading

jakpiase commented Jun 8, 2021

juncaipeng left a comment

Choose a reason for hiding this comment

jczaja commented May 27, 2021 •

edited

Loading

lidanqing-intel Jun 2, 2021 •

edited

Loading

lidanqing-intel Jun 7, 2021 •

edited

Loading

jczaja commented Jun 8, 2021 •

edited

Loading