<div dir="ltr"><div class="gmail_default" style="font-family:monospace;font-size:small;color:rgb(0,0,0)">Given Wei's feedback, I am ok with the current design now, including the external sample format.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 11, 2020 at 5:04 PM Wei Mi <<a href="mailto:wmi@google.com">wmi@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:"times new roman",serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 11, 2020 at 2:15 PM David Li via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">davidxl added inline comments.<br>
<br>
<br>
================<br>
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:361<br>
+// Example of full context string (note the wrapping `[]`):<br>
+// `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]`<br>
+// Example of context-less function name (same as AutoFDO):<br>
----------------<br>
wenlei wrote:<br>
> davidxl wrote:<br>
> > wenlei wrote:<br>
> > > davidxl wrote:<br>
> > > > wenlei wrote:<br>
> > > > > wenlei wrote:<br>
> > > > > > davidxl wrote:<br>
> > > > > > > The syntax of the context string can probably be made more consistent like:<br>
> > > > > > > <br>
> > > > > > > SampleContext : LeafContext<br>
> > > > > > > : ParentContext LeafContext<br>
> > > > > > > <br>
> > > > > > > LeafContext: function_name<br>
> > > > > > > ParentContext: [ParentFrames]<br>
> > > > > > > ParentFrames: OneParentFrame <br>
> > > > > > > : ParentFrames OneParentFrame<br>
> > > > > > > OneParentFrame: function_name:line @<br>
> > > > > > > <br>
> > > > > > > So in your example, the full context string should look like:<br>
> > > > > > > <br>
> > > > > > > [main:3 @_Z5funcAi:1 @] _Z8funcLeafi<br>
> > > > > > > <br>
> > > > > > Agreed, that's indeed more consistent and I like that better. Thanks for the suggestion. Will make the change (llvm-profgen change will follow too).<br>
> > > > > Actually, there're other implication if we were to change context string to be `[context] leaf`. With the currently syntax, when we promote context, new context string is a substring of the original one. So we just create StringRef wrapper for context promotion without creating new strings. <br>
> > > > > <br>
> > > > > E.g. when `main:3 @ _Z5funcAi:1 @ _Z8funcLeafi` is promoted, it becomes `_Z5funcAi:1 @ _Z8funcLeafi` which is sub string of the original one (StringRef that reuses the underlying string).<br>
> > > > > <br>
> > > > > If we use `[main:3 @_Z5funcAi:1 @] _Z8funcLeafi`, promotion will lead to something like `[_Z5funcAi:1 @] _Z8funcLeafi`, which is no longer a substring and a new string need to be created. We use two StringRef to represent context part and leaf part today, but we also need a consistent string representation for the full context `getNameWithContext`.<br>
> > > > > <br>
> > > > > So practically, current syntax is more efficient for context promotion.<br>
> > > > > <br>
> > > > > Additionally, with the proposed syntax, top level context would look like this `[] main` to differentiate from context-less header. In this case, `[main]` is probably better.<br>
> > > > > <br>
> > > > > What do you think?<br>
> > > > > <br>
> > > > > Actually, there're other implication if we were to change context string to be `[context] leaf`. With the currently syntax, when we promote context, new context string is a substring of the original one. So we just create StringRef wrapper for context promotion without creating new strings. <br>
> > > > > <br>
> > > > > E.g. when `main:3 @ _Z5funcAi:1 @ _Z8funcLeafi` is promoted, it becomes `_Z5funcAi:1 @ _Z8funcLeafi` which is sub string of the original one (StringRef that reuses the underlying string).<br>
> > > > <br>
> > > > In this case, where does the bracket go?<br>
> > > > <br>
> > > > > <br>
> > > > > If we use `[main:3 @_Z5funcAi:1 @] _Z8funcLeafi`, promotion will lead to something like `[_Z5funcAi:1 @] _Z8funcLeafi`, which is no longer a substring and a new string need to be created. We use two StringRef to represent context part and leaf part today, but we also need a consistent string representation for the full context `getNameWithContext`.<br>
> > > > > <br>
> > > > > So practically, current syntax is more efficient for context promotion.<br>
> > > > > <br>
> > > > > Additionally, with the proposed syntax, top level context would look like this `[] main` to differentiate from context-less header. In this case, `[main]` is probably better.<br>
> > > > <br>
> > > > what is the difference between top level context vs context less header?<br>
> > > > <br>
> > > > <br>
> > > > > <br>
> > > > > What do you think?<br>
> > > > > <br>
> > > > <br>
> > > > <br>
> > > > In this case, where does the bracket go?<br>
> > > <br>
> > > Internally, we don't use the bracket, and since they're the first and last character, we just create a StringRef with bracket removed for `SampleContext`, without creating new string.<br>
> > > <br>
> > > So in profile file, we have `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]`, while in `SampleContext` used by LLVM and tools, we have `main:3 @ _Z5funcAi:1 @ _Z8funcLeafi`. The form used by `SampleContext` is easier for context promotion, and the conversion from `[]` form to `SampleContext` is just a StringRef wrapper.<br>
> > > <br>
> > > > what is the difference between top level context vs context less header? <br>
> > > <br>
> > > They have different meanings, consider a dso, context-less profile `foo` means we just don't know the calling context, while context profile `[foo]` means this is called directly from external function. <br>
> > > <br>
> > > In practice, we don't have context-less profile and context profile in a single profile file now, so it's also about consistency in context profile and enough differentiation between context profile and context-less profile (i.e reserve the form `main` *only* for context-less profile for today's AFDO).<br>
> > > > In this case, where does the bracket go?<br>
> > > <br>
> > > Internally, we don't use the bracket, and since they're the first and last character, we just create a StringRef with bracket removed for `SampleContext`, without creating new string.<br>
> > > <br>
> > > So in profile file, we have `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]`, while in `SampleContext` used by LLVM and tools, we have `main:3 @ _Z5funcAi:1 @ _Z8funcLeafi`. The form used by `SampleContext` is easier for context promotion, and the conversion from `[]` form to `SampleContext` is just a StringRef wrapper.<br>
> > <br>
> > That is what I was thinking -- for external format, we need to make it well defined and as consistent as possible, while for internal representation, any format is fine. Here is the question:<br>
> > 1) is there a need to share external and internal rep? Once the external strings are parsed they can be discarded<br>
> > 2) for internal format, is there more compact form ? Is there a need to use string ?<br>
> > > <br>
> > > > what is the difference between top level context vs context less header? <br>
> > > <br>
> > > They have different meanings, consider a dso, context-less profile `foo` means we just don't know the calling context, while context profile `[foo]` means this is called directly from external function. <br>
> > > <br>
> > > In practice, we don't have context-less profile and context profile in a single profile file now, so it's also about consistency in context profile and enough differentiation between context profile and context-less profile (i.e reserve the form `main` *only* for context-less profile for today's AFDO).<br>
> > <br>
> > <br>
> > for internal format, is there more compact form ? Is there a need to use string ? <br>
> <br>
> This is something we thought about too. We were thinking about something along the lines of a rolling hash (integer encoding that is friendly to context promotion operation) to eliminate StringRef. But actually with current implementation, it's quite compact already because it's always StringRef and we never need to create any new string (the order of context syntax, root to leaf, was also intentional to make promoted context substring of original context). So we're happy with it.<br>
> <br>
> > is there a need to share external and internal rep? Once the external strings are parsed they can be discarded <br>
> <br>
> Sharing between internal and external format isn't must have, but I think it's nice to have if cost is minimal. <br>
> <br>
> External strings can be discarded but that would require non-trivial framework changes that affects AFDO too. Currently, for AFDO (and CSSPGO) we keep profile file in a memory buffer, and all external strings are StringRef, which is used throughout SampleProfileReader and hence SampleProfileLoader, assuming the underlying strings are always available. E.g. FunctionSamples::Name is StringRef wrapped around external string from the memory buffer of input profile. CSSPGO uses the same framework, and all of that would need to be changed if we want to discard external strings (essentially freeing the memory buffer after loading profile). <br>
> <br>
> This (freeing the memory buffer after loading profile for both AFDO and CSSPGO) feels like more of an optimization rather than part of the CSSPGO framework, and if we do that optimization later, we could change internal representation accordingly. What do you think?<br>
> <br>
> > for internal format, is there more compact form ? Is there a need to use string ? <br>
> <br>
> This is something we thought about too. We were thinking about something along the lines of a rolling hash (integer encoding that is friendly to context promotion operation) to eliminate StringRef. But actually with current implementation, it's quite compact already because it's always StringRef and we never need to create any new string (the order of context syntax, root to leaf, was also intentional to make promoted context substring of original context). So we're happy with it.<br>
<br>
<br>
A related question is how large is memory consumption when the raw string is used throughout ? By changing the internal format, can it bring down memory usage further for a large profile?<br>
<br>
> <br>
> > is there a need to share external and internal rep? Once the external strings are parsed they can be discarded <br>
> <br>
> Sharing between internal and external format isn't must have, but I think it's nice to have if cost is minimal. <br>
> <br>
> External strings can be discarded but that would require non-trivial framework changes that affects AFDO too. Currently, for AFDO (and CSSPGO) we keep profile file in a memory buffer, and all external strings are StringRef, which is used throughout SampleProfileReader and hence SampleProfileLoader, assuming the underlying strings are always available. E.g. FunctionSamples::Name is StringRef wrapped around external string from the memory buffer of input profile. CSSPGO uses the same framework, and all of that would need to be changed if we want to discard external strings (essentially freeing the memory buffer after loading profile). <br>
> <br>
> This (freeing the memory buffer after loading profile for both AFDO and CSSPGO) feels like more of an optimization rather than part of the CSSPGO framework, and if we do that optimization later, we could change internal representation accordingly. What do you think?<br>
<br>
Wei can probably chime in -- he has plans to reduce cross binary FDO string table size.<br>
<br>
If we tie our implementation too much on string sharing, it makes it less flexible to change in the future. If we can decouple external/internal representation, it allows us to get it right for the external format from the beginning.<br>
<br></blockquote><div><br></div><div><div class="gmail_default" style="font-family:"times new roman",serif">My current work is trying to reduce the names we have to load for cross binary FDO so as to reduce the build time. It doesn't change the string internal representation and that will still use StringRef. </div><br></div><div><div class="gmail_default" style="font-family:"times new roman",serif">One problem of string sharing is if we try to merge a lot of profiles, we need to keep all the memory buffer while we are doing the merge. If number of profiles is too large, we may run into OOM problem. Ideally, after we finish reading one profile we hope we can drop its memory buffer. That is one motivation to decouple the internal/external representation. Actually if the profile is using MD5, we already decouple it because external representation is uint64_t and internal representation is string, and we use a separate buffer called MD5StringBuffer to save all the MD5 strings. That can be done for regular AFDO profile reading as well. </div><div class="gmail_default" style="font-family:"times new roman",serif"><br></div><div class="gmail_default" style="font-family:"times new roman",serif">I agree the CSSPGO change is orthogonal to the improvement of internal representation and doesn't have to depend on it. </div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
<br>
<br>
<br>
<br>
Repository:<br>
rG LLVM Github Monorepo<br>
<br>
CHANGES SINCE LAST ACTION<br>
<a href="https://reviews.llvm.org/D90125/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D90125/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D90125" rel="noreferrer" target="_blank">https://reviews.llvm.org/D90125</a><br>
<br>
</blockquote></div></div>
</blockquote></div>