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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

In D89524#2337297 <https://reviews.llvm.org/D89524#2337297>, @wenlei wrote:

> Thanks for the refactoring. I'm wondering what is most flexible way for us to take advantage of extended binary format. I thought being able to mix and use different section is really flexible, but if we tie non-common section to specific new format/sub class, it may restrict the way we use this format. E.g. if `ExtFormatA` has `SecProfileSymbolList`, and `ExtFormatB` has something like `SecFuncMetadata` (which is what have for CSSPGO internally), then consider if there's a need for a profile to have both `SecProfileSymbolList` and `SecFuncMetadata` - implementing that through inheritance could be a bit cumbersome.
>
> I'm thinking about two alternatives:
>
> 1. Follow similar model like ELF, adding or removing a section does not change the format "type". With that, we can pass in `SectionHdrLayout` as parameter to ctor of reader and writer, and use the layout to drive the reading and writing. In this case, we don't need to use inheritance and all new section and its reader/writer functions are always registered in a single class.
> 2. Still use inheritance to allow new combination of sections as a new sub format, but always register all new section and its reader/writer functions in the base class. The subclasses only define unique `SectionHdrLayout`. With this, the example above will not need to inherit from both `ExtFormatA` and `ExtFormatB`.
>
> It's nice to keep the ability to use sections orthogonally, instead of in hierarchical manner. What do you think?

Good point. Thanks to raise it up. I considered those two alternatives and the current solution. They all have their own advantages and disadvantages.

For 1, it is flexible. Any Reader/Writer users can define their layout they want to have without having to define a new subclass. The disadvantage is the layout reflects the internal logic about how the sections will be implemented and used. Passing in SectionHdrLayout means to expose many details to the code using the reader/writer. Another disadvantage is that putting all the section reader/writer in a single class may be cumbersome.

For 2, it is also very flexible. The disadvantage is putting all the section read/writers in a class.

The current patch is an extension of 2. We still have inheritance and we register the section reader/writers at a proper level that all the subclasses can get/share the support they want (Each class can register their section reader/writer in readOtherSection/writeOtherSection). We may put a section in a subclass at the beginning but move the section up when more formats are added.

A common disadvantage of 2 and 3 are every time we need to define a new subclass, there is some routinary code we need to add.

So current patch is a compromise of the above consideration. I like to hear your opinions since CSSPGO will have some immediate requirement to define some new sections.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D89524