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

[David Blaikie via llvm-commits]

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

> davidxl added a comment.
>
> In summary, I think this patch should be minimized by
>

While I appreciate/value minimizing churn - I'm mostly motivated by/trying
to ensure the best form for the code, rather than the smallest change here.

If they happen to coincide, that's great.


> 1. sinking the minimal number of interfaces into Counter subclass
> 2. for any sunk interface, provide a wrapper in the parent class
> InstrProfRecord
> 3. avoid exposing internal members to client side other than
> InstrProfWriters (may be making those method private and making Writer a
> friend ?)
>

I did consider this (& certainly open to discussing the tradeoff further to
see if it's the right one) - my thinking was more that this change
indicates a new use case/way to pivot the InstrProfRecord data structure
that's not exclusively in the domain of the InstrProfWriter.

Looking at it that way, it seems to me that it's a good thing to move more
parts of the code over to only caring about the part of the data structure
that's relevant to them - which in many cases is just the counters (though
I'd love a better name that "CounterHolder" (& could make it a non-nested
class, especially if it had a good standalone name) - even potentially
using the InstrProfRecord as the name for this name/hash-less part of the
data and "NamedInstrProfRecord" for the Name+Hash+InstrProfRecord - though
I haven't prototyped that/given it a lot of thought)


> 4. split out unrelated changes
>
>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProf.h:253
> +                       ArrayRef<InstrProfValueData> VDs, uint64_t Sum,
> +                       InstrProfValueKind ValueKind, uint32_t MaxMDCount);
>
> ----------------
> unrelated format change?
>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProf.h:701
> +    /// counts of all target values at this site.
> +    inline uint64_t getValueForSite(InstrProfValueData Dest[],
> +                                    uint32_t ValueKind, uint32_t Site)
> const;
> ----------------
> format change?
>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProfData.inc:317
>     */
> -  void deserializeTo(InstrProfRecord &Record,
> -                     InstrProfRecord::ValueMapType *VMap);
> +  void deserializeTo(InstrProfRecord::CounterHolder &Record,
> +                     InstrProfRecord::CounterHolder::ValueMapType *VMap);
> ----------------
> Why this change? IMO, we should reduce exposing CounterHolder if possible
> and maintain/use APIs at the InstrProfRecord level.
>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProfData.inc:361
>     */
> -  static uint32_t getSize(const InstrProfRecord &Record);
> +  static uint32_t getSize(const InstrProfRecord::CounterHolder &Counters);
>    /*!
> ----------------
> Same here.
>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProfData.inc:530
>  INSTR_PROF_VISIBILITY uint32_t
> -getValueProfDataSize(ValueProfRecordClosure *Closure) {
> +getValueProfDataSize(const void *Record, const ValueProfRecordVTable
> *VTable) {
>    uint32_t Kind;
> ----------------
> Can this be split into different patch?
>
>
> ================
> Comment at: include/llvm/ProfileData/InstrProfWriter.h:66
>    /// Write \c Record in text format to \c OS
> -  static void writeRecordInText(const InstrProfRecord &Record,
> +  static void writeRecordInText(StringRef Name, uint64_t Hash,
> +                                const InstrProfRecord::CounterHolder
> &Counters,
> ----------------
> There is no need to change this interface. You are passing name, hash etc,
> so might as well create a InstrProfRecord.
>

But then the counters must be copied or moved into the InstrProfRecord,
making it destructive (or expensive if it's a copy instead of a move) to
the writing step (not the end of the world for InstrProfWriter - but seems
unnecessarily shuffling things around when this is an implementation detail
of the writer anyway).


>
>
> ================
> Comment at: lib/ProfileData/InstrProf.cpp:641
> +static const ValueProfRecordVTable InstrProfRecordCountersVTable = {
> +    getNumValueKindsInstrProfCounters,
> +    getNumValueSitesInstrProfCounters,
> ----------------
> These naming changes are not necessary
>

Seemed relevant if the name of the data structure being operated on changes
(but I agree, if the design ends up being not to reduce the surface area of
code that only needs to handle the counters, then these names wouldn't need
to change).


>
>
> ================
> Comment at: lib/ProfileData/InstrProf.cpp:651
> +uint32_t
> +ValueProfData::getSize(const InstrProfRecord::CounterHolder &Counters) {
> +  return getValueProfDataSize(&Counters, &InstrProfRecordCountersVTable);
> ----------------
> The interface change here is not necessary.
>
>
> ================
> Comment at: lib/ProfileData/InstrProfWriter.cpp:152
> +              .second; // FIXME: Avoid copying by fixing the addRecord API
> +      SummaryBuilder->addRecord(std::move(R));
>
> ----------------
> davidxl wrote:
> > Summary Builder does not need name, hash etc. so perfhaps fixing API is
> the right way.
> On second thought, to reduce exposing implementation details and
> minimizing interface changes, it is cleaner to just do
>
> SummaryBuilder->addRecord(InstrProfRecord(K, ProfileData.first, Counters));
>

But the SummaryBuilder doesn't need/use the name or hash - so it seems
strange to me to pass that. (again, to the design tradeoff/discussion above)


>
> Similarly for the SerializeFrom call below and ValueProfData::getSize()
> call above.
>
>
> https://reviews.llvm.org/D34838
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170705/9499be19/attachment.html>