[PATCH] D104267: [CSSPGO] Fix an invalid hash table reference issue in the CS preinliner.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:395
+  for (auto &I : ProfilesToBeAdded) {
+    ProfileMap.try_emplace(I.first(), I.second);
+  }
----------------
With try_emplace, we're expecting some of the insertions will fail? In that case, such profile will be lost. Perhaps we still need to order the insertion so such failure will not happen. 


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:383
   StringSet<> ProfilesToBeRemoved;
-  // Note that StringMap order is guaranteed to be top-down order,
-  // this makes sure we make room for promoted/merged context in the
-  // map, before we move profiles in the map.
+  StringMap<FunctionSamples> ProfilesToBeAdded;
   for (auto &I : ProfileMap) {
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > hoy wrote:
> > > > > wenlei wrote:
> > > > > > Good catch - not sure how I come to incorrectly rely on the order of StringMap..
> > > > > > 
> > > > > > However the intention was to avoid extra copies of FunctionSamples as much as possible - these can large objects for copying. 
> > > > > > 
> > > > > > I think what we could do is to collect a `std::map<StringRef, StringRef>` (old context to new context) for how we should update the `StringMap`, then iterate over the (top-down) ordered `std::map` to move profile to be keyed by correct context. This way we can avoid one extra copy for each FunctionSamples that needs to be moved.
> > > > > IIUC, we are trying to update the key of a promoted profile in `ProfileMap`. Since we cannot do that in place, we basically remove the current entry and add a new entry which just differs in the context key. If that's the case, not sure how to avoid copying a profile since the value field of an entry is not reference.
> > > > Here you're doing two copies, Map[A] -> temp -> Map[B]. With a `std::map<StringRef, StringRef>` for name mapping in top-down order, we should be able to do in-place copy Map[A] -> Map[B] by iterating over the name map?
> > > `A` and `B` are context strings, right? Why does the top-down order matter, say `A` is `main->foo->bar` and `B` is `bar`?
> > > 
> > > Yeah, we are using a intermediate temp. Should change `ProfilesToBeAdded` to `StringMap<FunctionSamples&>` and do the addition before the deletion.  
> > This is my thought - two things blocks in-place copy: 1) iterating over StringMap while updating, which is solved by iterating over name mapping `std::map<StringRef, StringRef>`; 2) not overwriting items yet to be updated, this can be solved by ordering the updates so we make room for destination key of an update before we actually update.
> Still trying to follow. Say `ProfileMap` only has one entry `{"main->foo->bar", Profile}`. The context in `Profile` is "bar" and we are trying to update the key to "bar".  With an extra `std::map<StringRef, StringRef>` , we would make something like `{"main->foo->bar", "bar"}`. Then how do we proceed next? Still figuring out how to avoid copying `Profile`.
So the idea is to avoid copying out to a temp map then copying back. Now I see that you changed the temp map to use ref type. In that case, it's equally good as there's going to be only one copy. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104267