[PATCH] D147740: [llvm-profdata] Refactoring Sample Profile Reader to increase FDO build speed using MD5 as key to Sample Profile map

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1292
+  std::pair<iterator, bool>
+  try_emplace(const key_type &Key, const SampleContext &Ctx, Ts &&...Args) {
+    assert(Key == Ctx.getHashCode());
----------------
Since this forces insert in case of conflict, should it be called just 'emplace'?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1301
+        Ret.second = true;
+        Ret.first->second = mapped_type();
+      }
----------------
potential memory leak?  Also why inserting an empty FunctionSamples instead of the one passed in?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1315
+  template <typename... Ts>
+  std::pair<iterator, bool> emplace(const SampleContext &Ctx, Ts &&...Args) {
+    return try_emplace(Ctx, std::forward<Ts>(Args)...);
----------------
what is the purpose of this wrapper?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1319
+
+  mapped_type &operator[](const SampleContext &Ctx) {
+    return try_emplace(Ctx, FunctionSamples()).first->second;
----------------
This interface is confusing.  User expect it to return existing entry, but here it erases it. Should this interface be hidden (and not allowed with assert)?


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1327
+    auto Ret = try_emplace(Ctx, FunctionSamples());
+    if (Ret.second)
+      Ret.first->second.setContext(Ctx);
----------------
is this always true as currently implemented?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147740/new/

https://reviews.llvm.org/D147740