[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: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:
> > > > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104267