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

Wed Jul 5 13:57:25 PDT 2017

Also, the InstrProfRecordWithKey can inherit from InstProfRecord ...

David

On Wed, Jul 5, 2017 at 1:54 PM, Xinliang David Li <davidxl at google.com>
wrote:

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