[snapshot] improve API references.

We now only require API references to be provided when we
actually deserialize them. Also changed the internal implementation
to avoid copying API references into V8.

R=petermarshall@chromium.org

Bug: v8:6448
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: Iddb0465ff6e95020006d41b5e87614dce8f0140b
Reviewed-on: https://chromium-review.googlesource.com/632098Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47649}

src/api.cc

@@ -700,10 +700,6 @@ StartupData SnapshotCreator::CreateBlob(
     fun->CompleteInobjectSlackTrackingIfActive();
   }
 
-#ifdef DEBUG
-  i::ExternalReferenceTable::instance(isolate)->ResetCount();
-#endif  // DEBUG
-
   i::StartupSerializer startup_serializer(isolate, function_code_handling);
   startup_serializer.SerializeStrongReferences();
 
@@ -736,12 +732,6 @@ StartupData SnapshotCreator::CreateBlob(
   startup_serializer.SerializeWeakReferencesAndDeferred();
   can_be_rehashed = can_be_rehashed && startup_serializer.can_be_rehashed();
 
-#ifdef DEBUG
-  if (i::FLAG_external_reference_stats) {
-    i::ExternalReferenceTable::instance(isolate)->PrintCount();
-  }
-#endif  // DEBUG
-
   i::SnapshotData startup_snapshot(&startup_serializer);
   StartupData result = i::Snapshot::CreateSnapshotBlob(
       &startup_snapshot, context_snapshots, can_be_rehashed);

src/base/logging.h

@@ -103,6 +103,8 @@ V8_BASE_EXPORT void SetPrintStackTrace(void (*print_stack_trace_)());
     CHECK_WITH_MSG(_cmp, #lhs " " #op " " #rhs);                           \
   } while (0)
 
+#define DCHECK_WITH_MSG(condition, msg) void(0);
+
 #endif
 
 template <typename Op>

src/external-reference-table.cc

@@ -45,23 +45,8 @@ ExternalReferenceTable::ExternalReferenceTable(Isolate* isolate) {
   AddIsolateAddresses(isolate);
   AddAccessors(isolate);
   AddStubCache(isolate);
-  // API references must be added last.
-  AddApiReferences(isolate);
 }
 
-#ifdef DEBUG
-void ExternalReferenceTable::ResetCount() {
-  for (ExternalReferenceEntry& entry : refs_) entry.count = 0;
-}
-
-void ExternalReferenceTable::PrintCount() {
-  for (int i = 0; i < refs_.length(); i++) {
-    v8::base::OS::Print("index=%5d count=%5d  %-60s\n", i, refs_[i].count,
-                        refs_[i].name);
-  }
-}
-#endif  // DEBUG
-
 const char* ExternalReferenceTable::ResolveSymbol(void* address) {
 #ifdef SYMBOLIZE_FUNCTION
   char** names = backtrace_symbols(&address, 1);
@@ -448,19 +433,5 @@ void ExternalReferenceTable::AddStubCache(Isolate* isolate) {
       "Store StubCache::secondary_->map");
 }
 
-void ExternalReferenceTable::AddApiReferences(Isolate* isolate) {
-  // Add external references provided by the embedder (a null-terminated
-  // array).
-  api_refs_start_ = size();
-  intptr_t* api_external_references = isolate->api_external_references();
-  if (api_external_references != nullptr) {
-    while (*api_external_references != 0) {
-      Address address = reinterpret_cast<Address>(*api_external_references);
-      Add(address, ResolveSymbol(address));
-      api_external_references++;
-    }
-  }
-}
-
 }  // namespace internal
 }  // namespace v8

src/external-reference-table.h

