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

Wed Jul 5 13:54:57 PDT 2017

On Wed, Jul 5, 2017 at 1:27 PM, David Blaikie <dblaikie at gmail.com> wrote:

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

It is not just about minimizing the churns.  Currently the class
InstrProfRecord abstracts everything the client needs to know about the
per-function profile which is nice thing we'd like to maintain.

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

I like the suggestion to promote the CounterHolder class into the first
class citizen -- InstrProfRecord with the existing InstrProfRecord be
renamed to NamedInstrProfRecord or InstrProfRecordWithKey.   I expect
majority of the APIs do not care about the name key anyway. The only
affected interfaces are related to profile reader iterators and a few
writer changes.

I think the above will be clean.

thanks,

David

>
>
>> 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/bc0bf8c5/attachment-0001.html>