[PATCH] D148872: [llvm-profdata] ProfileReader cleanup - preparation for MD5 refactoring - 2

Thu Apr 27 19:03:33 PDT 2023

wenlei added a comment.

> Cleanup profile reader classes to prepare for complex refactoring as propsed in D147740 <https://reviews.llvm.org/D147740>, continuing D148868 <https://reviews.llvm.org/D148868>

Might be a bit of nitpicking, but thought I'd mentioned it since it's related to earlier comment in another patch (D148876 <https://reviews.llvm.org/D148876>) about explaining "why" in change description - here I'd appreciate if you can explain why you need "CSNameTable and related things" to be moved up in the type hierarchy, and how that helps to prepare for the change to improve FDO build speed.

My understanding is that since you need to use MD5 as key for the profile map, everything related to MD5, names, CSnames need to be exposed as the top level interface. It's useful to others if you articulate the why explicitly upfront and the same goes for D148868 <https://reviews.llvm.org/D148868>.

================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:694
+  /// for SampleContextFrames.
+  std::vector<SampleContextFrameVector> CSNameTable;

----------------
hoy wrote:
> Are we planning to retire `SampleProfileReaderCompactBinary`? The functions and fields being refactored are not supposed to be available for that class.
Well... one more reason to remove compact binary support first before this refactoring. As pointed out in the other patch, compact binary is effectively deprecated, and new features are not supported with compact binary. So it creates a dilemma with refactoring as deduplication by hosting would move things that don't actually apply to compact binary to its parent class. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148872