<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Jun 27, 2017 at 11:44 AM Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 27, 2017 at 11:30 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">On Tue, Jun 27, 2017 at 9:46 AM 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">davidxl added a comment.<br>
<br>
Yes, in general, the profile data is very sparse for large applications. Only a small percentage of modules have non-zero profile counts.</blockquote></span><div><br>Curious - the example I'm testing has no zero profile counts. (shouldEncode always returns true) - are they filtered out by some profile generation?<br></div></div></div></blockquote><div><br></div><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Do a dump </div><div><br></div><div>llvm-profdata show --counts --all-functions <profile data></div><div><br></div><div>I will be surprised if you don't see zero counts :) There is of course no zero value profile counts.</div></div></div></div></blockquote><div><br>Ah, yeah - my mistake. I think I found there were no zero-length Count vectors (unlike the IndirectCall/MemOP ones - I tested Counts to see if I could indirect/omit that too). Nevermind :)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br>If there were a lot of zero profile counts added to the profile writer, it'd be potentially significant wasted memory I think? Well, not an issue for the situation I'm looking at, so happy to leave that alone until someone has a situation where it comes up I guess.<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Unlike edge/block profile which is statically allocated, the value profile data is created on-demand, this makes it possible to take advantage the sparsity as your patch does.<br>
<br>
Regarding the followup change, here is my suggestion:<br>
<br>
1. refactor InstrProfRecord into two parts, one part is the identification related including name, hash, etc, and the second part includes only counters -- InstrProfRecord becomes a wrapper class.<br>
2. InstrProfWriter can use the counter record and maintains its own mapping.<br></blockquote></span><div><br>Ah, makes sense & then would compose with this change rather than superseding it.<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/InstrProf.h:716<br>
std::vector<InstrProfValueSiteRecord> &<br>
getValueSitesForKind(uint32_t ValueKind) {<br>
+ if (!ValueData)<br>
----------------<br>
Can we change the interface to return ArrayRef so that<br>
1) we don't need empty vector<br>
2) no need to duplicate the const/non-const version of the method?<br></blockquote></span><div><br>Nah, this is a bit subtle & certainly worth considering better options - but the way elements end up in these vectors is by calling the non-const version and mutating the result (importantly, adding elements). Even MutableArrayRef doesn't support element insertion (because it depends on the container type) though it does support mutating existing elements.<br><br>eg: InstrProfRecord::addValueData which does, basically, getValueSitesForKind(...).emplace_back(...);<br>InstrProfRecord::reserveSites -> getValueSitesForKind(...).reserve(...)<br><br>InstrProfRecord::sortValueData is a bit problematic - if this is called with empty value data, it could cause the value data struct to be allocated unnecessarily<br>Similarly with clearValueData<br>(& also calling reserveSites with 0 would be bad for memory usage too)<br><br>All the calls in InstrProf.cpp (mergeValueProfData, scaleValueProfData) seem to be appropriately conditionalized to not call getValueSitesForKind when the count is zero.<br><br>Perhaps the mutable API could/should be renamed to "getOrCreateValueSitesForKind" to make it clear it entails extra cost. (& that way the const version can be used even in a non-const context without extra work/oddities of casting to const, etc). The const version can probably use an ArrayRef.<br></div></div></div></blockquote><div><br></div><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>This makes sense.</div></div></div></div></blockquote><div><br>Done - though I did end up with a MutableArrayRef accessor (but it didn't need a separate implementation) for the sorting use case. Simpler than checking before using getOrCreate.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D34694" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34694</a><br>
<br>
<br>
<br>
</blockquote></div></div>
</blockquote></div><br></div></div>
</blockquote></div></div>