<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Oct 7, 2021 at 12:06 PM Snehasish Kumar <<a href="mailto:snehasishk@google.com" target="_blank">snehasishk@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">Hi Wenlei,<br>
<br>
Thanks for taking a look! Added responses inline.<br>
<br>
On Thu, Oct 7, 2021 at 9:29 AM Xinliang David Li <<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>> wrote:<br>
><br>
> Just a quick note -- IRPGO profile is not deterministic with multi-threaded programs due to contentions (there is of course atomic update mode, but it can be slow). Asynchronous dumping is another reason that the profile is not guaranteed to be repeatable.<br>
><br>
> David<br>
><br>
> On Thu, Oct 7, 2021 at 9:18 AM Wenlei He <<a href="mailto:wenlei@fb.com" target="_blank">wenlei@fb.com</a>> wrote:<br>
>><br>
>> Thanks for sharing the progress and details on the binary format. Overall this looks like a clean design that fits current PGO profile format with extensions.<br>
>><br>
>><br>
>><br>
>> Some high level comments:<br>
>><br>
>><br>
>><br>
<br>
Our focus is to have a single combined IR instrumentation and PGHO<br>
instrumentation phase to keep operational costs low. For CSPGO today,<br>
this would be the second IR instrumentation phase. We also intend to<br>
support a separate PGHO instrumentation phase.<br>
>> Does memprof/PGHO work together with today's IRPGO today, i.e. can we have one instrumented build to collect both PGO and PGHO profile, or we will need separate PGO instrumentation builds for each, in which case CSPGO + PGHO would need three iterations of training and build, which would be significant operational cost..<br>
<br>
Yes, the context tracker is quite relevant to the IR matching need.<br>
Teresa will share the detailed design soon and we can evaluate the<br>
benefit of reusing the existing logic for CSSPGO. I think this is<br>
orthogonal to this RFC (serialization format) so we can defer to the<br>
next one for a detailed discussion.<br>
>> I think some of the problems memprof faced when dealing with storing calling context and mapping context to IR is very similar to CSSPGO. I'm wondering if it makes sense to promote some existing infrastructure to be more general beyond just serving CSSPGO. One example is the IR mapping you mentioned (quoted below). In CSSPGO, we have the exact same need, and it's handled by `SampleContextTracker` which queries a context trie using an instruction/DILocation.<br>
>><br>
>><br>
>><br>
>>           >  Because the MIB corresponding to the A->B context is associated with function B in the profile, we do not find it by looking at function A’s profile when we see function A’s malloc call during matching. To address this we need to keep a correspondence from debug locations to the associated profile information.<br>
>><br>
>><br>
>><br>
<br>
We intend to retain as much of the calling context information until<br>
the IR matching. This is where we can leverage common solutions. We<br>
would be happy to generalize where appropriate and intend to tackle<br>
this topic in detail in the next RFC.<br></blockquote><div><br></div><div>In fact, we need to retain the calling context beyond matching, so that we can perform the context disambiguation transformations that Snehasish described in an earlier email. The next RFC will focus on the IR metadata needed to carry the PGHO data as well as the context.</div><div><br></div><div>From reading through the CSSPGO RFC it sounds like the context info is never annotated onto the IR, but rather just used during the sample PGO loading/inlining step to help generate more accurate IR prof md counts - is that correct? In that case perhaps some of the infrastructure can be shared for performing the matching for already inlined contexts, which I think is what the ContextTrieNode structures are used for from what I can tell perusing the code. It is a little unclear to me - how is the profile for a partially inlined context found in the data structure - i.e. how do you look up the ContextTrieNode for a given out of line function?</div><div><br></div><div>Thanks,</div><div>Teresa</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
>> The serialization of calling context, pruning of calling context are also example of shared problems, and we've put in some effort to have effective solutions (e.g. offline preinliner for most effective pruning, which I think could be adapted to help keep most important allocation context). Perhaps some of the frameworks can be merged, so LLVM has general context aware PGO support that can be leverage by different kinds of PGO (IRPGO, PGHO, CSSPGO). If you think this is worth pursuing, we’d be happy to help too.<br>
>><br>
>><br>
>><br>
>> More on the details:<br>
>><br>
>><br>
>><br>
As David mentioned, keeping the PGHO profile deterministic is a<br>
non-goal since IR PGO profile is non-deterministic.<br>
>> I saw that MemInfoBlock contains alloc/dealloc cpuid, does that make memprof profile non-deterministic in the sense that running memprof twice on the exact program and input would yield bit-wise different memory profile? I think IR PGO profile is deterministic?<br>
>><br>
>><br>
>><br>
We need to use the file path instead of the function to be able to<br>
distinguish COMDAT functions. The line_offset based matching is more<br>
resilient if the entire function is moved, I think it's a good idea<br>
and we can incorporate it into the IR matching phase.<br>
>> Why do we use `file:line:discriminator` instead of `func:line_offset:discriminator `? The later would be more resilient to source change. If function name string is too long, we could perhaps leverage the MD5 encoding used by sample PGO?<br>
>><br>
>><br>
>><br>
While we only intend to support Memprof optimizations for the main<br>
binary, retaining all executable mappings allow future analysis tools<br>
to symbolize shared library code.<br>
>> Is the design of mmap section (quoted below) trying to support memprof for multiple binaries in the same process at the same time, or mainly for handling multiple non-consecutive executable segments for a single binary?<br>
>><br>
>><br>
>><br>
>>            > The process memory mappings for the executable segment during profiling are stored in this section. This allows symbolization during post processing for binaries which are built with position independent code. For now all read only, executable  mappings are recorded, however in the future, mappings for heap data can also potentially be stored.<br>
>><br>
>><br>
Yes, we do intend to support Memprof profile section merging via<br>
`llvm-profdata merge`. The schema overhead per function is low now, so<br>
we opted for function granularity. We can revisit if the overheads are<br>
high or if the IR metadata scheme intends to keep it at module<br>
granularity (in which case we don't need the extra fidelity).<br>
>> Do we need each function record to have its own schema, do we expect different functions to use different versions/schemas? The is very flexible, but wondering what’s the use case. If the schema is for compatibility across versions, perhaps a file level scheme would be enough?<br>
>><br>
>><br>
>><br>
>>             > The InstrProfRecord for each function will hold the schema and an array of Memprof info blocks, one for each unique allocation context.<br>
>><br>
>><br>
>><br>
>><br>
>><br>
>> Thanks,<br>
>><br>
>> Wenlei<br>
>><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr"><div dir="ltr"><div><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top:2px solid rgb(213,15,37)">Teresa Johnson |</td><td nowrap style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td nowrap style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top:2px solid rgb(238,178,17)"><br></td></tr></tbody></table></span></div></div></div></div>