[PATCH] D89524: [NFC][SampleFDO] Move some common stuff from SampleProfileReaderExtBinary/SampleProfileWriterExtBinary to their parent classes.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfWriter.cpp:206
+  switch (Type) {
+  case SecProfileSymbolList:
+    if (auto EC = writeProfileSymbolListSection())
----------------
wmi wrote:
> hoy wrote:
> > wmi wrote:
> > > hoy wrote:
> > > > wmi wrote:
> > > > > hoy wrote:
> > > > > > Why is this separated from `writeOneSection`?
> > > > > That is somewhat explained in the reply to Wenlei's mail. I want to put a section to an inheritance level when foreseeably new subclasses created in other inheritance branch will not use it. Surely whether SecProfileSymbolList will not be used by other new formats are questionable. I just use it as an example and it will be easy to move the section up.
> > > > I see. Can I ask if you are going to introduce a new ExtBinary variant? Since we are also adding a new section (for pseudo probe) , I'm not sure if it should be treated as an optional section of ExtBinaryBase or a required section of a new variant. 
> > > Yes, I am going to add a new ExtBinary variant. 
> > > 
> > > To introduce it a little bit, we have a synthesized profile which is merged from many sample profiles and we use it for targets without regular sample profile. The merged profile is quite large. Previously inline instances in the synthesized profile are all flattened, i.e., all inline instances in the profile are extracted and merged to their outline instance. We found selectively flattening instead of fully flattening the synthesized profile is helpful for performance, however it may increase compile time significantly in the postlink phase during profile loading. To solve that, we are trying to split the profile into two parts, one part containing inline instance and the other part not containing inline instance. When thinlto is used, the second profile loading pass in postlink phase will only have to load the part containing inline instance, which will minimize the compile time impact.
> > > 
> > > For the new variant we are going to propose, it doesn't need SecProfileSymbolList for the moment because the synthesized profile doesn't need SecProfileSymbolList. But we find in some initial experiment that we can selectively flatten regular sampleFDO profile without performance impact but with much smaller profile size, so the new variant could be possibly useful for regular sampleFDO too in the future. In that case, SecProfileSymbolList will be needed. 
> > > 
> > > With that being said, maybe SecProfileSymbolList is not a good example to be put in a subclass, we can move SecProfileSymbolList to the Base class if you agree. 
> > Thanks for providing more context which is helpful. I'm not particularly concerned with where to place  `SecProfileSymbolList`. I'm still trying to understand class hierarchy. Are we separating the variants so that a section ends up being required but not optional in every variant? If a section can be optional, do we still need the inheritance?
> Currently if a section is registered in SectionHdrLayout of a class, it means the variant will have a place for the section in section table in the file header, but if you don't need it, just don't write anything to the section and it will be an empty section. That make section support essentially optional.
> 
> I tend to keep inheritance is because if we just use one class, we end up putting the requirement of every variant in the same class and I am worried that may be too much for the single class as the formats evolve. 
> 
> Surely we also don't want to have too many classes either. So maybe we can support multiple layouts for a certain subclass if the layouts are similar. Only if the new variant is significantly different from existing one, we create a new class. What do you think?
I see your point now. Yes, it's reasonable to not mix custom section implementation with the common base class. 

Should the base implementation `SampleProfileWriterExtBinaryBase::writeOtherSection` be also called here since a subclass inherits all sections from the base class?

Nit: consider naming it `writeCustomSection` if that sounds good to you?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D89524