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

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

In summary, I think this patch should be minimized by

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


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


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

Similarly for the SerializeFrom call below and ValueProfData::getSize() call above.


https://reviews.llvm.org/D34838