[PATCH] D108435: [CSSPGO] split context string II - reader/writer changes

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProfReader.h:754
+  /// underlying immutable buffer for all clients.
+  std::unique_ptr<std::vector<SampleContextStorageType>> CSNameTable;
+
----------------
wmi wrote:
> Same as SampleProfileReaderText, can we use std::list instead of std::vector so the storage underlying will not be moved after more elements are inserted?
Std::vector is good for looking up with an offset, which is needed during the load of function profiles. Also, std::vector is more efficient for the extbinary reader, since number of contexts is known beforehand.  Once the CS name table section is read, the std::vector should be frozen and no more names should be appended.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:239
+// `ContextStr` is in the form of `FuncName:StartLine.Discriminator`.
+void SampleProfileReaderText::decodeContextString(StringRef ContextStr,
+                                                  StringRef &FName,
----------------
wmi wrote:
> Looks like it can be a utility function in SampleProfile.h and may be reused somewhere else.
It's actually moved out of SampleProfile.h since it's only used by the text reader. Also the text reader should be the only part of the toolchain that needs to parse full context strings. Other parts just need to deal with ArrayRef form of context. What do you think?


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:506
+ErrorOr<SampleContext>
+SampleProfileReaderBinary::readNameFromTable(bool IsContextName) {
+  auto FName(readStringFromTable());
----------------
wmi wrote:
> This param is unused. Write it as readNameFromTable(bool /* IsContextName */) to make it more explicit. 
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108435