[PATCH] D127031: [CSSPGO][llvm-profgen] Reimplement SampleContextTracker using context trie

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:1008
+    } else {
+      ContextTracker.freeFSamples();
+    }
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > wlei wrote:
> > > > hoy wrote:
> > > > > I'm wondering if these two deallocations can be combined. Maybe in `buildProfileMap` we could exclude merged contexts there and then we don't need to do `Node->setFunctionSamples(nullptr);` inside `CSPreInliner::run`?
> > > > The `merged contexts` node is removed from the trie after merging, so we can't traverse the whole trie to delete all the FSamples. That's why I was thinking to leverage the `FuncToCtxtProfiles` which stores all the FSample pointers at the beginning. And if it's not the preinliner pass, `FuncToCtxtProfiles` is empty, then we need to traverse the trie to delete them. 
> > > > The merged contexts node is removed from the trie after merging, so we can't traverse the whole trie to delete all the FSamples.
> > > 
> > > I see. I guess we have to keep the diversion for now. A shared_ptr or a centralized FunctionSample storage such as what the reader maintains would typically help. That would require a change in `CSProfileGenerator::getOrCreateFunctionSamples` . Please leave a todo somewhere to be addressed laster.
> > > 
> > Good idea, I feel that's something feasible in this patch. Based on your idea to change `getOrCreateFunctionSamples `, I'm wondering if we can just use a vector to store all the `FunctionSamples`,  the trie will naturally make sure the node is unique, so we don't need a Map which is our concern before. 
> Yeah, a std::list would work. std::vector can be volatile when growing.
Yeah, thanks for the suggestion! I will implement it in https://reviews.llvm.org/D125246, that's the first diff 
introduce the memory leak.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031