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

[David Blaikie via llvm-commits]

On Tue, Jun 27, 2017 at 11:44 AM Xinliang David Li <davidxl at google.com>
wrote:

> 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.
>

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 :)


>
>
>
>
>>
>> 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.
>

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.


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