[PATCH] D90125: [CSSPGO] Infrastructure for context-sensitive Sample PGO and Inlining

[Xinliang David Li via llvm-commits]

Given Wei's feedback, I am ok with the current design now, including the
external sample format.

On Wed, Nov 11, 2020 at 5:04 PM Wei Mi <wmi at google.com> wrote:

>
>
> On Wed, Nov 11, 2020 at 2:15 PM David Li via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> davidxl added inline comments.
>>
>>
>> ================
>> Comment at: llvm/include/llvm/ProfileData/SampleProf.h:361
>> +// Example of full context string (note the wrapping `[]`):
>> +//    `[main:3 @ _Z5funcAi:1 @ _Z8funcLeafi]`
>> +// Example of context-less function name (same as AutoFDO):
>> ----------------
>> wenlei wrote:
>> > davidxl wrote:
>> > > wenlei wrote:
>> > > > davidxl wrote:
>> > > > > wenlei wrote:
>> > > > > > wenlei wrote:
>> > > > > > > davidxl wrote:
>> > > > > > > > The syntax of the context string can probably be made more
>> consistent like:
>> > > > > > > >
>> > > > > > > > SampleContext : LeafContext
>> > > > > > > >                            : ParentContext   LeafContext
>> > > > > > > >
>> > > > > > > > LeafContext: function_name
>> > > > > > > > ParentContext: [ParentFrames]
>> > > > > > > > ParentFrames:   OneParentFrame
>> > > > > > > >                         : ParentFrames   OneParentFrame
>> > > > > > > > OneParentFrame:  function_name:line @
>> > > > > > > >
>> > > > > > > > So in your example, the full context string should look
>> like:
>> > > > > > > >
>> > > > > > > > [main:3 @_Z5funcAi:1 @] _Z8funcLeafi
>> > > > > > > >
>> > > > > > > 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).
>> > > > > > 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.
>> > > > > >
>> > > > > > 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).
>> > > > > >
>> > > > > > 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`.
>> > > > > >
>> > > > > > So practically, current syntax is more efficient for context
>> promotion.
>> > > > > >
>> > > > > > 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.
>> > > > > >
>> > > > > > What do you think?
>> > > > > >
>> > > > > > 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.
>> > > > > >
>> > > > > > 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).
>> > > > >
>> > > > > In this case, where does the bracket go?
>> > > > >
>> > > > > >
>> > > > > > 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`.
>> > > > > >
>> > > > > > So practically, current syntax is more efficient for context
>> promotion.
>> > > > > >
>> > > > > > 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.
>> > > > >
>> > > > > what is the difference between top level context vs context less
>> header?
>> > > > >
>> > > > >
>> > > > > >
>> > > > > > What do you think?
>> > > > > >
>> > > > >
>> > > > >
>> > > > > In this case, where does the bracket go?
>> > > >
>> > > > 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.
>> > > >
>> > > > 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.
>> > > >
>> > > > > what is the difference between top level context vs context less
>> header?
>> > > >
>> > > > 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.
>> > > >
>> > > > 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).
>> > > > > In this case, where does the bracket go?
>> > > >
>> > > > 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.
>> > > >
>> > > > 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.
>> > >
>> > > 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:
>> > > 1) is there a need to share external and internal rep? Once the
>> external strings are parsed they can be discarded
>> > > 2) for internal format, is there more compact form ? Is there a need
>> to use string ?
>> > > >
>> > > > > what is the difference between top level context vs context less
>> header?
>> > > >
>> > > > 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.
>> > > >
>> > > > 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).
>> > >
>> > >
>> > > for internal format, is there more compact form ? Is there a need to
>> use string ?
>> >
>> > 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.
>> >
>> > > is there a need to share external and internal rep? Once the external
>> strings are parsed they can be discarded
>> >
>> > Sharing between internal and external format isn't must have, but I
>> think it's nice to have if cost is minimal.
>> >
>> > 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).
>> >
>> > 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?
>> >
>> > > for internal format, is there more compact form ? Is there a need to
>> use string ?
>> >
>> > 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.
>>
>>
>> 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?
>>
>> >
>> > > is there a need to share external and internal rep? Once the external
>> strings are parsed they can be discarded
>> >
>> > Sharing between internal and external format isn't must have, but I
>> think it's nice to have if cost is minimal.
>> >
>> > 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).
>> >
>> > 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?
>>
>> Wei can probably chime in -- he has plans to reduce cross binary FDO
>> string table size.
>>
>> 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.
>>
>>
> 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.
>
> 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.
>
> I agree the CSSPGO change is orthogonal to the improvement of internal
> representation and doesn't have to depend on it.
>
>
>
>>
>> >
>>
>>
>>
>>
>> Repository:
>>   rG LLVM Github Monorepo
>>
>> CHANGES SINCE LAST ACTION
>>   https://reviews.llvm.org/D90125/new/
>>
>> https://reviews.llvm.org/D90125
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201112/da31104d/attachment.html>