[vm/compiler] Do not mutate locs()->stack_bitmap() in RecordSafepoint

Instead create a copy of BitmapBuilder and mutate that. This
simplifies the logic, avoids unnecessary side-effects  and
allows us to delete some code which existed solely to validate
that two independent slow-path calls don't mutate the same
stack_bitmap in incompatible ways.

To make it cheap to copy BitmapBuilder we inline BitmapBuilder
backing storage into the object itself. Inlined capacity has
been chosen by looking at bitmap size distributions from large
Flutter applications.

TEST=ci

Cq-Include-Trybots: luci.dart.try:vm-kernel-precomp-linux-debug-x64-try,vm-kernel-precomp-linux-release-x64-try
Change-Id: I62ecb4ead388a367ad6904471d5923f5d1f5e3f4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/205783
Commit-Queue: Slava Egorov <vegorov@google.com>
Reviewed-by: Tess Strickland <sstrickl@google.com>

runtime/vm/bitmap.cc

@@ -20,11 +20,12 @@ void BitmapBuilder::SetLength(intptr_t new_length) {
       // First bit index (in the byte) to be cleared.
       intptr_t bit_index = new_length & (kBitsPerByte - 1);
       intptr_t mask = (1 << bit_index) - 1;
-      data_[byte_offset] &= mask;
+      BackingStore()[byte_offset] &= mask;
       // Clear the rest.
       ++byte_offset;
       if (byte_offset < data_size_in_bytes_) {
-        memset(&data_[byte_offset], 0, data_size_in_bytes_ - byte_offset);
+        memset(&BackingStore()[byte_offset], 0,
+               data_size_in_bytes_ - byte_offset);
       }
     }
   }
