[XRay][tools] Pack XRayRecord - reduce memory footprint by a third. (RFC)

[lebedev.ri]

Summary

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

In "basic log mode", we indeed only ever read 8 bits into that field.
But in FDR mode, the CPU field in log is 16 bits.
But if you look in the compiler-rt part, as far as i can tell, the CPU id is always
(in both modes, basic and FDR) received from uint64_t __xray::readTSC(uint8_t &CPU).
So naturally, CPU id is always only 8 bit, and in FDR mode, extra 8 bits is just padding.

Please don't take my word for it, do recheck!

Thus, i do not believe we need to have uint16_t for CPU. With the other current code
we can't ever get more than uint8_t value there, thus we save 1 byte.

The rest of the patch is trivial.
By specifying the base type of RecordTypes we save 3 bytes.

llvm::SmallVector<>/llvm::SmallString only cost 16 bytes each, as opposed to 24/32 bytes.

Thus, in total, old sizeof(XRayRecord) was 88 bytes, and new one is 56 bytes.
There is no padding between the fields of XRayRecord, and XRayRecord itself isn't being
padded when stored into a vector. Thus the footprint of XRayRecord is now optimal.

This is important because XRayRecord is what has the biggest memory footprint,
and most contributes to the peak heap memory usage at least of llvm-xray convert.

Some numbers:

xray-log.llvm-exegesis.FswRtO was acquired from llvm-exegesis
(compiled with -fxray-instruction-threshold=128)
analysis mode over -benchmarks-file with 10099 points (one full
latency measurement set), with normal runtime of 0.387s.

Time old:

$ perf stat -r9 ./bin/llvm-xray convert -sort -symbolize -instr_map=./bin/llvm-exegesis -output-format=trace_event -output=/tmp/trace-old.yml xray-log.llvm-exegesis.FswRtO 

 Performance counter stats for './bin/llvm-xray convert -sort -symbolize -instr_map=./bin/llvm-exegesis -output-format=trace_event -output=/tmp/trace-old.yml xray-log.llvm-exegesis.FswRtO' (9 runs):

           7607.69 msec task-clock                #    0.999 CPUs utilized            ( +-  0.48% )
               522      context-switches          #   68.635 M/sec                    ( +- 39.85% )
                 1      cpu-migrations            #    0.073 M/sec                    ( +- 60.83% )
             77905      page-faults               # 10241.090 M/sec                   ( +-  3.13% )
       30471867671      cycles                    # 4005708.241 GHz                   ( +-  0.48% )  (83.32%)
        2424264020      stalled-cycles-frontend   #    7.96% frontend cycles idle     ( +-  1.84% )  (83.30%)
       11097550400      stalled-cycles-backend    #   36.42% backend cycles idle      ( +-  0.35% )  (33.38%)
       36899274774      instructions              #    1.21  insn per cycle         
                                                  #    0.30  stalled cycles per insn  ( +-  0.07% )  (50.04%)
        6538597488      branches                  # 859537529.125 M/sec               ( +-  0.07% )  (66.70%)
          79769896      branch-misses             #    1.22% of all branches          ( +-  0.67% )  (83.35%)

            7.6143 +- 0.0371 seconds time elapsed  ( +-  0.49% )

Time new:

$ perf stat -r9 ./bin/llvm-xray convert -sort -symbolize -instr_map=./bin/llvm-exegesis -output-format=trace_event -output=/tmp/trace-new.yml xray-log.llvm-exegesis.FswRtO 

 Performance counter stats for './bin/llvm-xray convert -sort -symbolize -instr_map=./bin/llvm-exegesis -output-format=trace_event -output=/tmp/trace-new.yml xray-log.llvm-exegesis.FswRtO' (9 runs):

           7207.49 msec task-clock                #    1.000 CPUs utilized            ( +-  0.46% )
               174      context-switches          #   24.159 M/sec                    ( +- 30.10% )
                 0      cpu-migrations            #    0.062 M/sec                    ( +- 39.53% )
             52126      page-faults               # 7232.740 M/sec                    ( +-  0.69% )
       28876446408      cycles                    # 4006783.905 GHz                   ( +-  0.46% )  (83.31%)
        2352902586      stalled-cycles-frontend   #    8.15% frontend cycles idle     ( +-  2.08% )  (83.33%)
        8986901047      stalled-cycles-backend    #   31.12% backend cycles idle      ( +-  1.00% )  (33.36%)
       38630170181      instructions              #    1.34  insn per cycle         
                                                  #    0.23  stalled cycles per insn  ( +-  0.04% )  (50.02%)
        7016819734      branches                  # 973626739.925 M/sec               ( +-  0.04% )  (66.68%)
          86887572      branch-misses             #    1.24% of all branches          ( +-  0.39% )  (83.33%)

            7.2099 +- 0.0330 seconds time elapsed  ( +-  0.46% )

