Encode interpreter::SourcePositionTable as variable-length ints.

This reduces the memory consumption of SourcePositionTable by ca. 2/3.
Over Octane, this reduces the source position table memory consumption
from ~370kB to ~115kB, which makes it ca. 10% of the total bytecode size
(~1.1MB)

----------------

Reland CL in order to relive the glory days, and also fix memory leak w/ ENABLE_SLOW_CHECKS.

SourcePositionTableBuilder used to have a no destructor since everything
was zone allocated. But if ENABLE_SLOW_CHECKS, it has a heap allocated member
and thus needs a proper constructor. ASAN thankfully notices this, and V8 no
longer builds since this is called during mksnapshot.

Breakge example: http://build.chromium.org/p/client.v8/builders/V8%20Linux64%20ASAN%20arm64%20-%20debug%20builder/builds/4829

R=jochen@chromium.org, yangguo@chromium.org, rmcilroy@chromium.org
BUG=v8:4690
LOG=y

Committed: https://crrev.com/a6f41f7b8226555c5900440f6e3092b3545ee0f6
Cr-Commit-Position: refs/heads/master@{#34250}

patch from issue 1704943002 at patchset 200001 (http://crrev.com/1704943002#ps200001)

Review URL: https://codereview.chromium.org/1731883003

Cr-Commit-Position: refs/heads/master@{#34256}

src/debug/debug.cc

@@ -171,8 +171,9 @@ BreakLocation BreakLocation::CodeIterator::GetBreakLocation() {
 BreakLocation::BytecodeArrayIterator::BytecodeArrayIterator(
     Handle<DebugInfo> debug_info, BreakLocatorType type)
     : Iterator(debug_info),
-      source_position_iterator_(
-          debug_info->abstract_code()->GetBytecodeArray()),
+      source_position_iterator_(debug_info->abstract_code()
+                                    ->GetBytecodeArray()
+                                    ->source_position_table()),
       break_locator_type_(type),
       start_position_(debug_info->shared()->start_position()) {
   if (!Done()) Next();

src/heap/heap.cc

@@ -3036,7 +3036,7 @@ AllocationResult Heap::AllocateBytecodeArray(int length,
   instance->set_interrupt_budget(interpreter::Interpreter::InterruptBudget());
   instance->set_constant_pool(constant_pool);
   instance->set_handler_table(empty_fixed_array());
-  instance->set_source_position_table(empty_fixed_array());
+  instance->set_source_position_table(empty_byte_array());
   CopyBytes(instance->GetFirstBytecodeAddress(), raw_bytecodes, length);
 
   return result;

src/interpreter/bytecode-array-builder.cc

@@ -124,8 +124,8 @@ Handle<BytecodeArray> BytecodeArrayBuilder::ToBytecodeArray() {
   int frame_size = register_count * kPointerSize;
   Handle<FixedArray> constant_pool = constant_array_builder()->ToFixedArray();
   Handle<FixedArray> handler_table = handler_table_builder()->ToHandlerTable();
-  Handle<FixedArray> source_position_table =
-      source_position_table_builder()->ToFixedArray();
+  Handle<ByteArray> source_position_table =
+      source_position_table_builder()->ToSourcePositionTable();
   Handle<BytecodeArray> output = isolate_->factory()->NewBytecodeArray(
       bytecode_size, &bytecodes_.front(), frame_size, parameter_count(),
       constant_pool);
@@ -139,10 +139,7 @@ Handle<BytecodeArray> BytecodeArrayBuilder::ToBytecodeArray() {
 template <size_t N>
 void BytecodeArrayBuilder::Output(Bytecode bytecode, uint32_t(&operands)[N]) {
   // Don't output dead code.
-  if (exit_seen_in_block_) {
-    source_position_table_builder_.RevertPosition(bytecodes()->size());
-    return;
-  }
+  if (exit_seen_in_block_) return;
 
   int operand_count = static_cast<int>(N);
   DCHECK_EQ(Bytecodes::NumberOfOperands(bytecode), operand_count);
@@ -210,10 +207,7 @@ void BytecodeArrayBuilder::Output(Bytecode bytecode, uint32_t operand0) {
 
 void BytecodeArrayBuilder::Output(Bytecode bytecode) {
   // Don't output dead code.
-  if (exit_seen_in_block_) {
-    source_position_table_builder_.RevertPosition(bytecodes()->size());
-    return;
-  }
+  if (exit_seen_in_block_) return;
 
   DCHECK_EQ(Bytecodes::NumberOfOperands(bytecode), 0);
   last_bytecode_start_ = bytecodes()->size();
@@ -860,10 +854,7 @@ void BytecodeArrayBuilder::PatchJump(
 BytecodeArrayBuilder& BytecodeArrayBuilder::OutputJump(Bytecode jump_bytecode,
                                                        BytecodeLabel* label) {
   // Don't emit dead code.
-  if (exit_seen_in_block_) {
-    source_position_table_builder_.RevertPosition(bytecodes()->size());
-    return *this;
-  }
+  if (exit_seen_in_block_) return *this;
 
   // Check if the value in accumulator is boolean, if not choose an
   // appropriate JumpIfToBoolean bytecode.
@@ -1057,6 +1048,7 @@ void BytecodeArrayBuilder::EnsureReturn(FunctionLiteral* literal) {
     SetReturnPosition(literal);
     Return();
   }
+  DCHECK(exit_seen_in_block_);
 }
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::Call(Register callable,
@@ -1185,18 +1177,23 @@ size_t BytecodeArrayBuilder::GetConstantPoolEntry(Handle<Object> object) {
 }
 
 void BytecodeArrayBuilder::SetReturnPosition(FunctionLiteral* fun) {
+  // Don't emit dead code.
+  if (exit_seen_in_block_) return;
+
   int pos = std::max(fun->start_position(), fun->end_position() - 1);
   source_position_table_builder_.AddStatementPosition(bytecodes_.size(), pos);
 }
 
 void BytecodeArrayBuilder::SetStatementPosition(Statement* stmt) {
   if (stmt->position() == RelocInfo::kNoPosition) return;
+  if (exit_seen_in_block_) return;
   source_position_table_builder_.AddStatementPosition(bytecodes_.size(),
                                                       stmt->position());
 }
 
 void BytecodeArrayBuilder::SetExpressionPosition(Expression* expr) {
   if (expr->position() == RelocInfo::kNoPosition) return;
+  if (exit_seen_in_block_) return;
   source_position_table_builder_.AddExpressionPosition(bytecodes_.size(),
                                                        expr->position());
 }

src/interpreter/source-position-table.cc

@@ -4,7 +4,6 @@
 
 #include "src/interpreter/source-position-table.h"
 
-#include "src/assembler.h"
 #include "src/objects-inl.h"
 #include "src/objects.h"
 
@@ -12,71 +11,176 @@ namespace v8 {
 namespace internal {
 namespace interpreter {
 
-class IsStatementField : public BitField<bool, 0, 1> {};
-class SourcePositionField : public BitField<int, 1, 30> {};
+// We'll use a simple encoding scheme to record the source positions.
+// Conceptually, each position consists of:
+// - bytecode_offset: An integer index into the BytecodeArray
+// - source_position: An integer index into the source string.
+// - position type: Each position is either a statement or an expression.
+//
+// The basic idea for the encoding is to use a variable-length integer coding,
+// where each byte contains 7 bits of payload data, and 1 'more' bit that
+// determines whether additional bytes follow. Additionally:
+// - we record the difference from the previous position,
+// - we just stuff one bit for the type into the bytecode offset,
+// - we write least-significant bits first,
+// - negative numbers occur only rarely, so we use a denormalized
+//   most-significant byte (a byte with all zeros, which normally wouldn't
+//   make any sense) to encode a negative sign, so that we 'pay' nothing for
+//   positive numbers, but have to pay a full byte for negative integers.
+
+namespace {
+
+// A zero-value in the most-significant byte is used to mark negative numbers.
+const int kNegativeSignMarker = 0;
+
+// Each byte is encoded as MoreBit | ValueBits.
+class MoreBit : public BitField8<bool, 7, 1> {};
+class ValueBits : public BitField8<int, 0, 7> {};
+
+// Helper: Add the offsets from 'other' to 'value'. Also set is_statement.
+void AddAndSetEntry(PositionTableEntry& value,
+                    const PositionTableEntry& other) {
+  value.bytecode_offset += other.bytecode_offset;
+  value.source_position += other.source_position;
+  value.is_statement = other.is_statement;
+}
+
+// Helper: Substract the offsets from 'other' from 'value'.
+void SubtractFromEntry(PositionTableEntry& value,
+                       const PositionTableEntry& other) {
+  value.bytecode_offset -= other.bytecode_offset;
+  value.source_position -= other.source_position;
+}
+
+// Helper: Encode an integer.
+void EncodeInt(ZoneVector<byte>& bytes, int value) {
+  bool sign = false;
+  if (value < 0) {
+    sign = true;
+    value = -value;
+  }
+
+  bool more;
+  do {
+    more = value > ValueBits::kMax;
+    bytes.push_back(MoreBit::encode(more || sign) |
+                    ValueBits::encode(value & ValueBits::kMax));
+    value >>= ValueBits::kSize;
+  } while (more);
+
+  if (sign) {
+    bytes.push_back(MoreBit::encode(false) |
+                    ValueBits::encode(kNegativeSignMarker));
+  }
+}
+
+// Encode a PositionTableEntry.
+void EncodeEntry(ZoneVector<byte>& bytes, const PositionTableEntry& entry) {
+  // 1 bit for sign + is_statement each, which leaves 30b for the value.
+  DCHECK(abs(entry.bytecode_offset) < (1 << 30));
+  EncodeInt(bytes, (entry.is_statement ? 1 : 0) | (entry.bytecode_offset << 1));
+  EncodeInt(bytes, entry.source_position);
+}
+
+// Helper: Decode an integer.
+void DecodeInt(ByteArray* bytes, int* index, int* v) {
+  byte current;
+  int n = 0;
+  int value = 0;
+  bool more;
+  do {
+    current = bytes->get((*index)++);
+    value |= ValueBits::decode(current) << (n * ValueBits::kSize);
+    n++;
+    more = MoreBit::decode(current);
+  } while (more);
+
+  if (ValueBits::decode(current) == kNegativeSignMarker) {
+    value = -value;
+  }
+  *v = value;
+}
+
+void DecodeEntry(ByteArray* bytes, int* index, PositionTableEntry* entry) {
+  int tmp;
+  DecodeInt(bytes, index, &tmp);
+  entry->is_statement = (tmp & 1);
+
+  // Note that '>>' needs to be arithmetic shift in order to handle negative
+  // numbers properly.
+  entry->bytecode_offset = (tmp >> 1);
+
+  DecodeInt(bytes, index, &entry->source_position);
+}
+
+}  // namespace
 
 void SourcePositionTableBuilder::AddStatementPosition(size_t bytecode_offset,
                                                       int source_position) {
-  int offset = static_cast<int>(bytecode_offset);
-  // If a position has already been assigned to this bytecode offset,
-  // do not reassign a new statement position.
-  if (CodeOffsetHasPosition(offset)) return;
-  uint32_t encoded = IsStatementField::encode(true) |
-                     SourcePositionField::encode(source_position);
-  entries_.push_back({offset, encoded});
+  AddEntry({static_cast<int>(bytecode_offset), source_position, true});
 }
 
 void SourcePositionTableBuilder::AddExpressionPosition(size_t bytecode_offset,
                                                        int source_position) {
-  int offset = static_cast<int>(bytecode_offset);
-  // If a position has already been assigned to this bytecode offset,
-  // do not reassign a new statement position.
-  if (CodeOffsetHasPosition(offset)) return;
-  uint32_t encoded = IsStatementField::encode(false) |
-                     SourcePositionField::encode(source_position);
-  entries_.push_back({offset, encoded});
+  AddEntry({static_cast<int>(bytecode_offset), source_position, false});
 }
 
-void SourcePositionTableBuilder::RevertPosition(size_t bytecode_offset) {
-  int offset = static_cast<int>(bytecode_offset);
-  // If we already added a source position table entry, but the bytecode array
-  // builder ended up not outputting a bytecode for the corresponding bytecode
-  // offset, we have to remove that entry.
-  if (CodeOffsetHasPosition(offset)) entries_.pop_back();
+void SourcePositionTableBuilder::AddEntry(const PositionTableEntry& entry) {
+  // Don't encode a new entry if this bytecode already has a source position
+  // assigned.
+  if (bytes_.size() > 0 && previous_.bytecode_offset == entry.bytecode_offset) {
+    return;
+  }
+
+  PositionTableEntry tmp(entry);
+  SubtractFromEntry(tmp, previous_);
+  EncodeEntry(bytes_, tmp);
+  previous_ = entry;
+
+#ifdef ENABLE_SLOW_DCHECKS
+  raw_entries_.push_back(entry);
+#endif
 }
 
-Handle<FixedArray> SourcePositionTableBuilder::ToFixedArray() {
-  int length = static_cast<int>(entries_.size());
-  Handle<FixedArray> table =
-      isolate_->factory()->NewFixedArray(length * 2, TENURED);
-  for (int i = 0; i < length; i++) {
-    table->set(i * 2, Smi::FromInt(entries_[i].bytecode_offset));
-    table->set(i * 2 + 1, Smi::FromInt(entries_[i].source_position_and_type));
+Handle<ByteArray> SourcePositionTableBuilder::ToSourcePositionTable() {
+  Handle<ByteArray> table = isolate_->factory()->NewByteArray(
+      static_cast<int>(bytes_.size()), TENURED);
+  if (bytes_.empty()) return table;
+
+  MemCopy(table->GetDataStartAddress(), &*bytes_.begin(), bytes_.size());
+
+#ifdef ENABLE_SLOW_DCHECKS
+  // Brute force testing: Record all positions and decode
+  // the entire table to verify they are identical.
+  auto raw = raw_entries_.begin();
+  for (SourcePositionTableIterator encoded(*table); !encoded.done();
+       encoded.Advance(), raw++) {
+    DCHECK(raw != raw_entries_.end());
+    DCHECK_EQ(encoded.bytecode_offset(), raw->bytecode_offset);
+    DCHECK_EQ(encoded.source_position(), raw->source_position);
+    DCHECK_EQ(encoded.is_statement(), raw->is_statement);
   }
+  DCHECK(raw == raw_entries_.end());
+#endif
+
   return table;
 }
 
-SourcePositionTableIterator::SourcePositionTableIterator(
-    BytecodeArray* bytecode_array)
-    : table_(bytecode_array->source_position_table()),
-      index_(0),
-      length_(table_->length()) {
-  DCHECK(table_->length() % 2 == 0);
+SourcePositionTableIterator::SourcePositionTableIterator(ByteArray* byte_array)
+    : table_(byte_array), index_(0), current_() {
   Advance();
 }
 
 void SourcePositionTableIterator::Advance() {
-  if (index_ < length_) {
-    int new_bytecode_offset = Smi::cast(table_->get(index_))->value();
-    // Bytecode offsets are in ascending order.
-    DCHECK(bytecode_offset_ < new_bytecode_offset || index_ == 0);
-    bytecode_offset_ = new_bytecode_offset;
-    uint32_t source_position_and_type =
-        static_cast<uint32_t>(Smi::cast(table_->get(index_ + 1))->value());
-    is_statement_ = IsStatementField::decode(source_position_and_type);
-    source_position_ = SourcePositionField::decode(source_position_and_type);
+  DCHECK(!done());
+  DCHECK(index_ >= 0 && index_ <= table_->length());
+  if (index_ == table_->length()) {
+    index_ = kDone;
+  } else {
+    PositionTableEntry tmp;
+    DecodeEntry(table_, &index_, &tmp);
+    AddAndSetEntry(current_, tmp);
   }
-  index_ += 2;
 }
 
 }  // namespace interpreter

src/interpreter/source-position-table.h

@@ -6,72 +6,85 @@
 #define V8_INTERPRETER_SOURCE_POSITION_TABLE_H_
 
 #include "src/assert-scope.h"
+#include "src/checks.h"
 #include "src/handles.h"
-#include "src/zone.h"
 #include "src/zone-containers.h"
 
 namespace v8 {
 namespace internal {
 
 class BytecodeArray;
-class FixedArray;
+class ByteArray;
 class Isolate;
+class Zone;
 
 namespace interpreter {
 
+struct PositionTableEntry {
+  PositionTableEntry()
+      : bytecode_offset(0), source_position(0), is_statement(false) {}
+  PositionTableEntry(int bytecode, int source, bool statement)
+      : bytecode_offset(bytecode),
+        source_position(source),
+        is_statement(statement) {}
+
+  int bytecode_offset;
+  int source_position;
+  bool is_statement;
+};
+
 class SourcePositionTableBuilder {
  public:
   explicit SourcePositionTableBuilder(Isolate* isolate, Zone* zone)
-      : isolate_(isolate), entries_(zone) {}
+      : isolate_(isolate),
+        bytes_(zone),
+#ifdef ENABLE_SLOW_DCHECKS
+        raw_entries_(zone),
+#endif
+        previous_() {
+  }
 
   void AddStatementPosition(size_t bytecode_offset, int source_position);
   void AddExpressionPosition(size_t bytecode_offset, int source_position);
-  void RevertPosition(size_t bytecode_offset);
-  Handle<FixedArray> ToFixedArray();
+  Handle<ByteArray> ToSourcePositionTable();
 
  private:
-  struct Entry {
-    int bytecode_offset;
-    uint32_t source_position_and_type;
-  };
-
-  bool CodeOffsetHasPosition(int bytecode_offset) {
-    // Return whether bytecode offset already has a position assigned.
-    return entries_.size() > 0 &&
-           entries_.back().bytecode_offset == bytecode_offset;
-  }
+  void AddEntry(const PositionTableEntry& entry);
 
   Isolate* isolate_;
-  ZoneVector<Entry> entries_;
+  ZoneVector<byte> bytes_;
+#ifdef ENABLE_SLOW_DCHECKS
+  ZoneVector<PositionTableEntry> raw_entries_;
+#endif
+  PositionTableEntry previous_;
 };
 
 class SourcePositionTableIterator {
  public:
-  explicit SourcePositionTableIterator(BytecodeArray* bytecode_array);
+  explicit SourcePositionTableIterator(ByteArray* byte_array);
 
   void Advance();
 
   int bytecode_offset() const {
     DCHECK(!done());
-    return bytecode_offset_;
+    return current_.bytecode_offset;
   }
   int source_position() const {
     DCHECK(!done());
-    return source_position_;
+    return current_.source_position;
   }
   bool is_statement() const {
     DCHECK(!done());
-    return is_statement_;
+    return current_.is_statement;
   }
-  bool done() const { return index_ > length_; }
+  bool done() const { return index_ == kDone; }
 
  private:
-  FixedArray* table_;
+  static const int kDone = -1;
+
+  ByteArray* table_;
   int index_;
-  int length_;
-  bool is_statement_;
-  int bytecode_offset_;
-  int source_position_;
+  PositionTableEntry current_;
   DisallowHeapAllocation no_gc;
 };
 

src/objects-inl.h

@@ -3898,10 +3898,9 @@ int BytecodeArray::parameter_count() const {
 
 ACCESSORS(BytecodeArray, constant_pool, FixedArray, kConstantPoolOffset)
 ACCESSORS(BytecodeArray, handler_table, FixedArray, kHandlerTableOffset)
-ACCESSORS(BytecodeArray, source_position_table, FixedArray,
+ACCESSORS(BytecodeArray, source_position_table, ByteArray,
           kSourcePositionTableOffset)
 
-
 Address BytecodeArray::GetFirstBytecodeAddress() {
   return reinterpret_cast<Address>(this) - kHeapObjectTag + kHeaderSize;
 }

src/objects.cc

@@ -15048,7 +15048,8 @@ void Code::Disassemble(const char* name, std::ostream& os) {  // NOLINT
 
 int BytecodeArray::SourcePosition(int offset) {
   int last_position = 0;
-  for (interpreter::SourcePositionTableIterator iterator(this);
+  for (interpreter::SourcePositionTableIterator iterator(
+           source_position_table());
        !iterator.done() && iterator.bytecode_offset() <= offset;
        iterator.Advance()) {
     last_position = iterator.source_position();
@@ -15062,7 +15063,7 @@ int BytecodeArray::SourceStatementPosition(int offset) {
   int position = SourcePosition(offset);
   // Now find the closest statement position before the position.
   int statement_position = 0;
-  interpreter::SourcePositionTableIterator iterator(this);
+  interpreter::SourcePositionTableIterator iterator(source_position_table());
   while (!iterator.done()) {
     if (iterator.is_statement()) {
       int p = iterator.source_position();
@@ -15083,7 +15084,8 @@ void BytecodeArray::Disassemble(std::ostream& os) {
   const uint8_t* first_bytecode_address = GetFirstBytecodeAddress();
   int bytecode_size = 0;
 
-  interpreter::SourcePositionTableIterator source_positions(this);
+  interpreter::SourcePositionTableIterator source_positions(
+      source_position_table());
 
   for (int i = 0; i < this->length(); i += bytecode_size) {
     const uint8_t* bytecode_start = &first_bytecode_address[i];

src/objects.h

@@ -4462,7 +4462,7 @@ class BytecodeArray : public FixedArrayBase {
 
   // Accessors for source position table containing mappings between byte code
   // offset and source position.
-  DECL_ACCESSORS(source_position_table, FixedArray)
+  DECL_ACCESSORS(source_position_table, ByteArray)
 
   DECLARE_CAST(BytecodeArray)
 

test/unittests/interpreter/source-position-table-unittest.cc

+// Copyright 2016 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/v8.h"
+
+#include "src/interpreter/source-position-table.h"
+#include "test/unittests/test-utils.h"
+
+namespace v8 {
+namespace internal {
+namespace interpreter {
+
+class SourcePositionTableTest : public TestWithIsolateAndZone {
+ public:
+  SourcePositionTableTest() {}
+  ~SourcePositionTableTest() override {}
+};
+
+// Some random offsets, mostly at 'suspicious' bit boundaries.
+static int offsets[] = {0,   1,   2,    3,    4,     30,      31,  32,
+                        33,  62,  63,   64,   65,    126,     127, 128,
+                        129, 250, 1000, 9999, 12000, 31415926};
+
+TEST_F(SourcePositionTableTest, EncodeStatement) {
+  SourcePositionTableBuilder builder(isolate(), zone());
+  for (int i = 0; i < arraysize(offsets); i++) {
+    builder.AddStatementPosition(offsets[i], offsets[i]);
+  }
+
+  // To test correctness, we rely on the assertions in ToSourcePositionTable().
+  // (Also below.)
+  CHECK(!builder.ToSourcePositionTable().is_null());
+}
+
+TEST_F(SourcePositionTableTest, EncodeExpression) {
+  SourcePositionTableBuilder builder(isolate(), zone());
+  for (int i = 0; i < arraysize(offsets); i++) {
+    builder.AddExpressionPosition(offsets[i], offsets[i]);
+  }
+  CHECK(!builder.ToSourcePositionTable().is_null());
+}
+
+TEST_F(SourcePositionTableTest, EncodeAscending) {
+  SourcePositionTableBuilder builder(isolate(), zone());
+
+  int accumulator = 0;
+  for (int i = 0; i < arraysize(offsets); i++) {
+    accumulator += offsets[i];
+    if (i % 2) {
+      builder.AddStatementPosition(accumulator, accumulator);
+    } else {
+      builder.AddExpressionPosition(accumulator, accumulator);
+    }
+  }
+
+  // Also test negative offsets:
+  for (int i = 0; i < arraysize(offsets); i++) {
+    accumulator -= offsets[i];
+    if (i % 2) {
+      builder.AddStatementPosition(accumulator, accumulator);
+    } else {
+      builder.AddExpressionPosition(accumulator, accumulator);
+    }
+  }
+
+  CHECK(!builder.ToSourcePositionTable().is_null());
+}
+
+}  // namespace interpreter
+}  // namespace internal
+}  // namespace v8

test/unittests/unittests.gyp

@@ -102,6 +102,7 @@
         'interpreter/interpreter-assembler-unittest.cc',
         'interpreter/interpreter-assembler-unittest.h',
         'interpreter/register-translator-unittest.cc',
+        'interpreter/source-position-table-unittest.cc',
         'libplatform/default-platform-unittest.cc',
         'libplatform/task-queue-unittest.cc',
         'libplatform/worker-thread-unittest.cc',