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

[David Blaikie via llvm-commits]

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?

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.


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