[wasm][debug] Delta-encode the debug side table

For functions with a very large stack, the debug side table repeats a
lot of information: Most values will be spilled to the stack, still
every single entry in the debug side table repeats information about
them (type, stack offset). This leads to the size of the debug side
table to be quadratic in the size of the function.

In the linked bug, the generation of the debug side table took ~400ms,
whereas Liftoff compilation alone just took 16ms.

This CL optimized the debug side table by delta-encoding the entries,
i.e. only storing stack slots that changed. This reduces the size of the
table significantly, at the cost of making lookup slower, since that now
has to search the table backwards for the last entry that had
information about a specific slot. For now, this seems like a good
compromise. If it turns out to be a problem, we could speed up the
lookup by either forcing a full dump of the stack state after N entries,
or by dynamically inserting new entries during lookup, whenever we find
that we had to search backwards more than N entries. That would speed up
subsequent lookups then.

On the reproducer in the linked bug, this change reduces the time to
generate the debug side table from ~400ms to ~120ms.
Before this CL, the debug side table has 13,314 entries with a total of
38,599,606 stack value entries. After this CL, it shrinks to 20,037
stack value entries in the 13,314 entries (average of ~1.5 instead of
~2,899).

R=thibaudm@chromium.org

Bug: chromium:1172299
Change-Id: Ie726bb82d4c6648cc9ebd130115ee7ab3d1d551b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2676636Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72558}

src/wasm/baseline/liftoff-compiler.cc

