[PATCH] Value profiling - patchset 3
Xinliang David Li
davidxl at google.com
Wed Jul 8 10:07:35 PDT 2015
> I'm thinking an on-disk structure something like this would make sense:
>
> function entry:
> * hash
> * number of counts
> * [counters]
> * [value profiling data]
>
> value profiling datum:
> * kind
> * number of sites for kind
> * [sites]
>
> site:
> * number of targets recorded
> * [target and count pairs]
>
> Basically:
>
> - This starts the same as now, with the function hash and the list of
> counters.
>
> - The value profiling data encodes a kind directly - if we only have
> kind "3" we don't need to encode 'zero' counts for kinds "1" and "2".
> This also means that the structure if there is no value profiling
> enabled is unchanged from before adding value profiling.
>
> - We have a list of value records per site, which means the record just
> needs to include the target and the count.
>
> Then "target" is an index into the separate string table where we keep
> the names, or more generally an index into the table of values for the
> particular kind of value profiling.
>
> I think this makes the code simpler to read and write, keeps the
> generality and forwards compatibility of encoding the value profiling
> kind, and will make accessing this data more efficient.
>
> WDYT?
>
I like the suggestion - adding a level of indirection to split out the
value profile data. It is similar to how it is handled in raw format
(where the number of sites per kind is peeled out into a single
array).
thanks,
david
>> };
>>
>> } // end namespace llvm
>> Index: include/llvm/ProfileData/InstrProfReader.h
>> ===================================================================
>> --- include/llvm/ProfileData/InstrProfReader.h
>> +++ include/llvm/ProfileData/InstrProfReader.h
>> @@ -15,15 +15,16 @@
>> #ifndef LLVM_PROFILEDATA_INSTRPROFREADER_H
>> #define LLVM_PROFILEDATA_INSTRPROFREADER_H
>>
>> -#include "llvm/ADT/ArrayRef.h"
>> #include "llvm/ADT/StringExtras.h"
>> #include "llvm/ProfileData/InstrProf.h"
>> #include "llvm/Support/EndianStream.h"
>> #include "llvm/Support/ErrorOr.h"
>> #include "llvm/Support/LineIterator.h"
>> #include "llvm/Support/MemoryBuffer.h"
>> #include "llvm/Support/OnDiskHashTable.h"
>> #include <iterator>
>> +#include <map>
>> +#include <string>
>
> These look unused. Why add them?
>
>>
>> namespace llvm {
>>
>> @@ -224,7 +225,17 @@
>> return StringRef((const char *)D, N);
>> }
>>
>> - data_type ReadData(StringRef K, const unsigned char *D, offset_type N);
>> + data_type ReadDataV1V2(StringRef K, const unsigned char *D, offset_type N);
>> + data_type ReadDataV3(StringRef K, const unsigned char *D, offset_type N);
>> +
>> + data_type ReadData(StringRef K, const unsigned char *D, offset_type N) {
>> + switch (FormatVersion) {
>> + case 1:
>> + case 2: return ReadDataV1V2(K, D, N);
>> + case 3: return ReadDataV3(K, D, N);
>> + }
>> + return data_type();
>> + }
>> };
>>
>> typedef OnDiskIterableChainedHashTable<InstrProfLookupTrait>
>> Index: include/llvm/ProfileData/InstrProfWriter.h
>> ===================================================================
>> --- include/llvm/ProfileData/InstrProfWriter.h
>> +++ include/llvm/ProfileData/InstrProfWriter.h
>> @@ -15,33 +15,39 @@
>> #ifndef LLVM_PROFILEDATA_INSTRPROFWRITER_H
>> #define LLVM_PROFILEDATA_INSTRPROFWRITER_H
>>
>> -#include "llvm/ADT/ArrayRef.h"
>> #include "llvm/ADT/DenseMap.h"
>> #include "llvm/ADT/StringMap.h"
>> #include "llvm/ProfileData/InstrProf.h"
>> #include "llvm/Support/DataTypes.h"
>> #include "llvm/Support/MemoryBuffer.h"
>> #include "llvm/Support/raw_ostream.h"
>> -#include <vector>
>>
>> namespace llvm {
>>
>> +/// Profiling information for a single function for outputting through the
>> +/// IndexedInstrProfWriter
>> +struct IndexedInstrProfRecord {
>> + IndexedInstrProfRecord() {}
>> + IndexedInstrProfRecord(std::vector<uint64_t> Counts)
>> + : Counts(std::move(Counts)) {}
>> + std::vector<uint64_t> Counts;
>> + std::vector<InstrProfValueRecord> Values[instrprof_value_kind::size];
>> +};
>> +
>> /// Writer for instrumentation based profile data.
>> class InstrProfWriter {
>> public:
>> - typedef SmallDenseMap<uint64_t, std::vector<uint64_t>, 1> CounterData;
>> + typedef SmallDenseMap<uint64_t, IndexedInstrProfRecord, 1> ProfilingData;
>> private:
>> - StringMap<CounterData> FunctionData;
>> + StringMap<ProfilingData> FunctionData;
>> uint64_t MaxFunctionCount;
>> public:
>> InstrProfWriter() : MaxFunctionCount(0) {}
>>
>> /// Add function counts for the given function. If there are already counts
>> /// for this function and the hash and number of counts match, each counter is
>> /// summed.
>> - std::error_code addFunctionCounts(StringRef FunctionName,
>> - uint64_t FunctionHash,
>> - ArrayRef<uint64_t> Counters);
>> + std::error_code addRecord(const InstrProfRecord &I);
>> /// Write the profile to \c OS
>> void write(raw_fd_ostream &OS);
>> /// Write the profile, returning the raw data. For testing.
>> Index: lib/ProfileData/InstrProfIndexed.h
>> ===================================================================
>> --- lib/ProfileData/InstrProfIndexed.h
>> +++ lib/ProfileData/InstrProfIndexed.h
>> @@ -47,7 +47,7 @@
>> }
>>
>> const uint64_t Magic = 0x8169666f72706cff; // "\xfflprofi\x81"
>> -const uint64_t Version = 2;
>> +const uint64_t Version = 3;
>> const HashT HashType = HashT::MD5;
>> }
>>
>> Index: lib/ProfileData/InstrProfReader.cpp
>> ===================================================================
>> --- lib/ProfileData/InstrProfReader.cpp
>> +++ lib/ProfileData/InstrProfReader.cpp
>> @@ -15,6 +15,7 @@
>> #include "llvm/ProfileData/InstrProfReader.h"
>> #include "InstrProfIndexed.h"
>> #include "llvm/ADT/STLExtras.h"
>> +#include "llvm/ProfileData/InstrProf.h"
>> #include <cassert>
>>
>> using namespace llvm;
>> @@ -302,8 +303,9 @@
>> typedef InstrProfLookupTrait::data_type data_type;
>> typedef InstrProfLookupTrait::offset_type offset_type;
>>
>> -data_type InstrProfLookupTrait::ReadData(StringRef K, const unsigned char *D,
>> - offset_type N) {
>> +data_type InstrProfLookupTrait::ReadDataV1V2(StringRef K,
>> + const unsigned char *D,
>> + offset_type N) {
>>
>> // Check if the data is corrupt. If so, don't try to read it.
>> if (N % sizeof(uint64_t))
>> @@ -342,6 +344,67 @@
>> return DataBuffer;
>> }
>>
>> +data_type InstrProfLookupTrait::ReadDataV3(StringRef K,
>> + const unsigned char *D,
>> + offset_type N) {
>> + using namespace support;
>> + const unsigned char *End = D + N;
>> + // Read number of data entries.
>> + if (D + sizeof (uint64_t) >= End)
>> + return data_type();
>> + uint64_t DataBufferSize = endian::readNext<uint64_t, little, unaligned>(D);
>> + DataBuffer.clear();
>> + DataBuffer.reserve(DataBufferSize);
>> + std::vector<uint64_t> CounterBuffer;
>> + std::vector<InstrProfValueRecord> ValueBuffer;
>> + for (unsigned I = 0; I < DataBufferSize; ++I) {
>> + // Read hash and number of counters
>> + if (D + sizeof (uint64_t) + sizeof(uint32_t) >= End)
>> + return data_type();
>> + uint64_t Hash = endian::readNext<uint64_t, little, unaligned>(D);
>> + uint32_t NumCounts = endian::readNext<uint32_t, little, unaligned>(D);
>> + // Read counter values
>> + if (D + NumCounts * sizeof(uint64_t) >= End)
>> + return data_type();
>> + CounterBuffer.clear();
>> + for (unsigned I = 0; I < NumCounts; ++I)
>> + CounterBuffer.push_back(endian::readNext<uint64_t, little, unaligned>(D));
>> +
>> + DataBuffer.push_back(InstrProfRecord(K, Hash, CounterBuffer));
>
> I think it will actually be simpler to keep a single ReadData instead of
> splitting into V1V2 and V3 - V3 is essentially V1V2 with more data at
> the end, and I can see here that the logic for the V1V2 data is nearly
> identical in both.
>
>> +
>> + // Read value profiling data
>> + for (uint32_t Kind = instrprof_value_kind::first;
>> + Kind < instrprof_value_kind::size; ++Kind) {
>> + // Read number of values for value kind
>> + if (D + sizeof(uint32_t) > End)
>> + return data_type();
>> + uint32_t NumValues = endian::readNext<uint32_t, little, unaligned>(D);
>> + ValueBuffer.clear();
>> + for (unsigned I = 0; I < NumValues; ++I) {
>> + // Read size of the name string
>> + if (D + sizeof(uint32_t) >= End)
>> + return data_type();
>> + uint32_t NameSize = endian::readNext<uint32_t, little, unaligned>(D);
>> + // Read name string
>> + if (D + NameSize * sizeof(char) >= End)
>> + return data_type();
>> + std::string Name((const char *)D, NameSize);
>> + D += NameSize;
>> + // Read raw value and numtaken and the counter index
>> + if (D + 2 * sizeof(uint64_t) + sizeof(uint32_t) > End)
>> + return data_type();
>> + uint64_t Value = endian::readNext<uint64_t, little, unaligned>(D);
>> + uint64_t NumTaken = endian::readNext<uint64_t, little, unaligned>(D);
>> + uint32_t Index = endian::readNext<uint32_t, little, unaligned>(D);
>> + ValueBuffer.push_back(
>> + InstrProfValueRecord(Name, Value, NumTaken, Index));
>> + }
>> + DataBuffer[DataBuffer.size()-1].Values[Kind] = ValueBuffer;
>> + }
>> + }
>> + return DataBuffer;
>> +}
>> +
>> bool IndexedInstrProfReader::hasFormat(const MemoryBuffer &DataBuffer) {
>> if (DataBuffer.getBufferSize() < 8)
>> return false;
>> Index: lib/ProfileData/InstrProfWriter.cpp
>> ===================================================================
>> --- lib/ProfileData/InstrProfWriter.cpp
>> +++ lib/ProfileData/InstrProfWriter.cpp
>> @@ -26,8 +26,8 @@
>> typedef StringRef key_type;
>> typedef StringRef key_type_ref;
>>
>> - typedef const InstrProfWriter::CounterData *const data_type;
>> - typedef const InstrProfWriter::CounterData *const data_type_ref;
>> + typedef const InstrProfWriter::ProfilingData *const data_type;
>> + typedef const InstrProfWriter::ProfilingData *const data_type_ref;
>>
>> typedef uint64_t hash_value_type;
>> typedef uint64_t offset_type;
>> @@ -44,9 +44,24 @@
>> offset_type N = K.size();
>> LE.write<offset_type>(N);
>>
>> - offset_type M = 0;
>> - for (const auto &Counts : *V)
>> - M += (2 + Counts.second.size()) * sizeof(uint64_t);
>> + offset_type M = sizeof(uint64_t); // size of ProfileData
>> + for (const auto &ProfileData : *V) {
>> + M += sizeof(uint64_t); // size of ProfileData.first i.e. hash
>> + M += sizeof(uint32_t); // size of ProfileData.second.Counts.size()
>> + M += ProfileData.second.Counts.size() * sizeof(uint64_t);
>> + for (uint32_t Kind = instrprof_value_kind::first;
>> + Kind < instrprof_value_kind::size; ++Kind) {
>> + M += sizeof(uint32_t); // size of ProfileData.second.Values[Kind].size()
>> + for (InstrProfValueRecord I : ProfileData.second.Values[Kind]) {
>> + M += sizeof(uint32_t); // size of I.Name
>> + M += I.Name.str().size() * sizeof(char);
>> + M += sizeof(uint64_t); // size of I.Value
>> + M += sizeof(uint64_t); // size of I.NumTaken
>> + M += sizeof(uint32_t); // size of I.CounterIndex
>> + }
>> + }
>> + }
>> +
>> LE.write<offset_type>(M);
>>
>> return std::make_pair(N, M);
>> @@ -61,51 +76,97 @@
>> using namespace llvm::support;
>> endian::Writer<little> LE(Out);
>>
>> - for (const auto &Counts : *V) {
>> - LE.write<uint64_t>(Counts.first);
>> - LE.write<uint64_t>(Counts.second.size());
>> - for (uint64_t I : Counts.second)
>> + LE.write<uint64_t>(V->size());
>> + for (const auto &ProfileData : *V) {
>> + LE.write<uint64_t>(ProfileData.first);
>> + LE.write<uint32_t>(ProfileData.second.Counts.size());
>> + for (uint64_t I : ProfileData.second.Counts)
>> LE.write<uint64_t>(I);
>> + for (uint32_t Kind = instrprof_value_kind::first;
>> + Kind < instrprof_value_kind::size; ++Kind) {
>> + LE.write<uint32_t>(ProfileData.second.Values[Kind].size());
>> + for (InstrProfValueRecord I : ProfileData.second.Values[Kind]) {
>> + LE.write<uint32_t>(I.Name.str().size());
>> + Out.write(I.Name.c_str(), I.Name.str().size());
>> + LE.write<uint64_t>(I.Value);
>> + LE.write<uint64_t>(I.NumTaken);
>> + LE.write<uint32_t>(I.CounterIndex);
>> + }
>> + }
>> }
>> }
>> };
>> }
>>
>> -std::error_code
>> -InstrProfWriter::addFunctionCounts(StringRef FunctionName,
>> - uint64_t FunctionHash,
>> - ArrayRef<uint64_t> Counters) {
>> - auto &CounterData = FunctionData[FunctionName];
>> -
>> - auto Where = CounterData.find(FunctionHash);
>> - if (Where == CounterData.end()) {
>> - // We've never seen a function with this name and hash, add it.
>> - CounterData[FunctionHash] = Counters;
>> - // We keep track of the max function count as we go for simplicity.
>> - if (Counters[0] > MaxFunctionCount)
>> - MaxFunctionCount = Counters[0];
>> - return instrprof_error::success;
>> - }
>> +static std::error_code combineInstrProfRecords(
>> + IndexedInstrProfRecord &Dest, const InstrProfRecord &Source,
>> + uint64_t &MaxFunctionCount) {
>>
>> - // We're updating a function we've seen before.
>> - auto &FoundCounters = Where->second;
>> - // If the number of counters doesn't match we either have bad data or a hash
>> - // collision.
>> - if (FoundCounters.size() != Counters.size())
>> + // If the number of counters doesn't match we either have bad data
>> + // or a hash collision.
>> + if (Dest.Counts.size() != Source.Counts.size())
>> return instrprof_error::count_mismatch;
>>
>> - for (size_t I = 0, E = Counters.size(); I < E; ++I) {
>> - if (FoundCounters[I] + Counters[I] < FoundCounters[I])
>> + for (size_t I = 0, E = Source.Counts.size(); I < E; ++I) {
>> + if (Dest.Counts[I] + Source.Counts[I] < Dest.Counts[I])
>> return instrprof_error::counter_overflow;
>> - FoundCounters[I] += Counters[I];
>> + Dest.Counts[I] += Source.Counts[I];
>> + }
>> +
>> + for (uint32_t Kind = instrprof_value_kind::first;
>> + Kind < instrprof_value_kind::size; ++Kind) {
>> + std::vector<InstrProfValueRecord>::const_iterator I =
>> + Source.Values[Kind].begin();
>> + for ( ; I != Source.Values[Kind].end(); ++I) {
>> + bool Found = false;
>> + std::vector<InstrProfValueRecord>::iterator J = Dest.Values[Kind].begin();
>> + for ( ; J != Dest.Values[Kind].end(); ++J) {
>> + if (J->CounterIndex == I->CounterIndex) {
>> + if ((Kind == instrprof_value_kind::indirect_call_target) &&
>> + (0 == J->Name.compare(I->Name)))
>> + Found = true;
>> + else if (J->Value == I->Value)
>> + Found = true;
>> +
>> + if (Found) {
>> + J->NumTaken += I->NumTaken;
>> + break;
>> + }
>> + }
>> + }
>> + if (!Found) Dest.Values[Kind].push_back(*I);
>> + }
>> }
>> +
>> // We keep track of the max function count as we go for simplicity.
>> - if (FoundCounters[0] > MaxFunctionCount)
>> - MaxFunctionCount = FoundCounters[0];
>> + if (Dest.Counts[0] > MaxFunctionCount)
>> + MaxFunctionCount = Dest.Counts[0];
>>
>> return instrprof_error::success;
>> }
>>
>> +std::error_code
>> +InstrProfWriter::addRecord(const InstrProfRecord &I) {
>> + auto &ProfileDataMap = FunctionData[I.Name];
>> +
>> + auto Where = ProfileDataMap.find(I.Hash);
>> + if (Where == ProfileDataMap.end()) {
>> + // We've never seen a function with this name and hash, add it.
>> + ProfileDataMap[I.Hash] = IndexedInstrProfRecord(I.Counts);
>> + for (uint32_t Kind = instrprof_value_kind::first;
>> + Kind < instrprof_value_kind::size; ++Kind)
>> + ProfileDataMap[I.Hash].Values[Kind] = I.Values[Kind];
>> +
>> + // We keep track of the max function count as we go for simplicity.
>> + if (I.Counts[0] > MaxFunctionCount)
>> + MaxFunctionCount = I.Counts[0];
>> + return instrprof_error::success;
>> + }
>> +
>> + // We're updating a function we've seen before.
>> + return combineInstrProfRecords(Where->second, I, MaxFunctionCount);
>> +}
>> +
>> std::pair<uint64_t, uint64_t> InstrProfWriter::writeImpl(raw_ostream &OS) {
>> OnDiskChainedHashTableGenerator<InstrProfRecordTrait> Generator;
>>
>> Index: tools/llvm-profdata/llvm-profdata.cpp
>> ===================================================================
>> --- tools/llvm-profdata/llvm-profdata.cpp
>> +++ tools/llvm-profdata/llvm-profdata.cpp
>> @@ -59,8 +59,7 @@
>>
>> auto Reader = std::move(ReaderOrErr.get());
>> for (const auto &I : *Reader)
>> - if (std::error_code EC =
>> - Writer.addFunctionCounts(I.Name, I.Hash, I.Counts))
>> + if (std::error_code EC = Writer.addRecord(I))
>> errs() << Filename << ": " << I.Name << ": " << EC.message() << "\n";
>> if (Reader->hasError())
>> exitWithError(Reader->getError().message(), Filename);
>> Index: unittests/ProfileData/CoverageMappingTest.cpp
>> ===================================================================
>> --- unittests/ProfileData/CoverageMappingTest.cpp
>> +++ unittests/ProfileData/CoverageMappingTest.cpp
>> @@ -188,7 +188,8 @@
>> }
>>
>> TEST_F(CoverageMappingTest, basic_coverage_iteration) {
>> - ProfileWriter.addFunctionCounts("func", 0x1234, {30, 20, 10, 0});
>> + InstrProfRecord Record("func", 0x1234, {30, 20, 10, 0});
>> + ProfileWriter.addRecord(Record);
>> readProfCounts();
>>
>> addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
>> @@ -238,7 +239,8 @@
>> }
>>
>> TEST_F(CoverageMappingTest, combine_regions) {
>> - ProfileWriter.addFunctionCounts("func", 0x1234, {10, 20, 30});
>> + InstrProfRecord Record("func", 0x1234, {10, 20, 30});
>> + ProfileWriter.addRecord(Record);
>> readProfCounts();
>>
>> addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
>> @@ -256,7 +258,8 @@
>> }
>>
>> TEST_F(CoverageMappingTest, dont_combine_expansions) {
>> - ProfileWriter.addFunctionCounts("func", 0x1234, {10, 20});
>> + InstrProfRecord Record("func", 0x1234, {10, 20});
>> + ProfileWriter.addRecord(Record);
>> readProfCounts();
>>
>> addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
>> @@ -275,7 +278,8 @@
>> }
>>
>> TEST_F(CoverageMappingTest, strip_filename_prefix) {
>> - ProfileWriter.addFunctionCounts("file1:func", 0x1234, {10});
>> + InstrProfRecord Record("file1:func", 0x1234, {10});
>> + ProfileWriter.addRecord(Record);
>> readProfCounts();
>>
>> addCMR(Counter::getCounter(0), "file1", 1, 1, 9, 9);
>> Index: unittests/ProfileData/InstrProfTest.cpp
>> ===================================================================
>> --- unittests/ProfileData/InstrProfTest.cpp
>> +++ unittests/ProfileData/InstrProfTest.cpp
>> @@ -50,7 +50,8 @@
>> }
>>
>> TEST_F(InstrProfTest, write_and_read_one_function) {
>> - Writer.addFunctionCounts("foo", 0x1234, {1, 2, 3, 4});
>> + InstrProfRecord Record("foo", 0x1234, {1, 2, 3, 4});
>> + Writer.addRecord(Record);
>> auto Profile = Writer.writeBuffer();
>> readProfile(std::move(Profile));
>>
>> @@ -67,13 +68,16 @@
>> }
>>
>> TEST_F(InstrProfTest, get_function_counts) {
>> - Writer.addFunctionCounts("foo", 0x1234, {1, 2});
>> - Writer.addFunctionCounts("foo", 0x1235, {3, 4});
>> + InstrProfRecord Record1("foo", 0x1234, {1, 2});
>> + InstrProfRecord Record2("foo", 0x1235, {3, 4});
>> + Writer.addRecord(Record1);
>> + Writer.addRecord(Record2);
>> auto Profile = Writer.writeBuffer();
>> readProfile(std::move(Profile));
>>
>> std::vector<uint64_t> Counts;
>> - ASSERT_TRUE(NoError(Reader->getFunctionCounts("foo", 0x1234, Counts)));
>> + ASSERT_TRUE(NoError(
>> + Reader->getFunctionCounts("foo", 0x1234, Counts)));
>> ASSERT_EQ(2U, Counts.size());
>> ASSERT_EQ(1U, Counts[0]);
>> ASSERT_EQ(2U, Counts[1]);
>> @@ -92,9 +96,12 @@
>> }
>>
>> TEST_F(InstrProfTest, get_max_function_count) {
>> - Writer.addFunctionCounts("foo", 0x1234, {1ULL << 31, 2});
>> - Writer.addFunctionCounts("bar", 0, {1ULL << 63});
>> - Writer.addFunctionCounts("baz", 0x5678, {0, 0, 0, 0});
>> + InstrProfRecord Record1("foo", 0x1234, {1ULL << 31, 2});
>> + InstrProfRecord Record2("bar", 0, {1ULL << 63});
>> + InstrProfRecord Record3("baz", 0x5678, {0, 0, 0, 0});
>> + Writer.addRecord(Record1);
>> + Writer.addRecord(Record2);
>> + Writer.addRecord(Record3);
>> auto Profile = Writer.writeBuffer();
>> readProfile(std::move(Profile));
>>
More information about the llvm-commits
mailing list