Reland "[interpreter] Speed up the BytecodeArrayAccessor through direct memory access"

This speeds up sparkplug by >20%.

This reland fixes the OffHeapBytecodeArray to also register a GC
callback. Turns out off-heap here doesn't mean that the underlying
bytecode array is off-heap and it can in fact move.

Change-Id: I7c6e82abd2a7be08ead537ab84855e76edc3b290
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2688400
Auto-Submit: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72677}

src/compiler/js-heap-broker.cc

@@ -5387,7 +5387,7 @@ bool JSHeapBroker::StackHasOverflowed() const {
 }
 
 OffHeapBytecodeArray::OffHeapBytecodeArray(BytecodeArrayRef bytecode_array)
-    : array_(bytecode_array) {}
+    : AbstractBytecodeArray(), array_(bytecode_array) {}
 
 int OffHeapBytecodeArray::length() const { return array_.length(); }
 
@@ -5395,10 +5395,6 @@ int OffHeapBytecodeArray::parameter_count() const {
   return array_.parameter_count();
 }
 
-uint8_t OffHeapBytecodeArray::get(int index) const { return array_.get(index); }
-
-void OffHeapBytecodeArray::set(int index, uint8_t value) { UNREACHABLE(); }
-
 Address OffHeapBytecodeArray::GetFirstBytecodeAddress() const {
   return array_.GetFirstBytecodeAddress();
 }

src/compiler/js-heap-broker.h