@@ -50,15 +51,17 @@ void BitmapBuilder::Set(intptr_t bit_offset, bool value) {
   if (value) {
     if (!InBackingStore(bit_offset)) {
       intptr_t byte_offset = bit_offset >> kBitsPerByteLog2;
-      uint8_t* old_data = data_;
+      uint8_t* old_data = BackingStore();
       intptr_t old_size = data_size_in_bytes_;
       data_size_in_bytes_ =
           Utils::RoundUp(byte_offset + 1, kIncrementSizeInBytes);
       ASSERT(data_size_in_bytes_ > 0);
-      data_ = Thread::Current()->zone()->Alloc<uint8_t>(data_size_in_bytes_);
-      ASSERT(data_ != NULL);
-      memmove(data_, old_data, old_size);
-      memset(&data_[old_size], 0, (data_size_in_bytes_ - old_size));
+      // Note: do not update data_ yet because it might overwrite old_data
+      // contents.
+      uint8_t* new_data = AllocBackingStore(data_size_in_bytes_);
+      memmove(new_data, old_data, old_size);
+      memset(&new_data[old_size], 0, (data_size_in_bytes_ - old_size));
+      data_.ptr_ = new_data;
     }
     ASSERT(InBackingStore(bit_offset));
   }
@@ -107,12 +110,11 @@ void BitmapBuilder::AppendAsBytesTo(BaseWriteStream* stream) const {
   // appending trailing zeroes are cleared to ensure deterministic snapshots.
   if (extra_size == 0 && Length() % kBitsPerByte != 0) {
     const int8_t mask = (1 << (Length() % kBitsPerByte)) - 1;
-    ASSERT_EQUAL(data_[payload_size - 1], (data_[payload_size - 1] & mask));
+    ASSERT_EQUAL(BackingStore()[payload_size - 1],
+                 (BackingStore()[payload_size - 1] & mask));
   }
 #endif
-  for (intptr_t i = 0; i < payload_size; i++) {
-    stream->WriteByte(data_[i]);
-  }
+  stream->WriteBytes(BackingStore(), payload_size);
   for (intptr_t i = 0; i < extra_size; i++) {
     stream->WriteByte(0U);
   }
@@ -126,8 +128,7 @@ bool BitmapBuilder::GetBit(intptr_t bit_offset) const {
   ASSERT(byte_offset < data_size_in_bytes_);
   intptr_t bit_remainder = bit_offset & (kBitsPerByte - 1);
   uint8_t mask = 1U << bit_remainder;
-  ASSERT(data_ != NULL);
-  return ((data_[byte_offset] & mask) != 0);
+  return ((BackingStore()[byte_offset] & mask) != 0);
 }
 
 void BitmapBuilder::SetBit(intptr_t bit_offset, bool value) {
@@ -141,11 +142,10 @@ void BitmapBuilder::SetBit(intptr_t bit_offset, bool value) {
   ASSERT(byte_offset < data_size_in_bytes_);
   intptr_t bit_remainder = bit_offset & (kBitsPerByte - 1);
   uint8_t mask = 1U << bit_remainder;
-  ASSERT(data_ != NULL);
   if (value) {
-    data_[byte_offset] |= mask;
+    BackingStore()[byte_offset] |= mask;
   } else {
-    data_[byte_offset] &= ~mask;
+    BackingStore()[byte_offset] &= ~mask;
   }
 }
 

runtime/vm/bitmap.h

@@ -17,12 +17,18 @@ namespace dart {
 // pointer map description of a stack).
 class BitmapBuilder : public ZoneAllocated {
  public:
-  BitmapBuilder()
-      : length_(0),
-        data_size_in_bytes_(kInitialSizeInBytes),
-        data_(ThreadState::Current()->zone()->Alloc<uint8_t>(
-            kInitialSizeInBytes)) {
-    memset(data_, 0, kInitialSizeInBytes);
+  BitmapBuilder() : length_(0), data_size_in_bytes_(kInlineCapacityInBytes) {
+    memset(data_.inline_, 0, data_size_in_bytes_);
+  }
+
+  BitmapBuilder(const BitmapBuilder& other)
+      : length_(other.length_), data_size_in_bytes_(other.data_size_in_bytes_) {
+    if (data_size_in_bytes_ == kInlineCapacityInBytes) {
+      memmove(data_.inline_, other.data_.inline_, kInlineCapacityInBytes);
+    } else {
+      data_.ptr_ = AllocBackingStore(data_size_in_bytes_);
+      memmove(data_.ptr_, other.data_.ptr_, data_size_in_bytes_);
+    }
   }
 
   intptr_t Length() const { return length_; }
@@ -47,8 +53,8 @@ class BitmapBuilder : public ZoneAllocated {
   void AppendAsBytesTo(BaseWriteStream* stream) const;
 
  private:
-  static const intptr_t kInitialSizeInBytes = 16;
-  static const intptr_t kIncrementSizeInBytes = 16;
+  static constexpr intptr_t kIncrementSizeInBytes = 16;
+  static constexpr intptr_t kInlineCapacityInBytes = 16;
 
   bool InRange(intptr_t offset) const {
     if (offset < 0) {
@@ -65,6 +71,20 @@ class BitmapBuilder : public ZoneAllocated {
     return byte_offset < data_size_in_bytes_;
   }
 
+  uint8_t* BackingStore() {
+    return data_size_in_bytes_ == kInlineCapacityInBytes ? &data_.inline_[0]
+                                                         : data_.ptr_;
+  }
+
+  const uint8_t* BackingStore() const {
+    return data_size_in_bytes_ == kInlineCapacityInBytes ? &data_.inline_[0]
+                                                         : data_.ptr_;
+  }
+
+  static uint8_t* AllocBackingStore(intptr_t size_in_bytes) {
+    return ThreadState::Current()->zone()->Alloc<uint8_t>(size_in_bytes);
+  }
+
   // Get/Set a bit that is known to be covered by the backing store.
   bool GetBit(intptr_t bit_offset) const;
   void SetBit(intptr_t bit_offset, bool value);
@@ -74,9 +94,10 @@ class BitmapBuilder : public ZoneAllocated {
   // Backing store for the bitmap.  Reading bits beyond the backing store
   // (up to length_) is allowed and they are assumed to be false.
   intptr_t data_size_in_bytes_;
-  uint8_t* data_;
-
-  DISALLOW_COPY_AND_ASSIGN(BitmapBuilder);
+  union {
+    uint8_t* ptr_;
+    uint8_t inline_[kInlineCapacityInBytes];
+  } data_;
 };
 
 }  // namespace dart

runtime/vm/compiler/backend/flow_graph_compiler.cc

@@ -949,36 +949,16 @@ void FlowGraphCompiler::RecordSafepoint(LocationSummary* locs,
     ASSERT(registers != NULL);
     const intptr_t kFpuRegisterSpillFactor =
         kFpuRegisterSize / compiler::target::kWordSize;
-    intptr_t saved_registers_size = 0;
     const bool using_shared_stub = locs->call_on_shared_slow_path();
-    if (using_shared_stub) {
-      saved_registers_size =
-          Utils::CountOneBitsWord(kDartAvailableCpuRegs) +
-          (registers->FpuRegisterCount() > 0
-               ? kFpuRegisterSpillFactor * kNumberOfFpuRegisters
-               : 0) +
-          1 /*saved PC*/;
-    } else {
-      saved_registers_size =
-          registers->CpuRegisterCount() +
-          (registers->FpuRegisterCount() * kFpuRegisterSpillFactor);
-    }
 
-    BitmapBuilder* bitmap = locs->stack_bitmap();
+    BitmapBuilder bitmap(locs->stack_bitmap());
 
-    // An instruction may have two safepoints in deferred code. The
-    // call to RecordSafepoint has the side-effect of appending the live
-    // registers to the bitmap. This is why the second call to RecordSafepoint
-    // with the same instruction (and same location summary) sees a bitmap that
-    // is larger that StackSize(). It will never be larger than StackSize() +
-    // unboxed_arg_bits_count + live_registers_size.
-    // The first safepoint will grow the bitmap to be the size of
-    // spill_area_size but the second safepoint will truncate the bitmap and
-    // append the bits for arguments and live registers to it again.
-    const intptr_t bitmap_previous_length = bitmap->Length();
-    bitmap->SetLength(spill_area_size);
-
-    intptr_t unboxed_arg_bits_count = 0;
+    // Expand the bitmap to cover the whole area reserved for spill slots.
+    // (register allocator takes care of marking slots containing live tagged
+    // values but it does not do the same for other slots so length might be
+    // below spill_area_size at this point).
+    RELEASE_ASSERT(bitmap.Length() <= spill_area_size);
+    bitmap.SetLength(spill_area_size);
 
     auto instr = current_instruction();
     const intptr_t args_count = instr->ArgumentCount();
@@ -989,18 +969,16 @@ void FlowGraphCompiler::RecordSafepoint(LocationSummary* locs,
           instr->ArgumentValueAt(i)->instruction()->AsPushArgument();
       switch (push_arg->representation()) {
         case kUnboxedInt64:
-          bitmap->SetRange(
-              bitmap->Length(),
-              bitmap->Length() + compiler::target::kIntSpillFactor - 1, false);
-          unboxed_arg_bits_count += compiler::target::kIntSpillFactor;
+          bitmap.SetRange(
+              bitmap.Length(),
+              bitmap.Length() + compiler::target::kIntSpillFactor - 1, false);
           pushed_unboxed = true;
           break;
         case kUnboxedDouble:
-          bitmap->SetRange(
-              bitmap->Length(),
-              bitmap->Length() + compiler::target::kDoubleSpillFactor - 1,
+          bitmap.SetRange(
+              bitmap.Length(),
+              bitmap.Length() + compiler::target::kDoubleSpillFactor - 1,
               false);
-          unboxed_arg_bits_count += compiler::target::kDoubleSpillFactor;
           pushed_unboxed = true;
           break;
         case kTagged:
@@ -1012,17 +990,13 @@ void FlowGraphCompiler::RecordSafepoint(LocationSummary* locs,
             // postfix.
             continue;
           }
-          bitmap->Set(bitmap->Length(), true);
-          unboxed_arg_bits_count++;
+          bitmap.Set(bitmap.Length(), true);
           break;
         default:
           UNREACHABLE();
           break;
       }
     }
-    ASSERT(bitmap_previous_length <=
-           (spill_area_size + unboxed_arg_bits_count + saved_registers_size));
-
     ASSERT(slow_path_argument_count == 0 || !using_shared_stub);
 
     // Mark the bits in the stack map in the same order we push registers in
@@ -1043,7 +1017,7 @@ void FlowGraphCompiler::RecordSafepoint(LocationSummary* locs,
           FpuRegister reg = static_cast<FpuRegister>(i);
           if (regs->ContainsFpuRegister(reg)) {
             for (intptr_t j = 0; j < kFpuRegisterSpillFactor; ++j) {
-              bitmap->Set(bitmap->Length(), false);
+              bitmap.Set(bitmap.Length(), false);
             }
           }
         }
@@ -1054,7 +1028,7 @@ void FlowGraphCompiler::RecordSafepoint(LocationSummary* locs,
       for (intptr_t i = kNumberOfCpuRegisters - 1; i >= 0; --i) {
         Register reg = static_cast<Register>(i);
         if (locs->live_registers()->ContainsRegister(reg)) {
-          bitmap->Set(bitmap->Length(), locs->live_registers()->IsTagged(reg));
+          bitmap.Set(bitmap.Length(), locs->live_registers()->IsTagged(reg));
         }
       }
     }
@@ -1063,29 +1037,28 @@ void FlowGraphCompiler::RecordSafepoint(LocationSummary* locs,
       // To simplify the code in the shared stub, we create an untagged hole
       // in the stack frame where the shared stub can leave the return address
       // before saving registers.
-      bitmap->Set(bitmap->Length(), false);
+      bitmap.Set(bitmap.Length(), false);
       if (registers->FpuRegisterCount() > 0) {
-        bitmap->SetRange(bitmap->Length(),
-                         bitmap->Length() +
-                             kNumberOfFpuRegisters * kFpuRegisterSpillFactor -
-                             1,
-                         false);
+        bitmap.SetRange(bitmap.Length(),
+                        bitmap.Length() +
+                            kNumberOfFpuRegisters * kFpuRegisterSpillFactor - 1,
+                        false);
       }
       for (intptr_t i = kNumberOfCpuRegisters - 1; i >= 0; --i) {
         if ((kReservedCpuRegisters & (1 << i)) != 0) continue;
         const Register reg = static_cast<Register>(i);
-        bitmap->Set(bitmap->Length(),
-                    locs->live_registers()->ContainsRegister(reg) &&
-                        locs->live_registers()->IsTagged(reg));
+        bitmap.Set(bitmap.Length(),
+                   locs->live_registers()->ContainsRegister(reg) &&
+                       locs->live_registers()->IsTagged(reg));
       }
     }
 
     // Arguments pushed after live registers in the slow path are tagged.
     for (intptr_t i = 0; i < slow_path_argument_count; ++i) {
-      bitmap->Set(bitmap->Length(), true);
+      bitmap.Set(bitmap.Length(), true);
     }
 
-    compressed_stackmaps_builder_->AddEntry(assembler()->CodeSize(), bitmap,
+    compressed_stackmaps_builder_->AddEntry(assembler()->CodeSize(), &bitmap,
                                             spill_area_size);
   }
 }

runtime/vm/compiler/backend/linearscan.cc

@@ -471,7 +471,7 @@ void LiveRange::Print() {
   SafepointPosition* safepoint = first_safepoint();
   while (safepoint != NULL) {
     THR_Print("    Safepoint [%" Pd "]: ", safepoint->pos());
-    safepoint->locs()->stack_bitmap()->Print();
+    safepoint->locs()->stack_bitmap().Print();
     THR_Print("\n");
     safepoint = safepoint->next();
   }

runtime/vm/compiler/backend/locations.h

@@ -781,13 +781,8 @@ class LocationSummary : public ZoneAllocated {
 
   void set_out(intptr_t index, Location loc);
 
-  BitmapBuilder* stack_bitmap() {
-    if (stack_bitmap_ == NULL) {
-      stack_bitmap_ = new BitmapBuilder();
-    }
-    return stack_bitmap_;
-  }
-  void SetStackBit(intptr_t index) { stack_bitmap()->Set(index, true); }
+  const BitmapBuilder& stack_bitmap() { return EnsureStackBitmap(); }
+  void SetStackBit(intptr_t index) { EnsureStackBitmap().Set(index, true); }
 
   bool always_calls() const {
     return contains_call_ == kCall || contains_call_ == kCallCalleeSafe;
@@ -820,6 +815,13 @@ class LocationSummary : public ZoneAllocated {
 #endif
 
  private:
+  BitmapBuilder& EnsureStackBitmap() {
+    if (stack_bitmap_ == NULL) {
+      stack_bitmap_ = new BitmapBuilder();
+    }
+    return *stack_bitmap_;
+  }
+
   const intptr_t num_inputs_;
   Location* input_locations_;
   const intptr_t num_temps_;