(Nice, accidentally improved by -5%)

Memory old:

$ heaptrack_print heaptrack.llvm-xray.3976.gz | tail -n 7
total runtime: 18.16s.
bytes allocated in total (ignoring deallocations): 5.25GB (289.03MB/s)
calls to allocation functions: 21840309 (1202792/s)
temporary memory allocations: 228301 (12573/s)
peak heap memory consumption: 354.62MB
peak RSS (including heaptrack overhead): 4.30GB
total memory leaked: 87.42KB

Memory new:

$ heaptrack_print heaptrack.llvm-xray.5234.gz | tail -n 7
total runtime: 17.93s.
bytes allocated in total (ignoring deallocations): 5.05GB (281.73MB/s)
calls to allocation functions: 21840309 (1217747/s)
temporary memory allocations: 228301 (12729/s)
peak heap memory consumption: 267.77MB
peak RSS (including heaptrack overhead): 2.16GB
total memory leaked: 83.50KB

Memory diff:

$ heaptrack_print -d heaptrack.llvm-xray.3976.gz heaptrack.llvm-xray.5234.gz | tail -n 7
total runtime: -0.22s.
bytes allocated in total (ignoring deallocations): -195.36MB (876.07MB/s)
calls to allocation functions: 0 (0/s)
temporary memory allocations: 0 (0/s)
peak heap memory consumption: -86.86MB
peak RSS (including heaptrack overhead): 0B
total memory leaked: -3.92KB

So we indeed improved (reduced) peak memory usage, by ~-25%.
Not by a third since now something else is the top contributor to the peak.

[dberris]

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

So, this is an accident of history, which should be changed, but to the other direction. I've learned some time ago that it turns out there are some platforms that can have enough CPU IDs which can't be represented by a uint8_t. For future-proofing, we really should change this to be larger (uint16_t) and change basic mode to store 16-bit CPU IDs. The other parts seem fine to me, except for the potential churn on the user side (this is an ABI change).

I think that's fine for the C++ APIs, but that it will need some release notes (in case someone has been using the Trace API).

[lebedev.ri]

In D58621#1409704, @dberris wrote:

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

So, this is an accident of history, which should be changed, but to the other direction. I've learned some time ago that it turns out there are some platforms that can have enough CPU IDs which can't be represented by a uint8_t. For future-proofing, we really should change this to be larger (uint16_t) and change basic mode to store 16-bit CPU IDs.

Boo :)
Unfortunately that won't just cost that one extra byte, it will have ripple effect on the padding in this struct.
I'm not sure as to exact numbers.

The other parts seem fine to me, except for the potential churn on the user side (this is an ABI change).

I think that's fine for the C++ APIs, but that it will need some release notes (in case someone has been using the Trace API).

[dberris]

In D58621#1409715, @lebedev.ri wrote:

In D58621#1409704, @dberris wrote:

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

So, this is an accident of history, which should be changed, but to the other direction. I've learned some time ago that it turns out there are some platforms that can have enough CPU IDs which can't be represented by a uint8_t. For future-proofing, we really should change this to be larger (uint16_t) and change basic mode to store 16-bit CPU IDs.

Boo :)

Boo indeed. :)

Unfortunately that won't just cost that one extra byte, it will have ripple effect on the padding in this struct.
I'm not sure as to exact numbers.

I like the idea of reducing top-line memory requirements, but it shouldn't be at the cost of functionality. The current state is a bug that we're only using 8 bits for the CPU ID.

