[PATCH] D111676: [memprof] Replace the block cache with a hashmap.

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added a comment.

PTAL, thanks!



================
Comment at: compiler-rt/lib/memprof/memprof_mibmap.h:5
+#include "memprof_meminfoblock.h"
+#include "sanitizer_common/sanitizer_addrhashmap.h"
+
----------------
vitalybuka wrote:
> snehasish wrote:
> > vitalybuka wrote:
> > > AddrHashMap is useful if we need to delete items or address is full 64bit address space
> > > D111608 just today made alloc_context_id is very sequential [0..2^31), so you can use simple structure:
> > > 
> > > ````
> > > using MIBMapTy = TwoLevelMap<MemInfoBlock, StackDepot::kNodesSize1, StackDepot::kNodesSize2>
> > > 
> > > inline void InsertOrMerge(const uptr Id, const MemInfoBlock &Block,
> > >                           MIBMapTy &Map) {
> > >   Map[id].Merge();
> > > }
> > > 
> > > Print(const MIBMapTy &Map) {
> > >   for (u32 i = 0; Map.contains(i) ; ++i)
> > >        Map[i].Print(); 
> > > }
> > > ```
> > > 
> > > 
> > > After that you have same without ForEach implementation
> > > Still Merge vs Merge is not solved
> > I'm very hesitant to rely on the sequential ordering of allocation context ids. I think it would be best for memprof to treat it as an opaque value so that the underlying implementation can evolve. For example, even memprof may store additional data which may not be indexed by a calling context.
> > 
> > So I would prefer it if we continue with the ForEach implementation in the parent patch unless you have a strong opinion.
> I would recommend you to keep it simple, which is TwoLevelMap.
> It requires trivial change to this patch, and you always can recover ForEach if needed. Very likely evolution of memprof will lead you to unexpected places where neither solution is good enough. 
> 
After internal discussion and evaluation on a large workload we decided to keep using the AddrHashMap implementation for now since it gives us more flexibility. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111676