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

Mon Aug 23 09:50:10 PDT 2021

wmi 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;
+
----------------
hoy wrote:
> 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.
Yeah, right, std::vector is good for looking up. Now once cs name table is read, there is no other code appending names to the table. Just want to know whether we can have some additional mechanism to enforce that. Can we save the size of the table after reading cs name table and check that the table size is the same before visiting any element from the table?

================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:239
+// `ContextStr` is in the form of `FuncName:StartLine.Discriminator`.
+void SampleProfileReaderText::decodeContextString(StringRef ContextStr,
+                                                  StringRef &FName,
----------------
hoy wrote:
> 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?
Ok, if it is less likely to be used by others, it is fine to keep as it is.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108435