@@ -25,15 +25,6 @@ class ExternalReferenceTable {
   uint32_t size() const { return static_cast<uint32_t>(refs_.length()); }
   Address address(uint32_t i) { return refs_[i].address; }
   const char* name(uint32_t i) { return refs_[i].name; }
-  bool is_api_reference(uint32_t i) { return i >= api_refs_start_; }
-  uint32_t num_api_references() { return size() - api_refs_start_; }
-
-#ifdef DEBUG
-  void increment_count(uint32_t i) { refs_[i].count++; }
-  int count(uint32_t i) { return refs_[i].count; }
-  void ResetCount();
-  void PrintCount();
-#endif  // DEBUG
 
   static const char* ResolveSymbol(void* address);
 
@@ -41,19 +32,12 @@ class ExternalReferenceTable {
   struct ExternalReferenceEntry {
     Address address;
     const char* name;
-#ifdef DEBUG
-    int count;
-#endif  // DEBUG
   };
 
   explicit ExternalReferenceTable(Isolate* isolate);
 
   void Add(Address address, const char* name) {
-#ifdef DEBUG
-    ExternalReferenceEntry entry = {address, name, 0};
-#else
     ExternalReferenceEntry entry = {address, name};
-#endif  // DEBUG
     refs_.Add(entry);
   }
 
@@ -63,11 +47,8 @@ class ExternalReferenceTable {
   void AddIsolateAddresses(Isolate* isolate);
   void AddAccessors(Isolate* isolate);
   void AddStubCache(Isolate* isolate);
-  void AddApiReferences(Isolate* isolate);
 
   List<ExternalReferenceEntry> refs_;
-  uint32_t api_refs_start_;
-
   DISALLOW_COPY_AND_ASSIGN(ExternalReferenceTable);
 };
 

src/snapshot/code-serializer.cc

@@ -395,9 +395,6 @@ SerializedCodeData::SanityCheckResult SerializedCodeData::SanityCheck(
   if (this->size_ < kHeaderSize) return INVALID_HEADER;
   uint32_t magic_number = GetMagicNumber();
   if (magic_number != ComputeMagicNumber(isolate)) return MAGIC_NUMBER_MISMATCH;
-  if (GetExtraReferences() > GetExtraReferences(isolate)) {
-    return MAGIC_NUMBER_MISMATCH;
-  }
   uint32_t version_hash = GetHeaderValue(kVersionHashOffset);
   uint32_t source_hash = GetHeaderValue(kSourceHashOffset);
   uint32_t cpu_features = GetHeaderValue(kCpuFeaturesOffset);

src/snapshot/deserializer.cc

@@ -59,12 +59,17 @@ void Deserializer::Initialize(Isolate* isolate) {
   isolate_ = isolate;
   DCHECK_NULL(external_reference_table_);
   external_reference_table_ = ExternalReferenceTable::instance(isolate);
+#ifdef DEBUG
+  // Count the number of external references registered through the API.
+  num_api_references_ = 0;
+  if (isolate_->api_external_references() != nullptr) {
+    while (isolate_->api_external_references()[num_api_references_] != 0) {
+      num_api_references_++;
+    }
+  }
+#endif  // DEBUG
   CHECK_EQ(magic_number_,
            SerializedData::ComputeMagicNumber(external_reference_table_));
-  // The current isolate must have at least as many API-provided external
-  // references as the to-be-deserialized snapshot expects and refers to.
-  CHECK_LE(num_extra_references_,
-           SerializedData::GetExtraReferences(external_reference_table_));
 }
 
 void Deserializer::SortMapDescriptors() {
@@ -564,6 +569,20 @@ bool Deserializer::ReadData(Object** current, Object** limit, int source_space,
         break;
       }
 
+      case kApiReference: {
+        int skip = source_.GetInt();
+        current = reinterpret_cast<Object**>(
+            reinterpret_cast<Address>(current) + skip);
+        uint32_t reference_id = static_cast<uint32_t>(source_.GetInt());
+        DCHECK_WITH_MSG(reference_id < num_api_references_,
+                        "too few external references provided through the API");
+        Address address = reinterpret_cast<Address>(
+            isolate->api_external_references()[reference_id]);
+        memcpy(current, &address, kPointerSize);
+        current++;
+        break;
+      }
+
       case kAlignmentPrefix:
       case kAlignmentPrefix + 1:
       case kAlignmentPrefix + 2:

src/snapshot/deserializer.h

@@ -47,7 +47,6 @@ class Deserializer : public SerializerDeserializer {
       : isolate_(NULL),
         source_(data->Payload()),
         magic_number_(data->GetMagicNumber()),
-        num_extra_references_(data->GetExtraReferences()),
         next_map_index_(0),
         external_reference_table_(NULL),
         deserialized_large_objects_(0),
@@ -140,7 +139,6 @@ class Deserializer : public SerializerDeserializer {
 
   SnapshotByteSource source_;
   uint32_t magic_number_;
-  uint32_t num_extra_references_;
 
   // The address of the next object that will be allocated in each space.
   // Each space has a number of chunks reserved by the GC, with each chunk
@@ -169,6 +167,10 @@ class Deserializer : public SerializerDeserializer {
   // TODO(6593): generalize rehashing, and remove this flag.
   bool can_rehash_;
 
+#ifdef DEBUG
+  uint32_t num_api_references_;
+#endif  // DEBUG
+
   DISALLOW_COPY_AND_ASSIGN(Deserializer);
 };
 

src/snapshot/serializer-common.cc

@@ -13,29 +13,52 @@ namespace v8 {
 namespace internal {
 
 ExternalReferenceEncoder::ExternalReferenceEncoder(Isolate* isolate) {
-  map_ = isolate->external_reference_map();
 #ifdef DEBUG
-  table_ = ExternalReferenceTable::instance(isolate);
+  api_references_ = isolate->api_external_references();
+  if (api_references_ != nullptr) {
+    for (uint32_t i = 0; api_references_[i] != 0; ++i) count_.push_back(0);
+  }
 #endif  // DEBUG
+  map_ = isolate->external_reference_map();
   if (map_ != nullptr) return;
   map_ = new AddressToIndexHashMap();
+  isolate->set_external_reference_map(map_);
+  // Add V8's external references.
   ExternalReferenceTable* table = ExternalReferenceTable::instance(isolate);
   for (uint32_t i = 0; i < table->size(); ++i) {
     Address addr = table->address(i);
-    // Ignore duplicate API references.
-    if (table->is_api_reference(i) && !map_->Get(addr).IsNothing()) continue;
-#ifndef V8_OS_WIN
-    // TODO(yangguo): On Windows memcpy and memmove can end up at the same
-    // address due to ICF. See http://crbug.com/726896.
-    DCHECK(map_->Get(addr).IsNothing());
-#endif
-    map_->Set(addr, i);
+    // Ignore duplicate references.
+    // This can happen due to ICF. See http://crbug.com/726896.
+    if (map_->Get(addr).IsNothing()) map_->Set(addr, Value::Encode(i, false));
     DCHECK(map_->Get(addr).IsJust());
   }
-  isolate->set_external_reference_map(map_);
+  // Add external references provided by the embedder.
+  intptr_t* api_references = isolate->api_external_references();
+  if (api_references == nullptr) return;
+  for (uint32_t i = 0; api_references[i] != 0; ++i) {
+    Address addr = reinterpret_cast<Address>(api_references[i]);
+    // Ignore duplicate references.
+    // This can happen due to ICF. See http://crbug.com/726896.
+    if (map_->Get(addr).IsNothing()) map_->Set(addr, Value::Encode(i, true));
+    DCHECK(map_->Get(addr).IsJust());
+  }
+}
+
+ExternalReferenceEncoder::~ExternalReferenceEncoder() {
+#ifdef DEBUG
+  if (!i::FLAG_external_reference_stats) return;
+  if (api_references_ == nullptr) return;
+  for (uint32_t i = 0; api_references_[i] != 0; ++i) {
+    Address addr = reinterpret_cast<Address>(api_references_[i]);
+    DCHECK(map_->Get(addr).IsJust());
+    v8::base::OS::Print("index=%5d count=%5d  %-60s\n", i, count_[i],
+                        ExternalReferenceTable::ResolveSymbol(addr));
+  }
+#endif  // DEBUG
 }
 
-uint32_t ExternalReferenceEncoder::Encode(Address address) const {
+ExternalReferenceEncoder::Value ExternalReferenceEncoder::Encode(
+    Address address) {
   Maybe<uint32_t> maybe_index = map_->Get(address);
   if (maybe_index.IsNothing()) {
     void* addr = address;
@@ -43,10 +66,11 @@ uint32_t ExternalReferenceEncoder::Encode(Address address) const {
     v8::base::OS::PrintError("%s", ExternalReferenceTable::ResolveSymbol(addr));
     v8::base::OS::Abort();
   }
+  Value result(maybe_index.FromJust());
 #ifdef DEBUG
-  table_->increment_count(maybe_index.FromJust());
+  if (result.is_from_api()) count_[result.index()]++;
 #endif  // DEBUG
-  return maybe_index.FromJust();
+  return result;
 }
 
 const char* ExternalReferenceEncoder::NameOfAddress(Isolate* isolate,

src/snapshot/serializer-common.h

@@ -19,16 +19,36 @@ class Isolate;
 
 class ExternalReferenceEncoder {
  public:
+  class Value {
+   public:
+    explicit Value(uint32_t raw) : value_(raw) {}
+    static uint32_t Encode(uint32_t index, bool is_from_api) {
+      return Index::encode(index) | IsFromAPI::encode(is_from_api);
+    }
+
+    bool is_from_api() const { return IsFromAPI::decode(value_); }
+    uint32_t index() const { return Index::decode(value_); }
+    uint32_t raw() const { return value_; }
+
+   private:
+    class Index : public BitField<uint32_t, 0, 31> {};
+    class IsFromAPI : public BitField<bool, 31, 1> {};
+    uint32_t value_;
+  };
+
   explicit ExternalReferenceEncoder(Isolate* isolate);
+  ~ExternalReferenceEncoder();
 
-  uint32_t Encode(Address key) const;
+  Value Encode(Address key);
 
   const char* NameOfAddress(Isolate* isolate, Address address) const;
 
  private:
   AddressToIndexHashMap* map_;
+
 #ifdef DEBUG
-  ExternalReferenceTable* table_;
+  std::vector<int> count_;
+  intptr_t* api_references_;
 #endif  // DEBUG
 
   DISALLOW_COPY_AND_ASSIGN(ExternalReferenceEncoder);
@@ -179,6 +199,9 @@ class SerializerDeserializer : public RootVisitor {
   // Used for embedder-allocated backing stores for TypedArrays.
   static const int kOffHeapBackingStore = 0x35;
 
+  // Used to encode external referenced provided through the API.
+  static const int kApiReference = 0x36;
+
   // 8 hot (recently seen or back-referenced) objects with optional skip.
   static const int kNumberOfHotObjects = 8;
   STATIC_ASSERT(kNumberOfHotObjects == HotObjectsList::kSize);
@@ -188,7 +211,7 @@ class SerializerDeserializer : public RootVisitor {
   static const int kHotObjectWithSkip = 0x58;
   static const int kHotObjectMask = 0x07;
 
-  // 0x36..0x37, 0x55..0x57, 0x75..0x7f unused.
+  // 0x37, 0x55..0x57, 0x75..0x7f unused.
 
   // ---------- byte code range 0x80..0xff ----------
   // First 32 root array items.
@@ -254,26 +277,17 @@ class SerializedData {
   }
 
   uint32_t GetMagicNumber() const { return GetHeaderValue(kMagicNumberOffset); }
-  uint32_t GetExtraReferences() const {
-    return GetHeaderValue(kExtraExternalReferencesOffset);
-  }
 
   class ChunkSizeBits : public BitField<uint32_t, 0, 31> {};
   class IsLastChunkBits : public BitField<bool, 31, 1> {};
 
   static uint32_t ComputeMagicNumber(ExternalReferenceTable* table) {
-    uint32_t external_refs = table->size() - table->num_api_references();
+    uint32_t external_refs = table->size();
     return 0xC0DE0000 ^ external_refs;
   }
-  static uint32_t GetExtraReferences(ExternalReferenceTable* table) {
-    return table->num_api_references();
-  }
 
   static const uint32_t kMagicNumberOffset = 0;
-  static const uint32_t kExtraExternalReferencesOffset =
-      kMagicNumberOffset + kUInt32Size;
-  static const uint32_t kVersionHashOffset =
-      kExtraExternalReferencesOffset + kUInt32Size;
+  static const uint32_t kVersionHashOffset = kMagicNumberOffset + kUInt32Size;
 
  protected:
   void SetHeaderValue(uint32_t offset, uint32_t value) {
@@ -289,13 +303,9 @@ class SerializedData {
   static uint32_t ComputeMagicNumber(Isolate* isolate) {
     return ComputeMagicNumber(ExternalReferenceTable::instance(isolate));
   }
-  static uint32_t GetExtraReferences(Isolate* isolate) {
-    return GetExtraReferences(ExternalReferenceTable::instance(isolate));
-  }
 
   void SetMagicNumber(Isolate* isolate) {
     SetHeaderValue(kMagicNumberOffset, ComputeMagicNumber(isolate));
-    SetHeaderValue(kExtraExternalReferencesOffset, GetExtraReferences(isolate));
   }
 
   byte* data_;

src/snapshot/serializer.cc

@@ -727,9 +727,14 @@ void Serializer::ObjectSerializer::VisitExternalReference(Foreign* host,
   int skip = OutputRawData(reinterpret_cast<Address>(p),
                            kCanReturnSkipInsteadOfSkipping);
   Address target = *p;
-  sink_->Put(kExternalReference + kPlain + kStartOfObject, "ExternalRef");
+  auto encoded_reference = serializer_->EncodeExternalReference(target);
+  if (encoded_reference.is_from_api()) {
+    sink_->Put(kApiReference, "ApiRef");
+  } else {
+    sink_->Put(kExternalReference + kPlain + kStartOfObject, "ExternalRef");
+  }
   sink_->PutInt(skip, "SkipB4ExternalRef");
-  sink_->PutInt(serializer_->EncodeExternalReference(target), "reference id");
+  sink_->PutInt(encoded_reference.index(), "reference index");
   bytes_processed_so_far_ += kPointerSize;
 }
 
@@ -737,12 +742,19 @@ void Serializer::ObjectSerializer::VisitExternalReference(Code* host,
                                                           RelocInfo* rinfo) {
   int skip = OutputRawData(rinfo->target_address_address(),
                            kCanReturnSkipInsteadOfSkipping);
-  HowToCode how_to_code = rinfo->IsCodedSpecially() ? kFromCode : kPlain;
   Address target = rinfo->target_external_reference();
-  sink_->Put(kExternalReference + how_to_code + kStartOfObject, "ExternalRef");
+  auto encoded_reference = serializer_->EncodeExternalReference(target);
+  if (encoded_reference.is_from_api()) {
+    DCHECK(!rinfo->IsCodedSpecially());
+    sink_->Put(kApiReference, "ApiRef");
+  } else {
+    HowToCode how_to_code = rinfo->IsCodedSpecially() ? kFromCode : kPlain;
+    sink_->Put(kExternalReference + how_to_code + kStartOfObject,
+               "ExternalRef");
+  }
   sink_->PutInt(skip, "SkipB4ExternalRef");
   DCHECK_NOT_NULL(target);  // Code does not reference null.
-  sink_->PutInt(serializer_->EncodeExternalReference(target), "reference id");
+  sink_->PutInt(encoded_reference.index(), "reference index");
   bytes_processed_so_far_ += rinfo->target_address_size();
 }
 
@@ -777,9 +789,11 @@ void Serializer::ObjectSerializer::VisitRuntimeEntry(Code* host,
                            kCanReturnSkipInsteadOfSkipping);
   HowToCode how_to_code = rinfo->IsCodedSpecially() ? kFromCode : kPlain;
   Address target = rinfo->target_address();
+  auto encoded_reference = serializer_->EncodeExternalReference(target);
+  DCHECK(!encoded_reference.is_from_api());
   sink_->Put(kExternalReference + how_to_code + kStartOfObject, "ExternalRef");
   sink_->PutInt(skip, "SkipB4ExternalRef");
-  sink_->PutInt(serializer_->EncodeExternalReference(target), "reference id");
+  sink_->PutInt(encoded_reference.index(), "reference index");
   bytes_processed_so_far_ += rinfo->target_address_size();
 }
 

src/snapshot/serializer.h

@@ -195,7 +195,7 @@ class Serializer : public SerializerDeserializer {
   SerializerReference AllocateLargeObject(int size);
   SerializerReference AllocateMap();
   SerializerReference Allocate(AllocationSpace space, int size);
-  int EncodeExternalReference(Address addr) {
+  ExternalReferenceEncoder::Value EncodeExternalReference(Address addr) {
     return external_reference_encoder_.Encode(addr);
   }
 

test/cctest/test-serialize.cc

@@ -2172,6 +2172,9 @@ intptr_t replaced_external_references[] = {
     reinterpret_cast<intptr_t>(&serialized_static_field),
     0};
 
+intptr_t short_external_references[] = {
+    reinterpret_cast<intptr_t>(SerializedCallbackReplacement), 0};
+
 TEST(SnapshotCreatorExternalReferences) {
   DisableAlwaysOpt();
   v8::StartupData blob;
@@ -2212,7 +2215,7 @@ TEST(SnapshotCreatorExternalReferences) {
     isolate->Dispose();
   }
 
-  // Deserialize with the some other external reference.
+  // Deserialize with some other external reference.
   {
     v8::Isolate::CreateParams params;
     params.snapshot_blob = &blob;
@@ -2232,6 +2235,48 @@ TEST(SnapshotCreatorExternalReferences) {
   delete[] blob.data;
 }
 
+TEST(SnapshotCreatorShortExternalReferences) {
+  DisableAlwaysOpt();
+  v8::StartupData blob;
+  {
+    v8::SnapshotCreator creator(original_external_references);
+    v8::Isolate* isolate = creator.GetIsolate();
+    {
+      v8::HandleScope handle_scope(isolate);
+      v8::Local<v8::Context> context = v8::Context::New(isolate);
+      v8::Context::Scope context_scope(context);
+      v8::Local<v8::FunctionTemplate> callback =
+          v8::FunctionTemplate::New(isolate, SerializedCallback);
+      v8::Local<v8::Value> function =
+          callback->GetFunction(context).ToLocalChecked();
+      CHECK(context->Global()->Set(context, v8_str("f"), function).FromJust());
+      ExpectInt32("f()", 42);
+      creator.SetDefaultContext(context);
+    }
+    blob =
+        creator.CreateBlob(v8::SnapshotCreator::FunctionCodeHandling::kClear);
+  }
+
+  // Deserialize with an incomplete list of external references.
+  {
+    v8::Isolate::CreateParams params;
+    params.snapshot_blob = &blob;
+    params.array_buffer_allocator = CcTest::array_buffer_allocator();
+    params.external_references = short_external_references;
+    // Test-appropriate equivalent of v8::Isolate::New.
+    v8::Isolate* isolate = TestIsolate::New(params);
+    {
+      v8::Isolate::Scope isolate_scope(isolate);
+      v8::HandleScope handle_scope(isolate);
+      v8::Local<v8::Context> context = v8::Context::New(isolate);
+      v8::Context::Scope context_scope(context);
+      ExpectInt32("f()", 1337);
+    }
+    isolate->Dispose();
+  }
+  delete[] blob.data;
+}
+
 TEST(SnapshotCreatorUnknownExternalReferences) {
   DisableAlwaysOpt();
   v8::SnapshotCreator creator;