[PATCH] D34838: Prototype: Reduce llvm-profdata merge memory usage further

[David Blaikie via llvm-commits]

On Thu, Jun 29, 2017 at 3:20 PM David Li via Phabricator <
reviews at reviews.llvm.org> wrote:

> davidxl added inline comments.
>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProf.h:600
> +  };
> +  struct CounterHolder {
> +    std::vector<uint64_t> Counts;
> ----------------
> nit: maybe just Counters?  Holder does not provide additional information.
>

Then it's the same name as the field (happy to rename that too, though)
which causes problems when trying to name the type
(InstrProfRecord::Counters no longer refers to the type, but is a member
data pointer expression (well, if you put '&' before it, or maybe that's
optional, I forget)).

Certainly open to ideas.


>
>
> ================
> Comment at: lib/ProfileData/InstrProf.cpp:504
>    if (ThisNumValueSites != OtherNumValueSites) {
> -    SIPE.addError(instrprof_error::value_site_count_mismatch);
> +    // SIPE.addError(instrprof_error::value_site_count_mismatch);
>      return;
> ----------------
> Keep this until the discussion about its fate is resolved.
>

Good point - that these changes are independent but complementary. I've
updated this patch to preserve the current SoftInstrProfError handling.

This brings the memory savings down (this change brings memory usage from
10GB to 8GB, removing the SoftInstrProfError will bring that down further
to 5GB), but still demonstrable/justified.


>
>
> ================
> Comment at: lib/ProfileData/InstrProfWriter.cpp:152
> +              .second; // FIXME: Avoid copying by fixing the addRecord API
> +      SummaryBuilder->addRecord(std::move(R));
>
> ----------------
> Summary Builder does not need name, hash etc. so perfhaps fixing API is
> the right way.
>

Ah, thanks for catching that - done!

(updated with the CR feedback addressed & got the tests all passing with a
few fixes, etc)

- Dave


>
>
> https://reviews.llvm.org/D34838
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170630/b9f0d75a/attachment.html>