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

This reverts commit 60748ee2.

Reason for revert: Broke Linux64 ASAN https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20ASAN/38792/overview.

There are 4 changes in that range causing the failure, I found that this change caused the failure by running locally `./tools/run-tests.py --outdir=out/repro mjsunit/wasm/gc-stress --variant turboprop_as_toptier --random-seed-stress-count 100`.

Original change's description:
> Reland "[interpreter] Speed up the BytecodeArrayAccessor through direct memory access"
>
> Tbr: ulan@chromium.org, neis@chromium.org, leszeks@chromium.org
> No-Presubmit: true
> Change-Id: I4ceb9e21ac7d78a87776b4be174772539d2da8d9
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2685173
> Commit-Queue: Toon Verwaest <verwaest@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Reviewed-by: Georg Neis <neis@chromium.org>
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Reviewed-by: Leszek Swirski <leszeks@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72632}

TBR=ulan@chromium.org,neis@chromium.org,leszeks@chromium.org,verwaest@chromium.org

Change-Id: I441ddfda5d852b7a01f38a9e60edc56f40ae626a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2686266Reviewed-by: Zhi An Ng <zhin@chromium.org>
Commit-Queue: Zhi An Ng <zhin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72635}

src/compiler/js-heap-broker.cc

@@ -5377,7 +5377,7 @@ bool JSHeapBroker::StackHasOverflowed() const {
 }
 
 OffHeapBytecodeArray::OffHeapBytecodeArray(BytecodeArrayRef bytecode_array)
-    : AbstractBytecodeArray(false), array_(bytecode_array) {}
+    : array_(bytecode_array) {}
 
 int OffHeapBytecodeArray::length() const { return array_.length(); }
 
@@ -5385,6 +5385,10 @@ 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,11 +463,12 @@ 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;
   Smi GetConstantAtIndexAsSmi(int index) const override;
-  LocalHeap* local_heap() const override { UNREACHABLE(); }
 
  private:
   BytecodeArrayRef array_;

src/heap/local-heap.cc

@@ -135,7 +135,7 @@ bool LocalHeap::IsParked() {
 
 void LocalHeap::Park() {
   base::MutexGuard guard(&state_mutex_);
-  CHECK_EQ(ThreadState::Running, state_);
+  CHECK(state_ == ThreadState::Running);
   state_ = ThreadState::Parked;
   state_change_.NotifyAll();
 }
@@ -208,7 +208,6 @@ 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),
@@ -218,7 +217,6 @@ 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,18 @@ namespace {
 class OnHeapBytecodeArray final : public AbstractBytecodeArray {
  public:
   explicit OnHeapBytecodeArray(Handle<BytecodeArray> bytecode_array)
-      : AbstractBytecodeArray(true), array_(bytecode_array) {
-    // TODO(verwaest): Pass the LocalHeap into the constructor.
-    local_heap_ = LocalHeap::Current();
-    if (!local_heap_) {
-      local_heap_ = Isolate::Current()->main_thread_local_heap();
-    }
-  }
+      : 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();
   }
@@ -48,10 +48,7 @@ class OnHeapBytecodeArray final : public AbstractBytecodeArray {
     return Smi::cast(array_->constant_pool().get(index));
   }
 
-  LocalHeap* local_heap() const override { return local_heap_; }
-
  private:
-  LocalHeap* local_heap_;
   Handle<BytecodeArray> array_;
 };
 