Now, an alternative here is to migrate the Basic Mode implementation to use a more compact log record (i.e. using the FDR mode format), and use a different converter approach, one that doesn't require reconstituting the whole Trace consisting of XRayRecord instances. The FDR log loading libraries/framework allow us to do this now (see llvm-xray fdr-dump). This is a more intensive project but one that isn't terribly hard to accomplish. If you'd like to take that on, I'd be happy to review patches going in that direction instead (really the only difference between the current basic mode implementation and the FDR mode implementation is that, basic mode threads returning a buffer to the queue will write out the contents before returning the buffer to the central buffer queue). In that process we can migrate FDR mode to use 16-bit CPU IDs.

[lebedev.ri]

In D58621#1409962, @dberris wrote:

In D58621#1409715, @lebedev.ri wrote:

In D58621#1409704, @dberris wrote:

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

So, this is an accident of history, which should be changed, but to the other direction. I've learned some time ago that it turns out there are some platforms that can have enough CPU IDs which can't be represented by a uint8_t. For future-proofing, we really should change this to be larger (uint16_t) and change basic mode to store 16-bit CPU IDs.

Boo :)

Boo indeed. :)

Unfortunately that won't just cost that one extra byte, it will have ripple effect on the padding in this struct.
I'm not sure as to exact numbers.

I like the idea of reducing top-line memory requirements, but it shouldn't be at the cost of functionality. The current state is a bug that we're only using 8 bits for the CPU ID.

No, i totally understand.
That is why i said it's RFC and is only valid if it is actually
only ever 8 bits. (which it is, but only due to the bug elsewhere)
I think this is still worth it even with 16-bit CPU, i'll take a look.

Now, an alternative here is to migrate the Basic Mode implementation to use a more compact log record (i.e. using the FDR mode format), and use a different converter approach, one that doesn't require reconstituting the whole Trace consisting of XRayRecord instances. The FDR log loading libraries/framework allow us to do this now (see llvm-xray fdr-dump). This is a more intensive project but one that isn't terribly hard to accomplish. If you'd like to take that on, I'd be happy to review patches going in that direction instead (really the only difference between the current basic mode implementation and the FDR mode implementation is that, basic mode threads returning a buffer to the queue will write out the contents before returning the buffer to the central buffer queue). In that process we can migrate FDR mode to use 16-bit CPU IDs.

So it's basically three co-dependent steps:

1. Teach xray convert to also work on FDR input
2. Afterwards, switch the compiler-rt X-Ray basic log to output FDR log format.
3. Finally, fix the truncation of CPU id in the compiler-rt xray code

I'm presently not constrained by the XRayRecord memory footprint any more,
so i'm not sure how much effort i want to spent here..

[dberris]

In D58621#1410322, @lebedev.ri wrote:

In D58621#1409962, @dberris wrote:

In D58621#1409715, @lebedev.ri wrote:

In D58621#1409704, @dberris wrote:

This is a RFC because of the uint8_t CPU change.
That chance needs discussing.

So, this is an accident of history, which should be changed, but to the other direction. I've learned some time ago that it turns out there are some platforms that can have enough CPU IDs which can't be represented by a uint8_t. For future-proofing, we really should change this to be larger (uint16_t) and change basic mode to store 16-bit CPU IDs.

Boo :)

Boo indeed. :)

Unfortunately that won't just cost that one extra byte, it will have ripple effect on the padding in this struct.
I'm not sure as to exact numbers.

I like the idea of reducing top-line memory requirements, but it shouldn't be at the cost of functionality. The current state is a bug that we're only using 8 bits for the CPU ID.

No, i totally understand.
That is why i said it's RFC and is only valid if it is actually
only ever 8 bits. (which it is, but only due to the bug elsewhere)
I think this is still worth it even with 16-bit CPU, i'll take a look.

Now, an alternative here is to migrate the Basic Mode implementation to use a more compact log record (i.e. using the FDR mode format), and use a different converter approach, one that doesn't require reconstituting the whole Trace consisting of XRayRecord instances. The FDR log loading libraries/framework allow us to do this now (see llvm-xray fdr-dump). This is a more intensive project but one that isn't terribly hard to accomplish. If you'd like to take that on, I'd be happy to review patches going in that direction instead (really the only difference between the current basic mode implementation and the FDR mode implementation is that, basic mode threads returning a buffer to the queue will write out the contents before returning the buffer to the central buffer queue). In that process we can migrate FDR mode to use 16-bit CPU IDs.

