[PATCH] D34694: llvm-profdata: Indirect infrequently used fields to reduce memory usage

[Xinliang David Li via llvm-commits]

On Tue, Jun 27, 2017 at 11:30 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Tue, Jun 27, 2017 at 9:46 AM David Li via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> davidxl added a comment.
>>
>> Yes, in general, the profile data is very sparse for large applications.
>> Only a small percentage of modules have non-zero profile counts.
>
>
> Curious - the example I'm testing has no zero profile counts.
> (shouldEncode always returns true) - are they filtered out by some profile
> generation?
>


Do a dump

llvm-profdata  show --counts --all-functions  <profile data>

I will be surprised if you don't see zero counts :)   There is of course no
 zero value profile counts.




>
> 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.
>
>
>>    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.
>>
>> Regarding the followup change, here is my suggestion:
>>
>> 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.
>> 2. InstrProfWriter can use the counter record and maintains its own
>> mapping.
>>
>
> Ah, makes sense & then would compose with this change rather than
> superseding it.
>
>
>>
>>
>>
>> ================
>> Comment at: include/llvm/ProfileData/InstrProf.h:716
>>    std::vector<InstrProfValueSiteRecord> &
>>    getValueSitesForKind(uint32_t ValueKind) {
>> +    if (!ValueData)
>> ----------------
>> Can we change the interface to return ArrayRef so that
>> 1) we don't need empty vector
>> 2) no need to duplicate the const/non-const version of the method?
>>
>
> 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.
>
> eg: InstrProfRecord::addValueData which does, basically,
> getValueSitesForKind(...).emplace_back(...);
> InstrProfRecord::reserveSites -> getValueSitesForKind(...).reserve(...)
>
> 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
> Similarly with clearValueData
> (& also calling reserveSites with 0 would be bad for memory usage too)
>
> All the calls in InstrProf.cpp (mergeValueProfData, scaleValueProfData)
> seem to be appropriately conditionalized to not call getValueSitesForKind
> when the count is zero.
>
> 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.
>


This  makes sense.

David



>
>
>>
>>
>> https://reviews.llvm.org/D34694
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170627/4479a04b/attachment.html>