[PATCH] D89707: [CSSPGO][llvm-profgen] Parse mmap events from perf script

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/llvm-profgen.cpp:66
+    // TODO:
+    IsLoaded = true;
+  }
----------------
mtrofin wrote:
> hoy wrote:
> > mtrofin wrote:
> > > should this set IsLoaded to false?
> > > 
> > > also, why not assert(!IsLoaded); or rename to "ensureLoaded"; 
> > > 
> > > or - is there value in a ProfiledBinary that's not loadable? If not, how about:
> > > - private ctor
> > > - fields are const; no need for IsLoaded
> > > - public static factory method returning a std::unique_ptr<ProfildBinary> that's null if loading fails.
> > > 
> > > that way, a reader knows they don't need to bother worrying about ProfiledBinaries that don't work - simpler overall state. wdyt?
> > Asserting the function is only run once per binary is reasonable since all provided binaries are loaded as soon as the tool starts. The initial design allowed for a load on demand, i.e, when binaries are not provided via the command line option, the profiled binaries record by mmap events will be automatically loaded while processing the events. `IsLoaded` was introduced as a signal for that. However that complicated the tool and was not continued. 
> > 
> > A load failure would result the tool to error and exit instead of returning a null object. Please see subsequent patch D89712.
> > 
> > 
> So then could the loading happen in the ctor, since failure to load => fast fail? Then the ProfiledBinary is known to be always loaded, and the only mutable field is the base address - to be clear, my suggestion is for simpler maintainability.
I see your point now. Thanks for the suggestion. There could be a case that user-specified binary never participated in profiling, which means they are not included in the mmap events, and they should not be loaded and if they are, their load failure should not block the processing. However, that sounds a user responsibility. Moving the load into ctor time is reasonable to me. It is a cleaner design.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89707