So it's basically three co-dependent steps:

1. Teach xray convert to also work on FDR input
2. Afterwards, switch the compiler-rt X-Ray basic log to output FDR log format.
3. Finally, fix the truncation of CPU id in the compiler-rt xray code

Step 1 is not strictly necessary, we already somewhat already do this although indirectly (through the Trace type) in llvm-xray convert. We can then later switch to stream-processing the FDR mode logs when converting, which will be very similar to what the fdr-dump subcommand already does.

Step 2 and 3 can happen in a single change.

I'm presently not constrained by the XRayRecord memory footprint any more,
so i'm not sure how much effort i want to spent here..

This I understand too. :)

[nbaksalyar]

In D58621#1409962, @dberris wrote:

Now, an alternative here is to migrate the Basic Mode implementation to use a more compact log record (i.e. using the FDR mode format), and use a different converter approach <..> This is a more intensive project but one that isn't terribly hard to accomplish. If you'd like to take that on, I'd be happy to review patches going in that direction instead

@dberris, is this still valid & desirable?
I would like to dip my toes into XRay/LLVM, and this seems like a good first mini project to me. :) This migration should also bring typed and custom events support for the basic mode, which is a nice bonus.

[dberris]

In D58621#1774144, @nbaksalyar wrote:

In D58621#1409962, @dberris wrote:

Now, an alternative here is to migrate the Basic Mode implementation to use a more compact log record (i.e. using the FDR mode format), and use a different converter approach <..> This is a more intensive project but one that isn't terribly hard to accomplish. If you'd like to take that on, I'd be happy to review patches going in that direction instead

@dberris, is this still valid & desirable?
I would like to dip my toes into XRay/LLVM, and this seems like a good first mini project to me. :) This migration should also bring typed and custom events support for the basic mode, which is a nice bonus.

Yes, still valid and still desirable. :)

include/llvm/XRay/XRayRecord.h

 //===- XRayRecord.h - XRay Trace Record -----------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file replicates the record definition for XRay log entries. This should
 // follow the evolution of the log record versions supported in the compiler-rt
 // xray project.
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_XRAY_XRAY_RECORD_H
 #define LLVM_XRAY_XRAY_RECORD_H
 