@@ -60,16 +57,10 @@ class OnHeapBytecodeArray final : public AbstractBytecodeArray {
 BytecodeArrayAccessor::BytecodeArrayAccessor(
     std::unique_ptr<AbstractBytecodeArray> bytecode_array, int initial_offset)
     : bytecode_array_(std::move(bytecode_array)),
-      start_(reinterpret_cast<uint8_t*>(
-          bytecode_array_->GetFirstBytecodeAddress())),
-      end_(start_ + bytecode_array_->length()),
-      cursor_(start_ + initial_offset),
+      bytecode_length_(bytecode_array_->length()),
+      bytecode_offset_(initial_offset),
       operand_scale_(OperandScale::kSingle),
-      prefix_size_(0) {
-  if (bytecode_array_->can_move()) {
-    bytecode_array_->local_heap()->AddGCEpilogueCallback(UpdatePointersCallback,
-                                                         this);
-  }
+      prefix_offset_(0) {
   UpdateOperandScale();
 }
 
@@ -79,17 +70,8 @@ BytecodeArrayAccessor::BytecodeArrayAccessor(
           std::make_unique<OnHeapBytecodeArray>(bytecode_array),
           initial_offset) {}
 
-BytecodeArrayAccessor::~BytecodeArrayAccessor() {
-  if (bytecode_array_->can_move()) {
-    bytecode_array_->local_heap()->RemoveGCEpilogueCallback(
-        UpdatePointersCallback, this);
-  }
-}
-
 void BytecodeArrayAccessor::SetOffset(int offset) {
-  if (offset < 0) return;
-  cursor_ = reinterpret_cast<uint8_t*>(
-      bytecode_array()->GetFirstBytecodeAddress() + offset);
+  bytecode_offset_ = offset;
   UpdateOperandScale();
 }
 
@@ -97,16 +79,45 @@ 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.
-  uint8_t* cursor = cursor_ - prefix_size_;
-  interpreter::Bytecode bytecode = interpreter::Bytecodes::FromByte(*cursor);
+  interpreter::Bytecode bytecode =
+      interpreter::Bytecodes::FromByte(bytecode_array()->get(bytecode_offset_));
   if (interpreter::Bytecodes::IsDebugBreak(bytecode)) return;
   interpreter::Bytecode debugbreak =
       interpreter::Bytecodes::GetDebugBreak(bytecode);
-  *cursor = interpreter::Bytecodes::ToByte(debugbreak);
+  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;
 }
 
 int BytecodeArrayAccessor::current_bytecode_size() const {
-  return prefix_size_ +
+  return current_prefix_offset() +
          Bytecodes::Size(current_bytecode(), current_operand_scale());
 }
 
@@ -118,7 +129,8 @@ uint32_t BytecodeArrayAccessor::GetUnsignedOperand(
             Bytecodes::GetOperandType(current_bytecode(), operand_index));
   DCHECK(Bytecodes::IsUnsignedOperandType(operand_type));
   Address operand_start =
-      reinterpret_cast<Address>(cursor_) +
+      bytecode_array()->GetFirstBytecodeAddress() + bytecode_offset_ +
+      current_prefix_offset() +
       Bytecodes::GetOperandOffset(current_bytecode(), operand_index,
                                   current_operand_scale());
   return BytecodeDecoder::DecodeUnsignedOperand(operand_start, operand_type,
@@ -133,7 +145,8 @@ int32_t BytecodeArrayAccessor::GetSignedOperand(
             Bytecodes::GetOperandType(current_bytecode(), operand_index));
   DCHECK(!Bytecodes::IsUnsignedOperandType(operand_type));
   Address operand_start =
-      reinterpret_cast<Address>(cursor_) +
+      bytecode_array()->GetFirstBytecodeAddress() + bytecode_offset_ +
+      current_prefix_offset() +
       Bytecodes::GetOperandOffset(current_bytecode(), operand_index,
                                   current_operand_scale());
   return BytecodeDecoder::DecodeSignedOperand(operand_start, operand_type,
@@ -194,7 +207,8 @@ Register BytecodeArrayAccessor::GetRegisterOperand(int operand_index) const {
   OperandType operand_type =
       Bytecodes::GetOperandType(current_bytecode(), operand_index);
   Address operand_start =
-      reinterpret_cast<Address>(cursor_) +
+      bytecode_array()->GetFirstBytecodeAddress() + bytecode_offset_ +
+      current_prefix_offset() +
       Bytecodes::GetOperandOffset(current_bytecode(), operand_index,
                                   current_operand_scale());
   return BytecodeDecoder::DecodeRegisterOperand(operand_start, operand_type,
@@ -298,11 +312,18 @@ JumpTableTargetOffsets BytecodeArrayAccessor::GetJumpTableTargetOffsets()
 }
 
 int BytecodeArrayAccessor::GetAbsoluteOffset(int relative_offset) const {
-  return current_offset() + relative_offset + prefix_size_;
+  return current_offset() + relative_offset + current_prefix_offset();
+}
+
+bool BytecodeArrayAccessor::OffsetWithinBytecode(int offset) const {
+  return current_offset() <= offset &&
+         offset < current_offset() + current_bytecode_size();
 }
 
 std::ostream& BytecodeArrayAccessor::PrintTo(std::ostream& os) const {
-  return BytecodeDecoder::Decode(os, cursor_ - prefix_size_,
+  const uint8_t* bytecode_addr = reinterpret_cast<const uint8_t*>(
+      bytecode_array()->GetFirstBytecodeAddress() + bytecode_offset_);
+  return BytecodeDecoder::Decode(os, bytecode_addr,
                                  bytecode_array()->parameter_count());
 }
 

src/interpreter/bytecode-array-accessor.h

@@ -69,9 +69,10 @@ class V8_EXPORT_PRIVATE JumpTableTargetOffsets final {
 
 class V8_EXPORT_PRIVATE AbstractBytecodeArray {
  public:
-  explicit AbstractBytecodeArray(bool can_move) : can_move_(can_move) {}
   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,
@@ -80,11 +81,6 @@ class V8_EXPORT_PRIVATE AbstractBytecodeArray {
   virtual Smi GetConstantAtIndexAsSmi(int index) const = 0;
 
   virtual ~AbstractBytecodeArray() = default;
-  virtual LocalHeap* local_heap() const = 0;
-  bool can_move() const { return can_move_; }
-
- private:
-  bool can_move_;
 };
 
 class V8_EXPORT_PRIVATE BytecodeArrayAccessor {
@@ -94,31 +90,19 @@ 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();
 
-  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;
-  }
+  Bytecode current_bytecode() const;
   int current_bytecode_size() const;
-  int current_offset() const {
-    return static_cast<int>(cursor_ - start_ - prefix_size_);
-  }
+  int current_offset() const { return bytecode_offset_; }
   OperandScale current_operand_scale() const { return operand_scale_; }
+  int current_prefix_offset() const { return prefix_offset_; }
   AbstractBytecodeArray* bytecode_array() const {
     return bytecode_array_.get();
   }
@@ -159,55 +143,26 @@ class V8_EXPORT_PRIVATE BytecodeArrayAccessor {
   // from the current bytecode.
   int GetAbsoluteOffset(int relative_offset) const;
 
-  std::ostream& PrintTo(std::ostream& os) const;
-
-  static void UpdatePointersCallback(void* accessor) {
-    reinterpret_cast<BytecodeArrayAccessor*>(accessor)->UpdatePointers();
-  }
+  bool OffsetWithinBytecode(int offset) 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;
-    }
-  }
+  std::ostream& PrintTo(std::ostream& os) const;
 
-  inline bool done() const { return cursor_ >= end_; }
+  int bytecode_length() const { return bytecode_length_; }
 
  private:
+  bool OffsetInBounds() const;
+
   uint32_t GetUnsignedOperand(int operand_index,
                               OperandType operand_type) const;
   int32_t GetSignedOperand(int operand_index, OperandType operand_type) const;
 
-  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;
-    }
-  }
+  void UpdateOperandScale();
 
   std::unique_ptr<AbstractBytecodeArray> bytecode_array_;
-  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_;
+  const int bytecode_length_;
+  int bytecode_offset_;
   OperandScale operand_scale_;
-  int prefix_size_;
+  int prefix_offset_;
 };
 
 }  // namespace interpreter

src/interpreter/bytecode-array-iterator.cc

@@ -18,6 +18,14 @@ 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,6 +22,9 @@ 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());
-    Advance();
+    SetOffset(current_offset() + current_bytecode_size());
   }
   GoToStart();
 }

test/cctest/interpreter/test-interpreter.cc

@@ -1722,20 +1722,18 @@ 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;
-      }
-      iterator.Advance();
+  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;
     }
-    CHECK(found_16bit_constant_jump);
+    iterator.Advance();
   }
+  CHECK(found_16bit_constant_jump);
 
   InterpreterTester tester(isolate, bytecode_array, metadata);
   auto callable = tester.GetCallable<>();
@@ -1768,20 +1766,19 @@ 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;
-      }
-      iterator.Advance();
+
+  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;
     }
-    CHECK(found_32bit_jump);
+    iterator.Advance();
   }
+  CHECK(found_32bit_jump);
 
   InterpreterTester tester(isolate, bytecode_array);
   auto callable = tester.GetCallable<>();

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

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