[Interpreter] Avoid dereferencing handles in ConstantPoolArrayBuilder.

Changes ConstantPoolArrayBuilder to do object lookups using the location
of the handles, rather than dereferencing the handles and comparing the
objects. This also updates CanonicalHandleScope when internalizing AST
nodes to ensure that duplicate objects share the same handles and so are
only added to the constant pool once.

BUG=v8:5203

Review-Url: https://codereview.chromium.org/2204243003
Cr-Commit-Position: refs/heads/master@{#38366}

src/compiler.cc

@@ -540,6 +540,12 @@ MUST_USE_RESULT MaybeHandle<Code> GetUnoptimizedCode(CompilationInfo* info) {
   VMState<COMPILER> state(info->isolate());
   PostponeInterruptsScope postpone(info->isolate());
 
+  // Create a canonical handle scope if compiling ignition bytecode. This is
+  // required by the constant array builder to de-duplicate common objects
+  // without dereferencing handles.
+  std::unique_ptr<CanonicalHandleScope> canonical;
+  if (FLAG_ignition) canonical.reset(new CanonicalHandleScope(info->isolate()));
+
   // Parse and update CompilationInfo with the results.
   if (!Parser::ParseStatic(info->parse_info())) return MaybeHandle<Code>();
   Handle<SharedFunctionInfo> shared = info->shared_info();
@@ -1068,6 +1074,12 @@ Handle<SharedFunctionInfo> CompileToplevel(CompilationInfo* info) {
   ParseInfo* parse_info = info->parse_info();
   Handle<Script> script = parse_info->script();
 
+  // Create a canonical handle scope if compiling ignition bytecode. This is
+  // required by the constant array builder to de-duplicate common objects
+  // without dereferencing handles.
+  std::unique_ptr<CanonicalHandleScope> canonical;
+  if (FLAG_ignition) canonical.reset(new CanonicalHandleScope(isolate));
+
   // TODO(svenpanne) Obscure place for this, perhaps move to OnBeforeCompile?
   FixedArray* array = isolate->native_context()->embedder_data();
   script->set_context_data(array->get(v8::Context::kDebugIdIndex));
@@ -1110,6 +1122,7 @@ Handle<SharedFunctionInfo> CompileToplevel(CompilationInfo* info) {
         parse_info->set_cached_data(nullptr);
         parse_info->set_compile_options(ScriptCompiler::kNoCompileOptions);
       }
+
       if (!Parser::ParseStatic(parse_info)) {
         return Handle<SharedFunctionInfo>::null();
       }

src/interpreter/constant-array-builder.cc

@@ -4,6 +4,8 @@
 
 #include "src/interpreter/constant-array-builder.h"
 
+#include <set>
+
 #include "src/isolate.h"
 #include "src/objects-inl.h"
 
@@ -53,6 +55,15 @@ void ConstantArrayBuilder::ConstantArraySlice::InsertAt(size_t index,
   constants_[index - start_index()] = object;
 }
 
+bool ConstantArrayBuilder::ConstantArraySlice::AllElementsAreUnique() const {
+  std::set<Object*> elements;
+  for (auto constant : constants_) {
+    if (elements.find(*constant) != elements.end()) return false;
+    elements.insert(*constant);
+  }
+  return true;
+}
+
 STATIC_CONST_MEMBER_DEFINITION const size_t ConstantArrayBuilder::k8BitCapacity;
 STATIC_CONST_MEMBER_DEFINITION const size_t
     ConstantArrayBuilder::k16BitCapacity;
@@ -60,7 +71,7 @@ STATIC_CONST_MEMBER_DEFINITION const size_t
     ConstantArrayBuilder::k32BitCapacity;
 
 ConstantArrayBuilder::ConstantArrayBuilder(Isolate* isolate, Zone* zone)
-    : isolate_(isolate), constants_map_(isolate->heap(), zone) {
+    : isolate_(isolate), constants_map_(zone) {
   idx_slice_[0] =
       new (zone) ConstantArraySlice(zone, 0, k8BitCapacity, OperandSize::kByte);
   idx_slice_[1] = new (zone) ConstantArraySlice(
@@ -111,6 +122,10 @@ Handle<FixedArray> ConstantArrayBuilder::ToFixedArray() {
     }
     DCHECK(array_index == 0 ||
            base::bits::IsPowerOfTwo32(static_cast<uint32_t>(array_index)));
+    // Different slices might contain the same element due to reservations, but
+    // all elements within a slice should be unique. If this DCHECK fails, then
+    // the AST nodes are not being internalized within a CanonicalHandleScope.
+    DCHECK(slice->AllElementsAreUnique());
     // Copy objects from slice into array.
     for (size_t i = 0; i < slice->size(); ++i) {
       fixed_array->set(array_index++, *slice->At(slice->start_index() + i));
@@ -124,21 +139,19 @@ Handle<FixedArray> ConstantArrayBuilder::ToFixedArray() {
     }
   }
   DCHECK_EQ(array_index, fixed_array->length());
-  constants_map()->Clear();
   return fixed_array;
 }
 
 size_t ConstantArrayBuilder::Insert(Handle<Object> object) {
-  index_t* entry = constants_map()->Find(object);
-  return (entry == nullptr) ? AllocateEntry(object) : *entry;
+  auto entry = constants_map_.find(object.address());
+  return (entry == constants_map_.end()) ? AllocateEntry(object)
+                                         : entry->second;
 }
 
 ConstantArrayBuilder::index_t ConstantArrayBuilder::AllocateEntry(
     Handle<Object> object) {
-  DCHECK(!object->IsOddball());
   index_t index = AllocateIndex(object);
-  index_t* entry = constants_map()->Get(object);
-  *entry = index;
+  constants_map_[object.address()] = index;
   return index;
 }
 
@@ -200,18 +213,19 @@ size_t ConstantArrayBuilder::CommitReservedEntry(OperandSize operand_size,
                                                  Handle<Object> object) {
   DiscardReservedEntry(operand_size);
   size_t index;
-  index_t* entry = constants_map()->Find(object);
-  if (nullptr == entry) {
+  auto entry = constants_map_.find(object.address());
+  if (entry == constants_map_.end()) {
     index = AllocateEntry(object);
   } else {
     ConstantArraySlice* slice = OperandSizeToSlice(operand_size);
-    if (*entry > slice->max_index()) {
+    index = entry->second;
+    if (index > slice->max_index()) {
       // The object is already in the constant array, but may have an
       // index too big for the reserved operand_size. So, duplicate
       // entry with the smaller operand size.
-      *entry = static_cast<index_t>(slice->Allocate(object));
+      index = slice->Allocate(object);
+      constants_map_[object.address()] = static_cast<index_t>(index);
     }
-    index = *entry;
   }
   return index;
 }

src/interpreter/constant-array-builder.h

@@ -81,6 +81,7 @@ class ConstantArrayBuilder final BASE_EMBEDDED {
     size_t Allocate(Handle<Object> object);
     Handle<Object> At(size_t index) const;
     void InsertAt(size_t index, Handle<Object> object);
+    bool AllElementsAreUnique() const;
 
     inline size_t available() const { return capacity() - reserved() - size(); }
     inline size_t reserved() const { return reserved_; }
@@ -103,11 +104,9 @@ class ConstantArrayBuilder final BASE_EMBEDDED {
   ConstantArraySlice* IndexToSlice(size_t index) const;
   ConstantArraySlice* OperandSizeToSlice(OperandSize operand_size) const;
 
-  IdentityMap<index_t>* constants_map() { return &constants_map_; }
-
   Isolate* isolate_;
   ConstantArraySlice* idx_slice_[3];
-  IdentityMap<index_t> constants_map_;
+  ZoneMap<Address, index_t> constants_map_;
 };
 
 }  // namespace interpreter

test/cctest/interpreter/test-interpreter.cc

@@ -1262,19 +1262,21 @@ TEST(InterpreterHeapNumberComparisons) {
 
 
 TEST(InterpreterStringComparisons) {
+  HandleAndZoneScope handles;
+  i::Isolate* isolate = handles.main_isolate();
+  i::Factory* factory = isolate->factory();
+
   std::string inputs[] = {"A", "abc", "z", "", "Foo!", "Foo"};
 
   for (size_t c = 0; c < arraysize(kComparisonTypes); c++) {
     Token::Value comparison = kComparisonTypes[c];
     for (size_t i = 0; i < arraysize(inputs); i++) {
       for (size_t j = 0; j < arraysize(inputs); j++) {
+        CanonicalHandleScope canonical(isolate);
         const char* lhs = inputs[i].c_str();
         const char* rhs = inputs[j].c_str();
-        HandleAndZoneScope handles;
-        i::Factory* factory = handles.main_isolate()->factory();
         BytecodeArrayBuilder builder(handles.main_isolate(),
                                      handles.main_zone(), 0, 0, 1);
-
         Register r0(0);
         builder.LoadLiteral(factory->NewStringFromAsciiChecked(lhs))
             .StoreAccumulatorInRegister(r0)

test/cctest/test-api.cc

@@ -23940,7 +23940,6 @@ void RunStreamingTest(const char** chunks,
   delete[] full_source;
 }
 
-
 TEST(StreamingSimpleScript) {
   // This script is unrealistically small, since no one chunk is enough to fill
   // the backing buffer of Scanner, let alone overflow it.
@@ -23949,6 +23948,17 @@ TEST(StreamingSimpleScript) {
   RunStreamingTest(chunks);
 }
 
+TEST(StreamingScriptConstantArray) {
+  // When run with Ignition, tests that the streaming parser canonicalizes
+  // handles so that they are only added to the constant pool array once.
+  const char* chunks[] = {"var a = {};",
+                          "var b = {};",
+                          "var c = 'testing';",
+                          "var d = 'testing';",
+                          "13;",
+                          NULL};
+  RunStreamingTest(chunks);
+}
 
 TEST(StreamingBiggerScript) {
   const char* chunk1 =

test/unittests/interpreter/bytecode-array-builder-unittest.cc

@@ -22,6 +22,7 @@ class BytecodeArrayBuilderTest : public TestWithIsolateAndZone {
 
 
 TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
+  CanonicalHandleScope canonical(isolate());
   BytecodeArrayBuilder builder(isolate(), zone(), 0, 1, 131);
   Factory* factory = isolate()->factory();
 
@@ -438,6 +439,7 @@ TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
 
 
 TEST_F(BytecodeArrayBuilderTest, FrameSizesLookGood) {
+  CanonicalHandleScope canonical(isolate());
   for (int locals = 0; locals < 5; locals++) {
     for (int contexts = 0; contexts < 4; contexts++) {
       for (int temps = 0; temps < 3; temps++) {
@@ -470,6 +472,7 @@ TEST_F(BytecodeArrayBuilderTest, FrameSizesLookGood) {
 
 
 TEST_F(BytecodeArrayBuilderTest, RegisterValues) {
+  CanonicalHandleScope canonical(isolate());
   int index = 1;
 
   Register the_register(index);
@@ -482,6 +485,7 @@ TEST_F(BytecodeArrayBuilderTest, RegisterValues) {
 
 
 TEST_F(BytecodeArrayBuilderTest, Parameters) {
+  CanonicalHandleScope canonical(isolate());
   BytecodeArrayBuilder builder(isolate(), zone(), 10, 0, 0);
 
   Register param0(builder.Parameter(0));
@@ -491,6 +495,7 @@ TEST_F(BytecodeArrayBuilderTest, Parameters) {
 
 
 TEST_F(BytecodeArrayBuilderTest, RegisterType) {
+  CanonicalHandleScope canonical(isolate());
   BytecodeArrayBuilder builder(isolate(), zone(), 10, 0, 3);
   BytecodeRegisterAllocator register_allocator(
       zone(), builder.temporary_register_allocator());
@@ -514,6 +519,7 @@ TEST_F(BytecodeArrayBuilderTest, RegisterType) {
 
 
 TEST_F(BytecodeArrayBuilderTest, Constants) {
+  CanonicalHandleScope canonical(isolate());
   BytecodeArrayBuilder builder(isolate(), zone(), 0, 0, 0);
 
   Factory* factory = isolate()->factory();
@@ -541,6 +547,7 @@ static Bytecode PeepholeToBoolean(Bytecode jump_bytecode) {
 }
 
 TEST_F(BytecodeArrayBuilderTest, ForwardJumps) {
+  CanonicalHandleScope canonical(isolate());
   static const int kFarJumpDistance = 256;
 
   BytecodeArrayBuilder builder(isolate(), zone(), 0, 0, 1);
@@ -662,6 +669,7 @@ TEST_F(BytecodeArrayBuilderTest, ForwardJumps) {
 
 
 TEST_F(BytecodeArrayBuilderTest, BackwardJumps) {
+  CanonicalHandleScope canonical(isolate());
   BytecodeArrayBuilder builder(isolate(), zone(), 0, 0, 1);
 
   Register reg(0);
@@ -779,6 +787,7 @@ TEST_F(BytecodeArrayBuilderTest, BackwardJumps) {
 
 
 TEST_F(BytecodeArrayBuilderTest, LabelReuse) {
+  CanonicalHandleScope canonical(isolate());
   BytecodeArrayBuilder builder(isolate(), zone(), 0, 0, 0);
 
   // Labels can only have 1 forward reference, but
@@ -811,6 +820,7 @@ TEST_F(BytecodeArrayBuilderTest, LabelReuse) {
 
 
 TEST_F(BytecodeArrayBuilderTest, LabelAddressReuse) {
+  CanonicalHandleScope canonical(isolate());
   static const int kRepeats = 3;
 
   BytecodeArrayBuilder builder(isolate(), zone(), 0, 0, 0);

test/unittests/interpreter/constant-array-builder-unittest.cc

@@ -29,6 +29,7 @@ STATIC_CONST_MEMBER_DEFINITION const size_t
     ConstantArrayBuilderTest::k8BitCapacity;
 
 TEST_F(ConstantArrayBuilderTest, AllocateAllEntries) {
+  CanonicalHandleScope canonical(isolate());
   ConstantArrayBuilder builder(isolate(), zone());
   for (size_t i = 0; i < k16BitCapacity; i++) {
     builder.Insert(handle(Smi::FromInt(static_cast<int>(i)), isolate()));
@@ -40,6 +41,7 @@ TEST_F(ConstantArrayBuilderTest, AllocateAllEntries) {
 }
 
 TEST_F(ConstantArrayBuilderTest, AllocateEntriesWithIdx8Reservations) {
+  CanonicalHandleScope canonical(isolate());
   for (size_t reserved = 1; reserved < k8BitCapacity; reserved *= 3) {
     ConstantArrayBuilder builder(isolate(), zone());
     for (size_t i = 0; i < reserved; i++) {
@@ -109,6 +111,7 @@ TEST_F(ConstantArrayBuilderTest, AllocateEntriesWithIdx8Reservations) {
 }
 
 TEST_F(ConstantArrayBuilderTest, AllocateEntriesWithWideReservations) {
+  CanonicalHandleScope canonical(isolate());
   for (size_t reserved = 1; reserved < k8BitCapacity; reserved *= 3) {
     ConstantArrayBuilder builder(isolate(), zone());
     for (size_t i = 0; i < k8BitCapacity; i++) {
@@ -145,6 +148,7 @@ TEST_F(ConstantArrayBuilderTest, AllocateEntriesWithWideReservations) {
 
 
 TEST_F(ConstantArrayBuilderTest, ToFixedArray) {
+  CanonicalHandleScope canonical(isolate());
   ConstantArrayBuilder builder(isolate(), zone());
   static const size_t kNumberOfElements = 37;
   for (size_t i = 0; i < kNumberOfElements; i++) {
@@ -160,6 +164,7 @@ TEST_F(ConstantArrayBuilderTest, ToFixedArray) {
 }
 
 TEST_F(ConstantArrayBuilderTest, ToLargeFixedArray) {
+  CanonicalHandleScope canonical(isolate());
   ConstantArrayBuilder builder(isolate(), zone());
   static const size_t kNumberOfElements = 37373;
   for (size_t i = 0; i < kNumberOfElements; i++) {
@@ -175,6 +180,7 @@ TEST_F(ConstantArrayBuilderTest, ToLargeFixedArray) {
 }
 
 TEST_F(ConstantArrayBuilderTest, GapFilledWhenLowReservationCommitted) {
+  CanonicalHandleScope canonical(isolate());
   ConstantArrayBuilder builder(isolate(), zone());
   for (size_t i = 0; i < k8BitCapacity; i++) {
     OperandSize operand_size = builder.CreateReservedEntry();
@@ -201,6 +207,7 @@ TEST_F(ConstantArrayBuilderTest, GapFilledWhenLowReservationCommitted) {
 }
 
 TEST_F(ConstantArrayBuilderTest, GapNotFilledWhenLowReservationDiscarded) {
+  CanonicalHandleScope canonical(isolate());
   ConstantArrayBuilder builder(isolate(), zone());
   for (size_t i = 0; i < k8BitCapacity; i++) {
     OperandSize operand_size = builder.CreateReservedEntry();
@@ -227,6 +234,7 @@ TEST_F(ConstantArrayBuilderTest, GapNotFilledWhenLowReservationDiscarded) {
 }
 
 TEST_F(ConstantArrayBuilderTest, HolesWithUnusedReservations) {
+  CanonicalHandleScope canonical(isolate());
   static int kNumberOfHoles = 128;
   ConstantArrayBuilder builder(isolate(), zone());
   for (int i = 0; i < kNumberOfHoles; ++i) {
@@ -250,6 +258,7 @@ TEST_F(ConstantArrayBuilderTest, HolesWithUnusedReservations) {
 }
 
 TEST_F(ConstantArrayBuilderTest, ReservationsAtAllScales) {
+  CanonicalHandleScope canonical(isolate());
   ConstantArrayBuilder builder(isolate(), zone());
   for (int i = 0; i < 256; i++) {
     CHECK_EQ(builder.CreateReservedEntry(), OperandSize::kByte);
@@ -284,6 +293,7 @@ TEST_F(ConstantArrayBuilderTest, ReservationsAtAllScales) {
 }
 
 TEST_F(ConstantArrayBuilderTest, AllocateEntriesWithFixedReservations) {
+  CanonicalHandleScope canonical(isolate());
   ConstantArrayBuilder builder(isolate(), zone());
   for (size_t i = 0; i < k16BitCapacity; i++) {
     if ((i % 2) == 0) {