@@ -156,6 +156,9 @@ constexpr LiftoffCondition GetCompareCondition(WasmOpcode opcode) {
 
 // Builds a {DebugSideTable}.
 class DebugSideTableBuilder {
+  using Entry = DebugSideTable::Entry;
+  using Value = Entry::Value;
+
  public:
   enum AssumeSpilling {
     // All register values will be spilled before the pc covered by the debug
@@ -170,12 +173,28 @@ class DebugSideTableBuilder {
 
   class EntryBuilder {
    public:
-    explicit EntryBuilder(int pc_offset,
-                          std::vector<DebugSideTable::Entry::Value> values)
-        : pc_offset_(pc_offset), values_(std::move(values)) {}
-
-    DebugSideTable::Entry ToTableEntry() {
-      return DebugSideTable::Entry{pc_offset_, std::move(values_)};
+    explicit EntryBuilder(int pc_offset, int stack_height,
+                          std::vector<Value> changed_values)
+        : pc_offset_(pc_offset),
+          stack_height_(stack_height),
+          changed_values_(std::move(changed_values)) {}
+
+    Entry ToTableEntry() {
+      return Entry{pc_offset_, stack_height_, std::move(changed_values_)};
+    }
+
+    void MinimizeBasedOnPreviousStack(const std::vector<Value>& last_values) {
+      auto dst = changed_values_.begin();
+      auto end = changed_values_.end();
+      for (auto src = dst; src != end; ++src) {
+        if (src->index < static_cast<int>(last_values.size()) &&
+            *src == last_values[src->index]) {
+          continue;
+        }
+        if (dst != src) *dst = *src;
+        ++dst;
+      }
+      changed_values_.erase(dst, end);
     }
 
     int pc_offset() const { return pc_offset_; }
@@ -183,65 +202,109 @@ class DebugSideTableBuilder {
 
    private:
     int pc_offset_;
-    std::vector<DebugSideTable::Entry::Value> values_;
+    int stack_height_;
+    std::vector<Value> changed_values_;
   };
 
-  // Adds a new entry, and returns a pointer to a builder for modifying that
-  // entry.
-  EntryBuilder* NewEntry(int pc_offset, int stack_height,
-                         LiftoffAssembler::VarState* stack_state,
-                         AssumeSpilling assume_spilling) {
-    // Record stack types.
-    std::vector<DebugSideTable::Entry::Value> values(stack_height);
-    for (int i = 0; i < stack_height; ++i) {
-      const auto& slot = stack_state[i];
-      values[i].type = slot.type();
-      values[i].stack_offset = slot.offset();
+  // Adds a new entry in regular code.
+  void NewEntry(int pc_offset, Vector<LiftoffAssembler::VarState> stack_state,
+                AssumeSpilling assume_spilling) {
+    entries_.emplace_back(
+        pc_offset, static_cast<int>(stack_state.size()),
+        GetChangedStackValues(last_values_, stack_state, assume_spilling));
+  }
+
+  // Adds a new entry for OOL code, and returns a pointer to a builder for
+  // modifying that entry.
+  EntryBuilder* NewOOLEntry(Vector<LiftoffAssembler::VarState> stack_state,
+                            AssumeSpilling assume_spilling) {
+    constexpr int kNoPcOffsetYet = -1;
+    ool_entries_.emplace_back(
+        kNoPcOffsetYet, static_cast<int>(stack_state.size()),
+        GetChangedStackValues(last_ool_values_, stack_state, assume_spilling));
+    return &ool_entries_.back();
+  }
+
+  void SetNumLocals(int num_locals) {
+    DCHECK_EQ(-1, num_locals_);
+    DCHECK_LE(0, num_locals);
+    num_locals_ = num_locals;
+  }
+
+  std::unique_ptr<DebugSideTable> GenerateDebugSideTable() {
+    DCHECK_LE(0, num_locals_);
+
+    // Connect {entries_} and {ool_entries_} by removing redundant stack
+    // information from the first {ool_entries_} entry (based on
+    // {last_values_}).
+    if (!entries_.empty() && !ool_entries_.empty()) {
+      ool_entries_.front().MinimizeBasedOnPreviousStack(last_values_);
+    }
+
+    std::vector<Entry> entries;
+    entries.reserve(entries_.size() + ool_entries_.size());
+    for (auto& entry : entries_) entries.push_back(entry.ToTableEntry());
+    for (auto& entry : ool_entries_) entries.push_back(entry.ToTableEntry());
+    DCHECK(std::is_sorted(
+        entries.begin(), entries.end(),
+        [](Entry& a, Entry& b) { return a.pc_offset() < b.pc_offset(); }));
+    return std::make_unique<DebugSideTable>(num_locals_, std::move(entries));
+  }
+
+ private:
+  static std::vector<Value> GetChangedStackValues(
+      std::vector<Value>& last_values,
+      Vector<LiftoffAssembler::VarState> stack_state,
+      AssumeSpilling assume_spilling) {
+    std::vector<Value> changed_values;
+    int old_stack_size = static_cast<int>(last_values.size());
+    last_values.resize(stack_state.size());
+
+    int index = 0;
+    for (const auto& slot : stack_state) {
+      Value new_value;
+      new_value.index = index;
+      new_value.type = slot.type();
       switch (slot.loc()) {
         case kIntConst:
-          values[i].kind = DebugSideTable::Entry::kConstant;
-          values[i].i32_const = slot.i32_const();
+          new_value.kind = Entry::kConstant;
+          new_value.i32_const = slot.i32_const();
           break;
         case kRegister:
           DCHECK_NE(kDidSpill, assume_spilling);
           if (assume_spilling == kAllowRegisters) {
-            values[i].kind = DebugSideTable::Entry::kRegister;
-            values[i].reg_code = slot.reg().liftoff_code();
+            new_value.kind = Entry::kRegister;
+            new_value.reg_code = slot.reg().liftoff_code();
             break;
           }
           DCHECK_EQ(kAssumeSpilling, assume_spilling);
           V8_FALLTHROUGH;
         case kStack:
-          values[i].kind = DebugSideTable::Entry::kStack;
-          values[i].stack_offset = slot.offset();
+          new_value.kind = Entry::kStack;
+          new_value.stack_offset = slot.offset();
           break;
       }
-    }
-    entries_.emplace_back(pc_offset, std::move(values));
-    return &entries_.back();
-  }
-
-  void SetNumLocals(int num_locals) {
-    DCHECK_EQ(-1, num_locals_);
-    DCHECK_LE(0, num_locals);
-    num_locals_ = num_locals;
-  }
 
-  std::unique_ptr<DebugSideTable> GenerateDebugSideTable() {
-    DCHECK_LE(0, num_locals_);
-    std::vector<DebugSideTable::Entry> entries;
-    entries.reserve(entries_.size());
-    for (auto& entry : entries_) entries.push_back(entry.ToTableEntry());
-    std::sort(entries.begin(), entries.end(),
-              [](DebugSideTable::Entry& a, DebugSideTable::Entry& b) {
-                return a.pc_offset() < b.pc_offset();
-              });
-    return std::make_unique<DebugSideTable>(num_locals_, std::move(entries));
+      if (index >= old_stack_size || last_values[index] != new_value) {
+        changed_values.push_back(new_value);
+        last_values[index] = new_value;
+      }
+      ++index;
+    }
+    return changed_values;
   }
 
- private:
   int num_locals_ = -1;
-  std::list<EntryBuilder> entries_;
+  // Keep a snapshot of the stack of the last entry, to generate a delta to the
+  // next entry.
+  std::vector<Value> last_values_;
+  std::vector<EntryBuilder> entries_;
+  // Keep OOL code entries separate so we can do proper delta-encoding (more
+  // entries might be added between the existing {entries_} and the
+  // {ool_entries_}). Store the entries in a list so the pointer is not
+  // invalidated by adding more entries.
+  std::vector<Value> last_ool_values_;
+  std::list<EntryBuilder> ool_entries_;
 };
 
 void CheckBailoutAllowed(LiftoffBailoutReason reason, const char* detail,
@@ -592,7 +655,7 @@ class LiftoffCompiler {
     }
     out_of_line_code_.push_back(OutOfLineCode::StackCheck(
         position, regs_to_save, spilled_regs, safepoint_info,
-        RegisterDebugSideTableEntry(DebugSideTableBuilder::kAssumeSpilling)));
+        RegisterOOLDebugSideTableEntry()));
     OutOfLineCode& ool = out_of_line_code_.back();
     LOAD_INSTANCE_FIELD(limit_address, StackLimitAddress, kSystemPointerSize);
     __ StackCheck(ool.label.get(), limit_address);
@@ -2313,8 +2376,7 @@ class LiftoffCompiler {
         stub, position,
         V8_UNLIKELY(for_debugging_) ? GetSpilledRegistersForInspection()
                                     : nullptr,
-        safepoint_info, pc,
-        RegisterDebugSideTableEntry(DebugSideTableBuilder::kAssumeSpilling)));
+        safepoint_info, pc, RegisterOOLDebugSideTableEntry()));
     return out_of_line_code_.back().label.get();
   }
 
@@ -2742,13 +2804,19 @@ class LiftoffCompiler {
     __ PushRegister(kWasmI32, result);
   }
 
-  DebugSideTableBuilder::EntryBuilder* RegisterDebugSideTableEntry(
+  void RegisterDebugSideTableEntry(
       DebugSideTableBuilder::AssumeSpilling assume_spilling) {
+    if (V8_LIKELY(!debug_sidetable_builder_)) return;
+    debug_sidetable_builder_->NewEntry(__ pc_offset(),
+                                       VectorOf(__ cache_state()->stack_state),
+                                       assume_spilling);
+  }
+
+  DebugSideTableBuilder::EntryBuilder* RegisterOOLDebugSideTableEntry() {
     if (V8_LIKELY(!debug_sidetable_builder_)) return nullptr;
-    int stack_height = static_cast<int>(__ cache_state()->stack_height());
-    return debug_sidetable_builder_->NewEntry(
-        __ pc_offset(), stack_height, __ cache_state()->stack_state.begin(),
-        assume_spilling);
+    return debug_sidetable_builder_->NewOOLEntry(
+        VectorOf(__ cache_state()->stack_state),
+        DebugSideTableBuilder::kAssumeSpilling);
   }
 
   enum CallKind : bool { kReturnCall = true, kNoReturnCall = false };

src/wasm/wasm-debug.cc

@@ -92,8 +92,9 @@ void DebugSideTable::Print(std::ostream& os) const {
 }
 
 void DebugSideTable::Entry::Print(std::ostream& os) const {
-  os << std::setw(6) << std::hex << pc_offset_ << std::dec << " [";
-  for (auto& value : values_) {
+  os << std::setw(6) << std::hex << pc_offset_ << std::dec << " stack height "
+     << stack_height_ << " [";
+  for (auto& value : changed_values_) {
     os << " " << value.type.name() << ":";
     switch (value.kind) {
       case kConstant:
@@ -127,25 +128,26 @@ class DebugInfoImpl {
   WasmValue GetLocalValue(int local, Address pc, Address fp,
                           Address debug_break_fp) {
     FrameInspectionScope scope(this, pc);
-    return GetValue(scope.debug_side_table_entry, local, fp, debug_break_fp);
+    return GetValue(scope.debug_side_table, scope.debug_side_table_entry, local,
+                    fp, debug_break_fp);
   }
 
   int GetStackDepth(Address pc) {
     FrameInspectionScope scope(this, pc);
     if (!scope.is_inspectable()) return 0;
-    int num_locals = static_cast<int>(scope.debug_side_table->num_locals());
-    int value_count = scope.debug_side_table_entry->num_values();
-    return value_count - num_locals;
+    int num_locals = scope.debug_side_table->num_locals();
+    int stack_height = scope.debug_side_table_entry->stack_height();
+    return stack_height - num_locals;
   }
 
   WasmValue GetStackValue(int index, Address pc, Address fp,
                           Address debug_break_fp) {
     FrameInspectionScope scope(this, pc);
-    int num_locals = static_cast<int>(scope.debug_side_table->num_locals());
-    int value_count = scope.debug_side_table_entry->num_values();
+    int num_locals = scope.debug_side_table->num_locals();
+    int value_count = scope.debug_side_table_entry->stack_height();
     if (num_locals + index >= value_count) return {};
-    return GetValue(scope.debug_side_table_entry, num_locals + index, fp,
-                    debug_break_fp);
+    return GetValue(scope.debug_side_table, scope.debug_side_table_entry,
+                    num_locals + index, fp, debug_break_fp);
   }
 
   const WasmFunction& GetFunctionAtAddress(Address pc) {
@@ -501,35 +503,34 @@ class DebugInfoImpl {
 
   // Get the value of a local (including parameters) or stack value. Stack
   // values follow the locals in the same index space.
-  WasmValue GetValue(const DebugSideTable::Entry* debug_side_table_entry,
+  WasmValue GetValue(const DebugSideTable* debug_side_table,
+                     const DebugSideTable::Entry* debug_side_table_entry,
                      int index, Address stack_frame_base,
                      Address debug_break_fp) const {
-    ValueType type = debug_side_table_entry->value_type(index);
-    if (debug_side_table_entry->is_constant(index)) {
-      DCHECK(type == kWasmI32 || type == kWasmI64);
-      return type == kWasmI32
-                 ? WasmValue(debug_side_table_entry->i32_constant(index))
-                 : WasmValue(
-                       int64_t{debug_side_table_entry->i32_constant(index)});
+    const auto* value =
+        debug_side_table->FindValue(debug_side_table_entry, index);
+    if (value->is_constant()) {
+      DCHECK(value->type == kWasmI32 || value->type == kWasmI64);
+      return value->type == kWasmI32 ? WasmValue(value->i32_const)
+                                     : WasmValue(int64_t{value->i32_const});
     }
 
-    if (debug_side_table_entry->is_register(index)) {
-      LiftoffRegister reg = LiftoffRegister::from_liftoff_code(
-          debug_side_table_entry->register_code(index));
+    if (value->is_register()) {
+      auto reg = LiftoffRegister::from_liftoff_code(value->reg_code);
       auto gp_addr = [debug_break_fp](Register reg) {
         return debug_break_fp +
                WasmDebugBreakFrameConstants::GetPushedGpRegisterOffset(
                    reg.code());
       };
       if (reg.is_gp_pair()) {
-        DCHECK_EQ(kWasmI64, type);
+        DCHECK_EQ(kWasmI64, value->type);
         uint32_t low_word = ReadUnalignedValue<uint32_t>(gp_addr(reg.low_gp()));
         uint32_t high_word =
             ReadUnalignedValue<uint32_t>(gp_addr(reg.high_gp()));
         return WasmValue((uint64_t{high_word} << 32) | low_word);
       }
       if (reg.is_gp()) {
-        return type == kWasmI32
+        return value->type == kWasmI32
                    ? WasmValue(ReadUnalignedValue<uint32_t>(gp_addr(reg.gp())))
                    : WasmValue(ReadUnalignedValue<uint64_t>(gp_addr(reg.gp())));
       }
@@ -543,11 +544,11 @@ class DebugInfoImpl {
       Address spilled_addr =
           debug_break_fp +
           WasmDebugBreakFrameConstants::GetPushedFpRegisterOffset(code);
-      if (type == kWasmF32) {
+      if (value->type == kWasmF32) {
         return WasmValue(ReadUnalignedValue<float>(spilled_addr));
-      } else if (type == kWasmF64) {
+      } else if (value->type == kWasmF64) {
         return WasmValue(ReadUnalignedValue<double>(spilled_addr));
-      } else if (type == kWasmS128) {
+      } else if (value->type == kWasmS128) {
         return WasmValue(Simd128(ReadUnalignedValue<int16>(spilled_addr)));
       } else {
         // All other cases should have been handled above.
@@ -556,9 +557,8 @@ class DebugInfoImpl {
     }
 
     // Otherwise load the value from the stack.
-    Address stack_address =
-        stack_frame_base - debug_side_table_entry->stack_offset(index);
-    switch (type.kind()) {
+    Address stack_address = stack_frame_base - value->stack_offset;
+    switch (value->type.kind()) {
       case ValueType::kI32:
         return WasmValue(ReadUnalignedValue<int32_t>(stack_address));
       case ValueType::kI64:

src/wasm/wasm-debug.h

@@ -13,6 +13,7 @@
 #include "src/base/iterator.h"
 #include "src/base/logging.h"
 #include "src/base/macros.h"
+#include "src/utils/vector.h"
 #include "src/wasm/value-type.h"
 
 namespace v8 {
@@ -20,8 +21,6 @@ namespace internal {
 
 template <typename T>
 class Handle;
-template <typename T>
-class Vector;
 class WasmFrame;
 
 namespace wasm {
@@ -43,6 +42,7 @@ class DebugSideTable {
    public:
     enum ValueKind : int8_t { kConstant, kRegister, kStack };
     struct Value {
+      int index;
       ValueType type;
       ValueKind kind;
       union {
@@ -50,51 +50,61 @@ class DebugSideTable {
         int reg_code;       // if kind == kRegister
         int stack_offset;   // if kind == kStack
       };
+
+      bool operator==(const Value& other) const {
+        if (index != other.index) return false;
+        if (type != other.type) return false;
+        if (kind != other.kind) return false;
+        switch (kind) {
+          case kConstant:
+            return i32_const == other.i32_const;
+          case kRegister:
+            return reg_code == other.reg_code;
+          case kStack:
+            return stack_offset == other.stack_offset;
+        }
+      }
+      bool operator!=(const Value& other) const { return !(*this == other); }
+
+      bool is_constant() const { return kind == kConstant; }
+      bool is_register() const { return kind == kRegister; }
     };
 
-    Entry(int pc_offset, std::vector<Value> values)
-        : pc_offset_(pc_offset), values_(std::move(values)) {}
+    Entry(int pc_offset, int stack_height, std::vector<Value> changed_values)
+        : pc_offset_(pc_offset),
+          stack_height_(stack_height),
+          changed_values_(std::move(changed_values)) {}
 
     // Constructor for map lookups (only initializes the {pc_offset_}).
     explicit Entry(int pc_offset) : pc_offset_(pc_offset) {}
 
     int pc_offset() const { return pc_offset_; }
 
-    int num_values() const { return static_cast<int>(values_.size()); }
-    ValueType value_type(int index) const { return values_[index].type; }
-
-    auto values() const {
-      return base::make_iterator_range(values_.begin(), values_.end());
-    }
+    // Stack height, including locals.
+    int stack_height() const { return stack_height_; }
 
-    int stack_offset(int index) const {
-      DCHECK_EQ(kStack, values_[index].kind);
-      return values_[index].stack_offset;
+    Vector<const Value> changed_values() const {
+      return VectorOf(changed_values_);
     }
 
-    bool is_constant(int index) const {
-      return values_[index].kind == kConstant;
-    }
-
-    bool is_register(int index) const {
-      return values_[index].kind == kRegister;
-    }
-
-    int32_t i32_constant(int index) const {
-      DCHECK_EQ(kConstant, values_[index].kind);
-      return values_[index].i32_const;
-    }
-
-    int32_t register_code(int index) const {
-      DCHECK_EQ(kRegister, values_[index].kind);
-      return values_[index].reg_code;
+    const Value* FindChangedValue(int stack_index) const {
+      DCHECK_GT(stack_height_, stack_index);
+      auto it = std::lower_bound(
+          changed_values_.begin(), changed_values_.end(), stack_index,
+          [](const Value& changed_value, int stack_index) {
+            return changed_value.index < stack_index;
+          });
+      return it != changed_values_.end() && it->index == stack_index ? &*it
+                                                                     : nullptr;
     }
 
     void Print(std::ostream&) const;
 
    private:
     int pc_offset_;
-    std::vector<Value> values_;
+    int stack_height_;
+    // Only store differences from the last entry, to keep the table small.
+    std::vector<Value> changed_values_;
   };
 
   // Technically it would be fine to copy this class, but there should not be a
@@ -111,10 +121,25 @@ class DebugSideTable {
     auto it = std::lower_bound(entries_.begin(), entries_.end(),
                                Entry{pc_offset}, EntryPositionLess{});
     if (it == entries_.end() || it->pc_offset() != pc_offset) return nullptr;
-    DCHECK_LE(num_locals_, it->num_values());
+    DCHECK_LE(num_locals_, it->stack_height());
     return &*it;
   }
 
+  const Entry::Value* FindValue(const Entry* entry, int stack_index) const {
+    while (true) {
+      if (auto* value = entry->FindChangedValue(stack_index)) {
+        // Check that the table was correctly minimized: If the previous stack
+        // also had an entry for {stack_index}, it must be different.
+        DCHECK(entry == &entries_.front() ||
+               (entry - 1)->stack_height() <= stack_index ||
+               *FindValue(entry - 1, stack_index) != *value);
+        return value;
+      }
+      DCHECK_NE(&entries_.front(), entry);
+      --entry;
+    }
+  }
+
   auto entries() const {
     return base::make_iterator_range(entries_.begin(), entries_.end());
   }