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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1023
 
+std::error_code SampleProfileReaderExtBinaryBase::readCSNameTableSec() {
+  auto Size = readNumber<uint32_t>();
----------------
> Comment added to the definition of SampleProfileReaderExtBinaryBase::readCSNameTableSec. 

Not seeing comments in the latest update. Did I miss anything?


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:326-327
+        // otherwise it's treated as context-less function name only.
+        auto Context = createContext(FName);
+        FContext.setContext(Context);
         ++CSProfileCount;
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > Even though we only need to deal with string context in profile reader/writer for text profile, it's probably still cleaner to keep all string context related parsing into SampleContext. 
> > > > 
> > > > `createContext` is more like a ctor. I'd prefer keep string decoding, createContext in its original place in SampleContext. That way, we can construct a context from string, SampleContext::setContext remain a private helper too, and the logic here can be simpler, just like before.
> > > > 
> > > > SampleContext does still have getContextString and toString, so it's not really isolated from string representation, might as well keep all string stuff together there for consistency. 
> > > Makes sense. Moved back to SampleContext.
> > Thanks. Can you move it back in D107299, so we don't see the change back and forth, just for review?
> > 
> > Can we also fold `FName.startswith("[")` in to SampleContext? 
> > 
> > Additionally, why we do need a `createContextFromString` instead of using overload ctors? This is also inconsistent between how SampleContext is created from text profile (createContextFromString) vs from binary profile (ctor). What I was thinking is have SampleContext decide how to create the object, so there's no changed needed here, just like before. 
> Constructing a CS context will require additional parameter than non-CS profile, especially the underlying context vector. That is causing the inconsistency with the context split work and I'm separating the construction of CS and non-CS contexts. Currently CS context is only constructible from a `SampleContextFrameVector`. 
> 
> Another reason is that I was hoping to construct `SampleContext` in a quick manner from `StringRef` to favor non-CS profile.
> 
> 
> Can you move it back in D107299, so we don't see the change back and forth, just for review?

Sorry my bad, I meant to say D108433. 

I think the key blocker for everything to be taken care from within ctor is that you need reader to own the context created from the string. How about having a ctor with StringRef and CSNameTable as parameter - it puts the context into CSNameTable (owned by reader) for CS profile, and the CSNameTable would be ignored for non-CS profile. 

The current implementation works, but reader has to be aware of the actual string representation of context. I thought it'd be cleaner if such representation is all dealt with from within SampleContext. Currently, it's indeed mostly handled by SampleContext, except it's bleeding into reader here. 




================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:638
 
-  auto FName(readStringFromTable());
-  if (std::error_code EC = FName.getError())
+  auto FContext(readNameFromTable(IsContextName));
+  if (std::error_code EC = FContext.getError())
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > nit: this can be confusing, readNameFromTable can mislead people to think FContext is string (or vector of strings) type. the auto also isn't helping. Either spell out the type name, or rename `readNameFromTable` to something like `readSampleContextFromTable`
> > > 
> > > 
> > Fixed by using explicit type.
> Ok, but on 2nd thought, why do we call it readName while it's actually returning a context? Especially given that we've renamed addName to AddContext, writeNameIdx to writeContextIdx.
I think it'd be good to establish a convention as to when we call things `Name` vs `Context`. My thought is it goes with the type, what do you think? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108435