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

This reverts commit f4bd6f6b.

Reason for revert: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20-%20pointer%20compression/13275/overview

Original change's description:
> [interpreter] Speed up the BytecodeArrayAccessor through direct memory access
>
> This speeds up the sparkplug compiler by >20%.
>
> Change-Id: I9dece7209b0ddf53d876df9aca4b2feede6259c5
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2682644
> Auto-Submit: Toon Verwaest <verwaest@chromium.org>
> Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
> Reviewed-by: Georg Neis <neis@chromium.org>
> Reviewed-by: Leszek Swirski <leszeks@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72624}

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

Change-Id: Iac3832bd08a9075e3127886635ba96f7acfc49cf
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2685169Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Commit-Queue: Nico Hartmann <nicohartmann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72625}

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,8 +135,7 @@ bool LocalHeap::IsParked() {
 
 void LocalHeap::Park() {
   base::MutexGuard guard(&state_mutex_);
-  CHECK_EQ(ThreadState::Running, state_);
-  DCHECK(gc_epilogue_callbacks_.empty());
+  CHECK(state_ == ThreadState::Running);
   state_ = ThreadState::Parked;
   state_change_.NotifyAll();
 }
@@ -209,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),
@@ -219,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/unittests/heap/local-heap-unittest.cc

@@ -118,14 +118,19 @@ class GCEpilogue {
 
 class BackgroundThreadForGCEpilogue final : public v8::base::Thread {
  public:
-  explicit BackgroundThreadForGCEpilogue(Heap* heap, GCEpilogue* epilogue)
+  explicit BackgroundThreadForGCEpilogue(Heap* heap, bool parked,
+                                         GCEpilogue* epilogue)
       : v8::base::Thread(base::Thread::Options("BackgroundThread")),
         heap_(heap),
+        parked_(parked),
         epilogue_(epilogue) {}
 
   void Run() override {
     LocalHeap lh(heap_, ThreadKind::kBackground);
-    UnparkedScope unparked_scope(&lh);
+    base::Optional<UnparkedScope> unparked_scope;
+    if (!parked_) {
+      unparked_scope.emplace(&lh);
+    }
     epilogue_->NotifyStarted();
     lh.AddGCEpilogueCallback(&GCEpilogue::Callback, epilogue_);
     while (!epilogue_->StopRequested()) {
@@ -135,6 +140,7 @@ class BackgroundThreadForGCEpilogue final : public v8::base::Thread {
   }
 
   Heap* heap_;
+  bool parked_;
   GCEpilogue* epilogue_;
 };
 
@@ -143,13 +149,12 @@ class BackgroundThreadForGCEpilogue final : public v8::base::Thread {
 TEST_F(LocalHeapTest, GCEpilogue) {
   Heap* heap = i_isolate()->heap();
   LocalHeap lh(heap, ThreadKind::kMain);
-  UnparkedScope unparked_scope(&lh);
   std::array<GCEpilogue, 3> epilogue;
   lh.AddGCEpilogueCallback(&GCEpilogue::Callback, &epilogue[0]);
   auto thread1 =
-      std::make_unique<BackgroundThreadForGCEpilogue>(heap, &epilogue[1]);
-  auto thread2 =
-      std::make_unique<BackgroundThreadForGCEpilogue>(heap, &epilogue[2]);
+      std::make_unique<BackgroundThreadForGCEpilogue>(heap, true, &epilogue[1]);
+  auto thread2 = std::make_unique<BackgroundThreadForGCEpilogue>(heap, false,
+                                                                 &epilogue[2]);
   CHECK(thread1->Start());
   CHECK(thread2->Start());
   epilogue[1].WaitUntilStarted();