@@ -463,8 +463,6 @@ class OffHeapBytecodeArray final : public interpreter::AbstractBytecodeArray {
 
   int length() const override;
   int parameter_count() const override;
-  uint8_t get(int index) const override;
-  void set(int index, uint8_t value) override;
   Address GetFirstBytecodeAddress() const override;
   Handle<Object> GetConstantAtIndex(int index, Isolate* isolate) const override;
   bool IsConstantAtIndexSmi(int index) const override;

src/heap/local-heap.cc

@@ -135,7 +135,7 @@ bool LocalHeap::IsParked() {
 
 void LocalHeap::Park() {
   base::MutexGuard guard(&state_mutex_);
-  CHECK(state_ == ThreadState::Running);
+  CHECK_EQ(ThreadState::Running, state_);
   state_ = ThreadState::Parked;
   state_change_.NotifyAll();
 }
@@ -208,6 +208,7 @@ Address LocalHeap::PerformCollectionAndAllocateAgain(
 
 void LocalHeap::AddGCEpilogueCallback(GCEpilogueCallback* callback,
                                       void* data) {
+  DCHECK(!IsParked());
   std::pair<GCEpilogueCallback*, void*> callback_and_data(callback, data);
   DCHECK_EQ(std::find(gc_epilogue_callbacks_.begin(),
                       gc_epilogue_callbacks_.end(), callback_and_data),
@@ -217,6 +218,7 @@ void LocalHeap::AddGCEpilogueCallback(GCEpilogueCallback* callback,
 
 void LocalHeap::RemoveGCEpilogueCallback(GCEpilogueCallback* callback,
                                          void* data) {
+  DCHECK(!IsParked());
   std::pair<GCEpilogueCallback*, void*> callback_and_data(callback, data);
   auto it = std::find(gc_epilogue_callbacks_.begin(),
                       gc_epilogue_callbacks_.end(), callback_and_data);

src/interpreter/bytecode-array-accessor.cc

@@ -19,18 +19,12 @@ namespace {
 class OnHeapBytecodeArray final : public AbstractBytecodeArray {
  public:
   explicit OnHeapBytecodeArray(Handle<BytecodeArray> bytecode_array)
-      : array_(bytecode_array) {}
+      : AbstractBytecodeArray(), array_(bytecode_array) {}
 
   int length() const override { return array_->length(); }
 
   int parameter_count() const override { return array_->parameter_count(); }
 
-  uint8_t get(int index) const override { return array_->get(index); }
-
-  void set(int index, uint8_t value) override {
-    return array_->set(index, value);
-  }
-
   Address GetFirstBytecodeAddress() const override {
     return array_->GetFirstBytecodeAddress();
   }
@@ -54,13 +48,25 @@ class OnHeapBytecodeArray final : public AbstractBytecodeArray {
 
 }  // namespace
 
+// TODO(verwaest): Pass the LocalHeap into the constructor.
+AbstractBytecodeArray::AbstractBytecodeArray()
+    : local_heap_(LocalHeap::Current()) {
+  if (!local_heap_) {
+    local_heap_ = Isolate::Current()->main_thread_local_heap();
+  }
+}
+
 BytecodeArrayAccessor::BytecodeArrayAccessor(
     std::unique_ptr<AbstractBytecodeArray> bytecode_array, int initial_offset)
     : bytecode_array_(std::move(bytecode_array)),
-      bytecode_length_(bytecode_array_->length()),
-      bytecode_offset_(initial_offset),
+      start_(reinterpret_cast<uint8_t*>(
+          bytecode_array_->GetFirstBytecodeAddress())),
+      end_(start_ + bytecode_array_->length()),
+      cursor_(start_ + initial_offset),
       operand_scale_(OperandScale::kSingle),
-      prefix_offset_(0) {
+      prefix_size_(0) {
+  bytecode_array_->local_heap()->AddGCEpilogueCallback(UpdatePointersCallback,
+                                                       this);
   UpdateOperandScale();
 }
 
@@ -70,8 +76,15 @@ BytecodeArrayAccessor::BytecodeArrayAccessor(
           std::make_unique<OnHeapBytecodeArray>(bytecode_array),
           initial_offset) {}
 
+BytecodeArrayAccessor::~BytecodeArrayAccessor() {
+  bytecode_array_->local_heap()->RemoveGCEpilogueCallback(
+      UpdatePointersCallback, this);
+}
+
 void BytecodeArrayAccessor::SetOffset(int offset) {
-  bytecode_offset_ = offset;
+  if (offset < 0) return;
+  cursor_ = reinterpret_cast<uint8_t*>(
+      bytecode_array()->GetFirstBytecodeAddress() + offset);
   UpdateOperandScale();
 }
 
@@ -79,45 +92,16 @@ void BytecodeArrayAccessor::ApplyDebugBreak() {
   // Get the raw bytecode from the bytecode array. This may give us a
   // scaling prefix, which we can patch with the matching debug-break
   // variant.
-  interpreter::Bytecode bytecode =
-      interpreter::Bytecodes::FromByte(bytecode_array()->get(bytecode_offset_));
+  uint8_t* cursor = cursor_ - prefix_size_;
+  interpreter::Bytecode bytecode = interpreter::Bytecodes::FromByte(*cursor);
   if (interpreter::Bytecodes::IsDebugBreak(bytecode)) return;
   interpreter::Bytecode debugbreak =
       interpreter::Bytecodes::GetDebugBreak(bytecode);
-  bytecode_array()->set(bytecode_offset_,
-                        interpreter::Bytecodes::ToByte(debugbreak));
-}
-
-void BytecodeArrayAccessor::UpdateOperandScale() {
-  if (OffsetInBounds()) {
-    uint8_t current_byte = bytecode_array()->get(bytecode_offset_);
-    Bytecode current_bytecode = Bytecodes::FromByte(current_byte);
-    if (Bytecodes::IsPrefixScalingBytecode(current_bytecode)) {
-      operand_scale_ =
-          Bytecodes::PrefixBytecodeToOperandScale(current_bytecode);
-      prefix_offset_ = 1;
-    } else {
-      operand_scale_ = OperandScale::kSingle;
-      prefix_offset_ = 0;
-    }
-  }
-}
-
-bool BytecodeArrayAccessor::OffsetInBounds() const {
-  return bytecode_offset_ >= 0 && bytecode_offset_ < bytecode_length_;
-}
-
-Bytecode BytecodeArrayAccessor::current_bytecode() const {
-  DCHECK(OffsetInBounds());
-  uint8_t current_byte =
-      bytecode_array()->get(bytecode_offset_ + current_prefix_offset());
-  Bytecode current_bytecode = Bytecodes::FromByte(current_byte);
-  DCHECK(!Bytecodes::IsPrefixScalingBytecode(current_bytecode));
-  return current_bytecode;
+  *cursor = interpreter::Bytecodes::ToByte(debugbreak);
 }
 
 int BytecodeArrayAccessor::current_bytecode_size() const {
-  return current_prefix_offset() +
+  return prefix_size_ +
          Bytecodes::Size(current_bytecode(), current_operand_scale());
 }
 
@@ -129,8 +113,7 @@ uint32_t BytecodeArrayAccessor::GetUnsignedOperand(
             Bytecodes::GetOperandType(current_bytecode(), operand_index));
   DCHECK(Bytecodes::IsUnsignedOperandType(operand_type));
   Address operand_start =
-      bytecode_array()->GetFirstBytecodeAddress() + bytecode_offset_ +
-      current_prefix_offset() +
+      reinterpret_cast<Address>(cursor_) +
       Bytecodes::GetOperandOffset(current_bytecode(), operand_index,
                                   current_operand_scale());
   return BytecodeDecoder::DecodeUnsignedOperand(operand_start, operand_type,
@@ -145,8 +128,7 @@ int32_t BytecodeArrayAccessor::GetSignedOperand(
             Bytecodes::GetOperandType(current_bytecode(), operand_index));
   DCHECK(!Bytecodes::IsUnsignedOperandType(operand_type));
   Address operand_start =
-      bytecode_array()->GetFirstBytecodeAddress() + bytecode_offset_ +
-      current_prefix_offset() +
+      reinterpret_cast<Address>(cursor_) +
       Bytecodes::GetOperandOffset(current_bytecode(), operand_index,
                                   current_operand_scale());
   return BytecodeDecoder::DecodeSignedOperand(operand_start, operand_type,
@@ -207,8 +189,7 @@ Register BytecodeArrayAccessor::GetRegisterOperand(int operand_index) const {
   OperandType operand_type =
       Bytecodes::GetOperandType(current_bytecode(), operand_index);
   Address operand_start =
-      bytecode_array()->GetFirstBytecodeAddress() + bytecode_offset_ +
-      current_prefix_offset() +
+      reinterpret_cast<Address>(cursor_) +
       Bytecodes::GetOperandOffset(current_bytecode(), operand_index,
                                   current_operand_scale());
   return BytecodeDecoder::DecodeRegisterOperand(operand_start, operand_type,
@@ -312,18 +293,11 @@ JumpTableTargetOffsets BytecodeArrayAccessor::GetJumpTableTargetOffsets()
 }
 
 int BytecodeArrayAccessor::GetAbsoluteOffset(int relative_offset) const {
-  return current_offset() + relative_offset + current_prefix_offset();
-}
-
-bool BytecodeArrayAccessor::OffsetWithinBytecode(int offset) const {
-  return current_offset() <= offset &&
-         offset < current_offset() + current_bytecode_size();
+  return current_offset() + relative_offset + prefix_size_;
 }
 
 std::ostream& BytecodeArrayAccessor::PrintTo(std::ostream& os) const {
-  const uint8_t* bytecode_addr = reinterpret_cast<const uint8_t*>(
-      bytecode_array()->GetFirstBytecodeAddress() + bytecode_offset_);
-  return BytecodeDecoder::Decode(os, bytecode_addr,
+  return BytecodeDecoder::Decode(os, cursor_ - prefix_size_,
                                  bytecode_array()->parameter_count());
 }
 

src/interpreter/bytecode-array-accessor.h

@@ -69,10 +69,9 @@ class V8_EXPORT_PRIVATE JumpTableTargetOffsets final {
 
 class V8_EXPORT_PRIVATE AbstractBytecodeArray {
  public:
+  AbstractBytecodeArray();
   virtual int length() const = 0;
   virtual int parameter_count() const = 0;
-  virtual uint8_t get(int index) const = 0;
-  virtual void set(int index, uint8_t value) = 0;
   virtual Address GetFirstBytecodeAddress() const = 0;
 
   virtual Handle<Object> GetConstantAtIndex(int index,
@@ -81,6 +80,10 @@ class V8_EXPORT_PRIVATE AbstractBytecodeArray {
   virtual Smi GetConstantAtIndexAsSmi(int index) const = 0;
 
   virtual ~AbstractBytecodeArray() = default;
+  LocalHeap* local_heap() const { return local_heap_; }
+
+ private:
+  LocalHeap* local_heap_;
 };
 
 class V8_EXPORT_PRIVATE BytecodeArrayAccessor {
@@ -90,19 +93,31 @@ class V8_EXPORT_PRIVATE BytecodeArrayAccessor {
 
   BytecodeArrayAccessor(Handle<BytecodeArray> bytecode_array,
                         int initial_offset);
+  ~BytecodeArrayAccessor();
 
   BytecodeArrayAccessor(const BytecodeArrayAccessor&) = delete;
   BytecodeArrayAccessor& operator=(const BytecodeArrayAccessor&) = delete;
 
+  inline void Advance() {
+    cursor_ += Bytecodes::Size(current_bytecode(), current_operand_scale());
+    UpdateOperandScale();
+  }
   void SetOffset(int offset);
 
   void ApplyDebugBreak();
 
-  Bytecode current_bytecode() const;
+  inline Bytecode current_bytecode() const {
+    DCHECK(!done());
+    uint8_t current_byte = *cursor_;
+    Bytecode current_bytecode = Bytecodes::FromByte(current_byte);
+    DCHECK(!Bytecodes::IsPrefixScalingBytecode(current_bytecode));
+    return current_bytecode;
+  }
   int current_bytecode_size() const;
-  int current_offset() const { return bytecode_offset_; }
+  int current_offset() const {
+    return static_cast<int>(cursor_ - start_ - prefix_size_);
+  }
   OperandScale current_operand_scale() const { return operand_scale_; }
-  int current_prefix_offset() const { return prefix_offset_; }
   AbstractBytecodeArray* bytecode_array() const {
     return bytecode_array_.get();
   }
@@ -143,26 +158,55 @@ class V8_EXPORT_PRIVATE BytecodeArrayAccessor {
   // from the current bytecode.
   int GetAbsoluteOffset(int relative_offset) const;
 
-  bool OffsetWithinBytecode(int offset) const;
-
   std::ostream& PrintTo(std::ostream& os) const;
 
-  int bytecode_length() const { return bytecode_length_; }
+  static void UpdatePointersCallback(void* accessor) {
+    reinterpret_cast<BytecodeArrayAccessor*>(accessor)->UpdatePointers();
+  }
 
- private:
-  bool OffsetInBounds() const;
+  void UpdatePointers() {
+    DisallowGarbageCollection no_gc;
+    uint8_t* start =
+        reinterpret_cast<uint8_t*>(bytecode_array_->GetFirstBytecodeAddress());
+    if (start != start_) {
+      start_ = start;
+      uint8_t* end = start + bytecode_array_->length();
+      size_t distance_to_end = end_ - cursor_;
+      cursor_ = end - distance_to_end;
+      end_ = end;
+    }
+  }
 
+  inline bool done() const { return cursor_ >= end_; }
+
+ private:
   uint32_t GetUnsignedOperand(int operand_index,
                               OperandType operand_type) const;
   int32_t GetSignedOperand(int operand_index, OperandType operand_type) const;
 
-  void UpdateOperandScale();
+  inline void UpdateOperandScale() {
+    if (done()) return;
+    uint8_t current_byte = *cursor_;
+    Bytecode current_bytecode = Bytecodes::FromByte(current_byte);
+    if (Bytecodes::IsPrefixScalingBytecode(current_bytecode)) {
+      operand_scale_ =
+          Bytecodes::PrefixBytecodeToOperandScale(current_bytecode);
+      ++cursor_;
+      prefix_size_ = 1;
+    } else {
+      operand_scale_ = OperandScale::kSingle;
+      prefix_size_ = 0;
+    }
+  }
 
   std::unique_ptr<AbstractBytecodeArray> bytecode_array_;
-  const int bytecode_length_;
-  int bytecode_offset_;
+  uint8_t* start_;
+  uint8_t* end_;
+  // The cursor always points to the active bytecode. If there's a prefix, the
+  // prefix is at (cursor - 1).
+  uint8_t* cursor_;
   OperandScale operand_scale_;
-  int prefix_offset_;
+  int prefix_size_;
 };
 
 }  // namespace interpreter

src/interpreter/bytecode-array-iterator.cc

@@ -18,14 +18,6 @@ BytecodeArrayIterator::BytecodeArrayIterator(
     Handle<BytecodeArray> bytecode_array)
     : BytecodeArrayAccessor(bytecode_array, 0) {}
 
-void BytecodeArrayIterator::Advance() {
-  SetOffset(current_offset() + current_bytecode_size());
-}
-
-bool BytecodeArrayIterator::done() const {
-  return current_offset() >= bytecode_length();
-}
-
 }  // namespace interpreter
 }  // namespace internal
 }  // namespace v8

src/interpreter/bytecode-array-iterator.h

@@ -22,9 +22,6 @@ class V8_EXPORT_PRIVATE BytecodeArrayIterator final
 
   BytecodeArrayIterator(const BytecodeArrayIterator&) = delete;
   BytecodeArrayIterator& operator=(const BytecodeArrayIterator&) = delete;
-
-  void Advance();
-  bool done() const;
 };
 
 }  // namespace interpreter

src/interpreter/bytecode-array-random-iterator.cc

@@ -22,7 +22,7 @@ void BytecodeArrayRandomIterator::Initialize() {
   // bytecode.
   while (current_offset() < bytecode_array()->length()) {
     offsets_.push_back(current_offset());
-    SetOffset(current_offset() + current_bytecode_size());
+    Advance();
   }
   GoToStart();
 }

test/cctest/interpreter/test-interpreter.cc

@@ -1722,18 +1722,20 @@ TEST(InterpreterJumpConstantWith16BitOperand) {
 
   ast_factory.Internalize(isolate);
   Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray(isolate);
-  BytecodeArrayIterator iterator(bytecode_array);
-
-  bool found_16bit_constant_jump = false;
-  while (!iterator.done()) {
-    if (iterator.current_bytecode() == Bytecode::kJumpConstant &&
-        iterator.current_operand_scale() == OperandScale::kDouble) {
-      found_16bit_constant_jump = true;
-      break;
+  {
+    BytecodeArrayIterator iterator(bytecode_array);
+
+    bool found_16bit_constant_jump = false;
+    while (!iterator.done()) {
+      if (iterator.current_bytecode() == Bytecode::kJumpConstant &&
+          iterator.current_operand_scale() == OperandScale::kDouble) {
+        found_16bit_constant_jump = true;
+        break;
+      }
+      iterator.Advance();
     }
-    iterator.Advance();
+    CHECK(found_16bit_constant_jump);
   }
-  CHECK(found_16bit_constant_jump);
 
   InterpreterTester tester(isolate, bytecode_array, metadata);
   auto callable = tester.GetCallable<>();
@@ -1766,19 +1768,20 @@ TEST(InterpreterJumpWith32BitOperand) {
 
   ast_factory.Internalize(isolate);
   Handle<BytecodeArray> bytecode_array = builder.ToBytecodeArray(isolate);
-
-  BytecodeArrayIterator iterator(bytecode_array);
-
-  bool found_32bit_jump = false;
-  while (!iterator.done()) {
-    if (iterator.current_bytecode() == Bytecode::kJump &&
-        iterator.current_operand_scale() == OperandScale::kQuadruple) {
-      found_32bit_jump = true;
-      break;
+  {
+    BytecodeArrayIterator iterator(bytecode_array);
+
+    bool found_32bit_jump = false;
+    while (!iterator.done()) {
+      if (iterator.current_bytecode() == Bytecode::kJump &&
+          iterator.current_operand_scale() == OperandScale::kQuadruple) {
+        found_32bit_jump = true;
+        break;
+      }
+      iterator.Advance();
     }
-    iterator.Advance();
+    CHECK(found_32bit_jump);
   }
-  CHECK(found_32bit_jump);
 
   InterpreterTester tester(isolate, bytecode_array);
   auto callable = tester.GetCallable<>();

test/unittests/heap/local-heap-unittest.cc

@@ -132,11 +132,19 @@ class BackgroundThreadForGCEpilogue final : public v8::base::Thread {
       unparked_scope.emplace(&lh);
     }
     epilogue_->NotifyStarted();
-    lh.AddGCEpilogueCallback(&GCEpilogue::Callback, epilogue_);
+    {
+      base::Optional<UnparkedScope> unparked_scope;
+      if (parked_) unparked_scope.emplace(&lh);
+      lh.AddGCEpilogueCallback(&GCEpilogue::Callback, epilogue_);
+    }
     while (!epilogue_->StopRequested()) {
       lh.Safepoint();
     }
-    lh.RemoveGCEpilogueCallback(&GCEpilogue::Callback, epilogue_);
+    {
+      base::Optional<UnparkedScope> unparked_scope;
+      if (parked_) unparked_scope.emplace(&lh);
+      lh.RemoveGCEpilogueCallback(&GCEpilogue::Callback, epilogue_);
+    }
   }
 
   Heap* heap_;
@@ -150,7 +158,10 @@ TEST_F(LocalHeapTest, GCEpilogue) {
   Heap* heap = i_isolate()->heap();
   LocalHeap lh(heap, ThreadKind::kMain);
   std::array<GCEpilogue, 3> epilogue;
-  lh.AddGCEpilogueCallback(&GCEpilogue::Callback, &epilogue[0]);
+  {
+    UnparkedScope unparked(&lh);
+    lh.AddGCEpilogueCallback(&GCEpilogue::Callback, &epilogue[0]);
+  }
   auto thread1 =
       std::make_unique<BackgroundThreadForGCEpilogue>(heap, true, &epilogue[1]);
   auto thread2 = std::make_unique<BackgroundThreadForGCEpilogue>(heap, false,
@@ -165,7 +176,10 @@ TEST_F(LocalHeapTest, GCEpilogue) {
   epilogue[2].RequestStop();
   thread1->Join();
   thread2->Join();
-  lh.RemoveGCEpilogueCallback(&GCEpilogue::Callback, &epilogue[0]);
+  {
+    UnparkedScope unparked(&lh);
+    lh.RemoveGCEpilogueCallback(&GCEpilogue::Callback, &epilogue[0]);
+  }
   for (auto& e : epilogue) {
     CHECK(e.WasInvoked());
   }