<html xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
        {font-family:"Cambria Math";
        panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
        {font-family:DengXian;
        panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
        {font-family:Calibri;
        panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
        {font-family:"\@DengXian";
        panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
        {margin:0in;
        font-size:11.0pt;
        font-family:"Calibri",sans-serif;}
.MsoChpDefault
        {mso-style-type:export-only;
        font-size:10.0pt;}
@page WordSection1
        {size:8.5in 11.0in;
        margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
        {page:WordSection1;}
--></style>
</head>
<body lang="EN-US" link="#0563C1" vlink="#954F72" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal">Thanks for the reply and clarification. Having a single combined IR instrumentation and PGHO instrumentation sounds good.
<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I’m also wondering if you have any data you could share that tells the overall benefit of memprof driven optimization since last RFC, perhaps with some early prototype and on small/synthetic workload? Asking because even though this all
 looks promising, from runtime support to binary format, later profile loader and optimization, there’s non-trivial complexity being added to a few places.
<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Thanks,<o:p></o:p></p>
<p class="MsoNormal">Wenlei<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span style="font-size:12.0pt;color:black">From:
</span></b><span style="font-size:12.0pt;color:black">Snehasish Kumar <snehasishk@google.com><br>
<b>Date: </b>Thursday, October 7, 2021 at 12:06 PM<br>
<b>To: </b>Xinliang David Li <davidxl@google.com><br>
<b>Cc: </b>Wenlei He <wenlei@fb.com>, llvm-dev <llvm-dev@lists.llvm.org>, Vedant Kumar <vsk@apple.com>, andreybokhanko@gmail.com <andreybokhanko@gmail.com>, Teresa Johnson <tejohnson@google.com>, Hongtao Yu <hoy@fb.com><br>
<b>Subject: </b>Re: RFC: A binary serialization format for MemProf<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal">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 <davidxl@google.com> 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 <wenlei@fb.com> 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>
>> 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>
>><o:p></o:p></p>
</div>
</div>
</body>
</html>