+#include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include <cstdint>
-#include <vector>
-#include <string>
 
 namespace llvm {
 namespace xray {
 
 /// XRay traces all have a header providing some top-matter information useful
 /// to help tools determine how to interpret the information available in the
 /// trace.
 struct XRayFileHeader {
struct XRayFileHeader {
   // buffer.
   char FreeFormData[16];
 };
 
 /// Determines the supported types of records that could be seen in XRay traces.
 /// This may or may not correspond to actual record types in the raw trace (as
 /// the loader implementation may synthesize this information in the process of
 /// of loading).
-enum class RecordTypes {
+enum class RecordTypes : uint8_t {
   ENTER,
   EXIT,
   TAIL_EXIT,
   ENTER_ARG,
   CUSTOM_EVENT,
   TYPED_EVENT
 };
 
 /// An XRayRecord is the denormalized view of data associated in a trace. These
 /// records may not correspond to actual entries in the raw traces, but they are
 /// the logical representation of records in a higher-level event log.
 struct XRayRecord {
   /// RecordType values are used as "sub-types" which have meaning in the
   /// context of the `Type` below. For function call and custom event records,
   /// the RecordType is always 0, while for typed events we store the type in
   /// the RecordType field.
   uint16_t RecordType;
 
-  /// The CPU where the thread is running. We assume number of CPUs <= 65536.
-  uint16_t CPU;
+  /// The CPU where the thread is running. We assume number of CPUs <= 255.
+  uint8_t CPU;
 
   /// Identifies the type of record.
   RecordTypes Type;
 
   /// The function ID for the record, if this is a function call record.
   int32_t FuncId;
 
   /// Get the full 8 bytes of the TSC when we get the log record.
   uint64_t TSC;
 
   /// The thread ID for the currently running thread.
   uint32_t TId;
 
   /// The process ID for the currently running process.
   uint32_t PId;
 
   /// The function call arguments.
-  std::vector<uint64_t> CallArgs;
+  llvm::SmallVector<uint64_t, 0> CallArgs;
 
   /// For custom and typed events, we provide the raw data from the trace.
-  std::string Data;
+  llvm::SmallString<0> Data;
 };
+static_assert(sizeof(XRayRecord) <= 56, "XRayRecord should be small");
+static_assert(sizeof(XRayRecord) % alignof(XRayRecord) == 0,
+              "Should not have any padding between two XRayRecord");
 
 } // namespace xray
 } // namespace llvm
 
 #endif // LLVM_XRAY_XRAY_RECORD_H

include/llvm/XRay/YAMLXRayRecord.h

struct YAMLXRayFileHeader {
   uint16_t Type;
   bool ConstantTSC;
   bool NonstopTSC;
   uint64_t CycleFrequency;
 };
 
 struct YAMLXRayRecord {
   uint16_t RecordType;
-  uint16_t CPU;
+  uint8_t CPU;
   RecordTypes Type;
   int32_t FuncId;
   std::string Function;
   uint64_t TSC;
   uint32_t TId;
   uint32_t PId;
   std::vector<uint64_t> CallArgs;
   std::string Data;

lib/XRay/Trace.cpp

Error loadYAMLLog(StringRef Data, XRayFileHeader &FileHeader,
   if (FileHeader.Version != 1)
     return make_error<StringError>(
         Twine("Unsupported XRay file version: ") + Twine(FileHeader.Version),
         std::make_error_code(std::errc::invalid_argument));
 
   Records.clear();
   std::transform(Trace.Records.begin(), Trace.Records.end(),
                  std::back_inserter(Records), [&](const YAMLXRayRecord &R) {
-                   return XRayRecord{R.RecordType, R.CPU,      R.Type,
-                                     R.FuncId,     R.TSC,      R.TId,
-                                     R.PId,        R.CallArgs, R.Data};
+                   return XRayRecord{R.RecordType,
+                                     R.CPU,
+                                     R.Type,
+                                     R.FuncId,
+                                     R.TSC,
+                                     R.TId,
+                                     R.PId,
+                                     llvm::SmallVector<uint64_t, 0>{
+                                         R.CallArgs.begin(), R.CallArgs.end()},
+                                     StringRef(R.Data)};
                  });
   return Error::success();
 }
 } // namespace
 
 Expected<Trace> llvm::xray::loadTraceFile(StringRef Filename, bool Sort) {
   int Fd;
   if (auto EC = sys::fs::openFileForRead(Filename, Fd)) {

tools/llvm-xray/xray-converter.cpp

 
 void TraceConverter::exportAsYAML(const Trace &Records, raw_ostream &OS) {
   YAMLXRayTrace Trace;
   const auto &FH = Records.getFileHeader();
   Trace.Header = {FH.Version, FH.Type, FH.ConstantTSC, FH.NonstopTSC,
                   FH.CycleFrequency};
   Trace.Records.reserve(Records.size());
   for (const auto &R : Records) {
-    Trace.Records.push_back({R.RecordType, R.CPU, R.Type, R.FuncId,
-                             Symbolize ? FuncIdHelper.SymbolOrNumber(R.FuncId)
-                                       : llvm::to_string(R.FuncId),
-                             R.TSC, R.TId, R.PId, R.CallArgs, R.Data});
+    Trace.Records.emplace_back(YAMLXRayRecord{
+        R.RecordType, R.CPU, R.Type, R.FuncId,
+        R.TSC, R.TId, R.PId, ArrayRef<uint64_t>(R.CallArgs), R.Data.str()});
   }
   Output Out(OS, nullptr, 0);
   Out.setWriteDefaultValues(false);
   Out << Trace;
 }
 
 void TraceConverter::exportAsRAWv1(const Trace &Records, raw_ostream &OS) {
   // First write out the file header, in the correct endian-appropriate format