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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
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) {
----------------
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.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104267