[PATCH] D121655: [llvm-profgen] Read sample profiles for post-processing.

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:135-137
+                   const ContextSampleCounterMap &&Counters)
+      : ProfileGeneratorBase(Binary, std::move(Counters)){};
+  ProfileGenerator(ProfiledBinary *Binary, const SampleProfileMap &&Profiles)
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > hoy wrote:
> > > > > wenlei wrote:
> > > > > > hoy wrote:
> > > > > > > wenlei wrote:
> > > > > > > > Both `Reader->getSampleCounters()` and `Reader->getProfiles()` return by reference so there should be no copy, why do we need move ctor here? 
> > > > > > > This is because previously the `SampleCounters` field is a reference and it must be initialized in constructors. I'm now changing it to a value field and using the move constructor to initialize it when needed.
> > > > > > Ok, move still has some cost as a new object is constructed. In this case can we just use pointer type instead? 
> > > > > > 
> > > > > > IIUC, the change is to allow them to be "optional" depending on whether input is perf profile or llvm profile, for that it seems pointer which is nullable fits well.. 
> > > > > Yes, pointer type should work for `SampleCounters` since it's always read-only. For `ProfileMap`, which is not optional and is mutable during the postprocessing, a pointer type may result in ProfileGenerator changing the Reader's buffer.  This doesn't sound right conceptually, though technically this has no difference with std::move stealing the reader buffer. Another issue with using pointer type for `ProfileMap` is that it has to be explicitly initialized (maybe via unique_ptr) in one of the constructors.
> > > > > 
> > > > > 
> > > > I am not sure if the reasoning to use move ctor makes sense, but I also don't have a strong opinion.
> > > > 
> > > > > For ProfileMap, which is not optional and is mutable during the postprocessing, a pointer type may result in ProfileGenerator changing the Reader's buffer. This doesn't sound right conceptually, though technically this has no difference with std::move stealing the reader buffer. 
> > > > 
> > > > Changing reader's buffer is not the problem of the API, and using move ctor API also does not solve the problem. In fact using move may make it worse, because the Reader's buffer become invalid instead of changed.
> > > > 
> > > > >Another issue with using pointer type for ProfileMap is that it has to be explicitly initialized (maybe via unique_ptr) in one of the constructors. 
> > > > 
> > > > Why is explicit initialization an *issue*? 
> > > An explicit initialization may need an explicit destruction, unless some wrapping DS is used like unique_ptr. However, using unique_ptr in one case and raw ptr in the other makes it inconsistent, ie., how do you define the `ProfileMap` field. Using raw ptrs for both will require an explicit delete for one case and we need a way to tell it apart.
> > > 
> > > Ideally what comes out of the reader should be readonly, however, it looks like not easy to achieve without duplicating the whole data. Sounds like using move constructor is easy to implement. WDYT?
> > > An explicit initialization may need an explicit destruction, unless some wrapping DS is used like unique_ptr. However, using unique_ptr in one case and raw ptr in the other makes it inconsistent, ie., how do you define the `ProfileMap` field. Using raw ptrs for both will require an explicit delete for one case and we need a way to tell it apart.
> > 
> > You can let reader own the data (so destruction belongs to reader as well), then ProfileGenerator only takes an escaped raw pointer. explicit initialization is justing assign a value to raw pointer. 
> > 
> > > 
> > > Ideally what comes out of the reader should be readonly, however, it looks like not easy to achieve without duplicating the whole data. Sounds like using move constructor is easy to implement. WDYT?
> > 
> > I don't have strong opinion, but I don't see how move achieves what you want, it's not readonly anyways. 
> > 
> > You can let reader own the data (so destruction belongs to reader as well), then ProfileGenerator only takes an escaped raw pointer. explicit initialization is justing assign a value to raw pointer.
> 
> That'd require a change to the reader and the generator's constructor, just to be consistent with the other path.
> 
> Right, neither solves the readonly issue. The std::move one seems a bit clearer. I'm inlined to go with it if you're fine with both.
sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121655