[api] Reland "Assign serial numbers when template infos are added to cache"

Instead of assigning serial numbers when the template infos are
created, this patch creates serial numbers only when they are added to
cache.

This way only the ones that are first instantiated are allocated the
fast template cache. Previously, various accessors and methods that
would almost never get instantiated got assigned to the fast template
cache.

Bug: v8:11284
Change-Id: I8f7578aa0dae48267bbc6303515114eb6e24c1c4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2621081Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#73655}
TBR: ulan@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2825592
Cr-Commit-Position: refs/heads/master@{#73951}

src/api/api-natives.cc

@@ -74,9 +74,9 @@ MaybeHandle<Object> DefineAccessorProperty(Isolate* isolate,
                                            Handle<Object> setter,
                                            PropertyAttributes attributes) {
   DCHECK(!getter->IsFunctionTemplateInfo() ||
-         !FunctionTemplateInfo::cast(*getter).do_not_cache());
+         FunctionTemplateInfo::cast(*getter).should_cache());
   DCHECK(!setter->IsFunctionTemplateInfo() ||
-         !FunctionTemplateInfo::cast(*setter).do_not_cache());
+         FunctionTemplateInfo::cast(*setter).should_cache());
   if (getter->IsFunctionTemplateInfo() &&
       FunctionTemplateInfo::cast(*getter).BreakAtEntry()) {
     ASSIGN_RETURN_ON_EXCEPTION(
@@ -287,16 +287,20 @@ enum class CachingMode { kLimited, kUnlimited };
 MaybeHandle<JSObject> ProbeInstantiationsCache(
     Isolate* isolate, Handle<NativeContext> native_context, int serial_number,
     CachingMode caching_mode) {
-  DCHECK_LE(1, serial_number);
-  if (serial_number <= TemplateInfo::kFastTemplateInstantiationsCacheSize) {
+  DCHECK_NE(serial_number, TemplateInfo::kDoNotCache);
+  if (serial_number == TemplateInfo::kUncached) {
+    return {};
+  }
+
+  if (serial_number < TemplateInfo::kFastTemplateInstantiationsCacheSize) {
     FixedArray fast_cache =
         native_context->fast_template_instantiations_cache();
-    Handle<Object> object{fast_cache.get(serial_number - 1), isolate};
+    Handle<Object> object{fast_cache.get(serial_number), isolate};
     if (object->IsTheHole(isolate)) return {};
     return Handle<JSObject>::cast(object);
   }
   if (caching_mode == CachingMode::kUnlimited ||
-      (serial_number <= TemplateInfo::kSlowTemplateInstantiationsCacheSize)) {
+      (serial_number < TemplateInfo::kSlowTemplateInstantiationsCacheSize)) {
     SimpleNumberDictionary slow_cache =
         native_context->slow_template_instantiations_cache();
     InternalIndex entry = slow_cache.FindEntry(isolate, serial_number);
@@ -309,19 +313,27 @@ MaybeHandle<JSObject> ProbeInstantiationsCache(
 
 void CacheTemplateInstantiation(Isolate* isolate,
                                 Handle<NativeContext> native_context,
-                                int serial_number, CachingMode caching_mode,
+                                Handle<TemplateInfo> data,
+                                CachingMode caching_mode,
                                 Handle<JSObject> object) {
-  DCHECK_LE(1, serial_number);
-  if (serial_number <= TemplateInfo::kFastTemplateInstantiationsCacheSize) {
+  DCHECK_NE(TemplateInfo::kDoNotCache, data->serial_number());
+
+  int serial_number = data->serial_number();
+  if (serial_number == TemplateInfo::kUncached) {
+    serial_number = isolate->heap()->GetNextTemplateSerialNumber();
+  }
+
+  if (serial_number < TemplateInfo::kFastTemplateInstantiationsCacheSize) {
     Handle<FixedArray> fast_cache =
         handle(native_context->fast_template_instantiations_cache(), isolate);
     Handle<FixedArray> new_cache =
-        FixedArray::SetAndGrow(isolate, fast_cache, serial_number - 1, object);
+        FixedArray::SetAndGrow(isolate, fast_cache, serial_number, object);
     if (*new_cache != *fast_cache) {
       native_context->set_fast_template_instantiations_cache(*new_cache);
     }
+    data->set_serial_number(serial_number);
   } else if (caching_mode == CachingMode::kUnlimited ||
-             (serial_number <=
+             (serial_number <
               TemplateInfo::kSlowTemplateInstantiationsCacheSize)) {
     Handle<SimpleNumberDictionary> cache =
         handle(native_context->slow_template_instantiations_cache(), isolate);
@@ -330,20 +342,28 @@ void CacheTemplateInstantiation(Isolate* isolate,
     if (*new_cache != *cache) {
       native_context->set_slow_template_instantiations_cache(*new_cache);
     }
+    data->set_serial_number(serial_number);
+  } else {
+    // we've overflowed the cache limit, no more caching
+    data->set_serial_number(TemplateInfo::kDoNotCache);
   }
 }
 
 void UncacheTemplateInstantiation(Isolate* isolate,
                                   Handle<NativeContext> native_context,
-                                  int serial_number, CachingMode caching_mode) {
-  DCHECK_LE(1, serial_number);
-  if (serial_number <= TemplateInfo::kFastTemplateInstantiationsCacheSize) {
+                                  Handle<TemplateInfo> data,
+                                  CachingMode caching_mode) {
+  int serial_number = data->serial_number();
+  if (serial_number < 0) return;
+
+  if (serial_number < TemplateInfo::kFastTemplateInstantiationsCacheSize) {
     FixedArray fast_cache =
         native_context->fast_template_instantiations_cache();
-    DCHECK(!fast_cache.get(serial_number - 1).IsUndefined(isolate));
-    fast_cache.set_undefined(serial_number - 1);
+    DCHECK(!fast_cache.get(serial_number).IsUndefined(isolate));
+    fast_cache.set_undefined(serial_number);
+    data->set_serial_number(TemplateInfo::kUncached);
   } else if (caching_mode == CachingMode::kUnlimited ||
-             (serial_number <=
+             (serial_number <
               TemplateInfo::kSlowTemplateInstantiationsCacheSize)) {
     Handle<SimpleNumberDictionary> cache =
         handle(native_context->slow_template_instantiations_cache(), isolate);
@@ -351,6 +371,7 @@ void UncacheTemplateInstantiation(Isolate* isolate,
     DCHECK(entry.is_found());
     cache = SimpleNumberDictionary::DeleteEntry(isolate, cache, entry);
     native_context->set_slow_template_instantiations_cache(*cache);
+    data->set_serial_number(TemplateInfo::kUncached);
   }
 }
 
@@ -372,20 +393,20 @@ MaybeHandle<JSObject> InstantiateObject(Isolate* isolate,
                                         bool is_prototype) {
   RCS_SCOPE(isolate, RuntimeCallCounterId::kInstantiateObject);
   Handle<JSFunction> constructor;
-  int serial_number = info->serial_number();
+  bool should_cache = info->should_cache();
   if (!new_target.is_null()) {
     if (IsSimpleInstantiation(isolate, *info, *new_target)) {
       constructor = Handle<JSFunction>::cast(new_target);
     } else {
       // Disable caching for subclass instantiation.
-      serial_number = 0;
+      should_cache = false;
     }
   }
   // Fast path.
   Handle<JSObject> result;
-  if (serial_number) {
+  if (should_cache && info->is_cached()) {
     if (ProbeInstantiationsCache(isolate, isolate->native_context(),
-                                 serial_number, CachingMode::kLimited)
+                                 info->serial_number(), CachingMode::kLimited)
             .ToHandle(&result)) {
       return isolate->factory()->CopyJSObject(result);
     }
@@ -428,9 +449,9 @@ MaybeHandle<JSObject> InstantiateObject(Isolate* isolate,
     // TODO(dcarney): is this necessary?
     JSObject::MigrateSlowToFast(result, 0, "ApiNatives::InstantiateObject");
     // Don't cache prototypes.
-    if (serial_number) {
-      CacheTemplateInstantiation(isolate, isolate->native_context(),
-                                 serial_number, CachingMode::kLimited, result);
+    if (should_cache) {
+      CacheTemplateInstantiation(isolate, isolate->native_context(), info,
+                                 CachingMode::kLimited, result);
       result = isolate->factory()->CopyJSObject(result);
     }
   }
@@ -464,10 +485,10 @@ MaybeHandle<JSFunction> InstantiateFunction(
     Isolate* isolate, Handle<NativeContext> native_context,
     Handle<FunctionTemplateInfo> data, MaybeHandle<Name> maybe_name) {
   RCS_SCOPE(isolate, RuntimeCallCounterId::kInstantiateFunction);
-  int serial_number = data->serial_number();
-  if (serial_number) {
+  bool should_cache = data->should_cache();
+  if (should_cache && data->is_cached()) {
     Handle<JSObject> result;
-    if (ProbeInstantiationsCache(isolate, native_context, serial_number,
+    if (ProbeInstantiationsCache(isolate, native_context, data->serial_number(),
                                  CachingMode::kUnlimited)
             .ToHandle(&result)) {
       return Handle<JSFunction>::cast(result);
@@ -514,18 +535,16 @@ MaybeHandle<JSFunction> InstantiateFunction(
 
   Handle<JSFunction> function = ApiNatives::CreateApiFunction(
       isolate, native_context, data, prototype, function_type, maybe_name);
-  if (serial_number) {
+  if (should_cache) {
     // Cache the function.
-    CacheTemplateInstantiation(isolate, native_context, serial_number,
+    CacheTemplateInstantiation(isolate, native_context, data,
                                CachingMode::kUnlimited, function);
   }
   MaybeHandle<JSObject> result = ConfigureInstance(isolate, function, data);
   if (result.is_null()) {
     // Uncache on error.
-    if (serial_number) {
-      UncacheTemplateInstantiation(isolate, native_context, serial_number,
-                                   CachingMode::kUnlimited);
-    }
+    UncacheTemplateInstantiation(isolate, native_context, data,
+                                 CachingMode::kUnlimited);
     return MaybeHandle<JSFunction>();
   }
   data->set_published(true);

src/api/api.cc

@@ -1093,9 +1093,13 @@ void Context::SetAlignedPointerInEmbedderData(int index, void* value) {
 
 // --- T e m p l a t e ---
 
-static void InitializeTemplate(i::TemplateInfo that, int type) {
+static void InitializeTemplate(i::TemplateInfo that, int type,
+                               bool do_not_cache) {
   that.set_number_of_properties(0);
   that.set_tag(type);
+  int serial_number =
+      do_not_cache ? i::TemplateInfo::kDoNotCache : i::TemplateInfo::kUncached;
+  that.set_serial_number(serial_number);
 }
 
 void Template::Set(v8::Local<Name> name, v8::Local<Data> value,
@@ -1105,15 +1109,18 @@ void Template::Set(v8::Local<Name> name, v8::Local<Data> value,
   ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
   i::HandleScope scope(isolate);
   auto value_obj = Utils::OpenHandle(*value);
+
   Utils::ApiCheck(!value_obj->IsJSReceiver() || value_obj->IsTemplateInfo(),
                   "v8::Template::Set",
                   "Invalid value, must be a primitive or a Template");
+
+  // The template cache only performs shallow clones, if we set an
+  // ObjectTemplate as a property value then we can not cache the receiver
+  // template.
   if (value_obj->IsObjectTemplateInfo()) {
-    templ->set_serial_number(0);
-    if (templ->IsFunctionTemplateInfo()) {
-      i::Handle<i::FunctionTemplateInfo>::cast(templ)->set_do_not_cache(true);
-    }
+    templ->set_serial_number(i::TemplateInfo::kDoNotCache);
   }
+
   i::ApiNatives::AddDataProperty(isolate, templ, Utils::OpenHandle(*name),
                                  value_obj,
                                  static_cast<i::PropertyAttributes>(attribute));
@@ -1145,8 +1152,9 @@ void Template::SetAccessorProperty(v8::Local<v8::Name> name,
 }
 
 // --- F u n c t i o n   T e m p l a t e ---
-static void InitializeFunctionTemplate(i::FunctionTemplateInfo info) {
-  InitializeTemplate(info, Consts::FUNCTION_TEMPLATE);
+static void InitializeFunctionTemplate(i::FunctionTemplateInfo info,
+                                       bool do_not_cache) {
+  InitializeTemplate(info, Consts::FUNCTION_TEMPLATE, do_not_cache);
   info.set_flag(0);
 }
 
@@ -1219,14 +1227,8 @@ static Local<FunctionTemplate> FunctionTemplateNew(
     // Disallow GC until all fields of obj have acceptable types.
     i::DisallowGarbageCollection no_gc;
     i::FunctionTemplateInfo raw = *obj;
-    InitializeFunctionTemplate(raw);
+    InitializeFunctionTemplate(raw, do_not_cache);
     raw.set_length(length);
-    raw.set_do_not_cache(do_not_cache);
-    int next_serial_number = i::FunctionTemplateInfo::kInvalidSerialNumber;
-    if (!do_not_cache) {
-      next_serial_number = isolate->heap()->GetNextTemplateSerialNumber();
-    }
-    raw.set_serial_number(next_serial_number);
     raw.set_undetectable(false);
     raw.set_needs_access_check(false);
     raw.set_accept_any_receiver(true);
@@ -1442,13 +1444,8 @@ static Local<ObjectTemplate> ObjectTemplateNew(
     // Disallow GC until all fields of obj have acceptable types.
     i::DisallowGarbageCollection no_gc;
     i::ObjectTemplateInfo raw = *obj;
-    InitializeTemplate(raw, Consts::OBJECT_TEMPLATE);
+    InitializeTemplate(raw, Consts::OBJECT_TEMPLATE, do_not_cache);
     raw.set_data(0);
-    int next_serial_number = 0;
-    if (!do_not_cache) {
-      next_serial_number = isolate->heap()->GetNextTemplateSerialNumber();
-    }
-    raw.set_serial_number(next_serial_number);
     if (!constructor.IsEmpty()) {
       raw.set_constructor(*Utils::OpenHandle(*constructor));
     }

src/heap/heap-inl.h

@@ -644,8 +644,8 @@ int Heap::NextDebuggingId() {
 }
 
 int Heap::GetNextTemplateSerialNumber() {
-  int next_serial_number = next_template_serial_number().value() + 1;
-  set_next_template_serial_number(Smi::FromInt(next_serial_number));
+  int next_serial_number = next_template_serial_number().value();
+  set_next_template_serial_number(Smi::FromInt(next_serial_number + 1));
   return next_serial_number;
 }
 

src/objects/templates-inl.h

@@ -34,7 +34,6 @@ BOOL_ACCESSORS(FunctionTemplateInfo, flag, read_only_prototype,
                ReadOnlyPrototypeBit::kShift)
 BOOL_ACCESSORS(FunctionTemplateInfo, flag, remove_prototype,
                RemovePrototypeBit::kShift)
-BOOL_ACCESSORS(FunctionTemplateInfo, flag, do_not_cache, DoNotCacheBit::kShift)
 BOOL_ACCESSORS(FunctionTemplateInfo, flag, accept_any_receiver,
                AcceptAnyReceiverBit::kShift)
 BOOL_ACCESSORS(FunctionTemplateInfo, flag, published, PublishedBit::kShift)
@@ -104,6 +103,11 @@ RARE_ACCESSORS(c_function, CFunction, Object, Smi(0))
 RARE_ACCESSORS(c_signature, CSignature, Object, Smi(0))
 #undef RARE_ACCESSORS
 
+bool TemplateInfo::should_cache() const {
+  return serial_number() != kDoNotCache;
+}
+bool TemplateInfo::is_cached() const { return serial_number() > kUncached; }
+
 bool FunctionTemplateInfo::instantiated() {
   return shared_function_info().IsSharedFunctionInfo();
 }

src/objects/templates.h

@@ -27,6 +27,16 @@ class TemplateInfo : public TorqueGeneratedTemplateInfo<TemplateInfo, Struct> {
   // instead of caching them.
   static const int kSlowTemplateInstantiationsCacheSize = 1 * MB;
 
+  // If the serial number is set to kDoNotCache, then we should never cache this
+  // TemplateInfo.
+  static const int kDoNotCache = -1;
+  // If the serial number is set to kUncached, it means that this TemplateInfo
+  // has not been cached yet but it can be.
+  static const int kUncached = -2;
+
+  inline bool should_cache() const;
+  inline bool is_cached() const;
+
   TQ_OBJECT_CONSTRUCTORS(TemplateInfo)
 };
 
@@ -111,10 +121,6 @@ class FunctionTemplateInfo
   // prototype_provoider_template are instantiated.
   DECL_BOOLEAN_ACCESSORS(remove_prototype)
 
-  // If set, do not attach a serial number to this FunctionTemplate and thus do
-  // not keep an instance boilerplate around.
-  DECL_BOOLEAN_ACCESSORS(do_not_cache)
-
   // If not set an access may be performed on calling the associated JSFunction.
   DECL_BOOLEAN_ACCESSORS(accept_any_receiver)
 
@@ -128,8 +134,6 @@ class FunctionTemplateInfo
   // Dispatched behavior.
   DECL_PRINTER(FunctionTemplateInfo)
 
-  static const int kInvalidSerialNumber = 0;
-
   static Handle<SharedFunctionInfo> GetOrCreateSharedFunctionInfo(
       Isolate* isolate, Handle<FunctionTemplateInfo> info,
       MaybeHandle<Name> maybe_name);

src/objects/templates.tq

@@ -33,7 +33,6 @@ bitfield struct FunctionTemplateInfoFlags extends uint31 {
   needs_access_check: bool: 1 bit;
   read_only_prototype: bool: 1 bit;
   remove_prototype: bool: 1 bit;
-  do_not_cache: bool: 1 bit;
   accept_any_receiver: bool: 1 bit;
   published: bool: 1 bit;
 }

test/cctest/test-api.cc

@@ -21979,7 +21979,7 @@ THREADED_TEST(FunctionNew) {
           ->shared()
           .get_api_func_data()
           .serial_number();
-  CHECK_EQ(i::FunctionTemplateInfo::kInvalidSerialNumber, serial_number);
+  CHECK_EQ(i::TemplateInfo::kDoNotCache, serial_number);
 
   // Verify that each Function::New creates a new function instance
   Local<Object> data2 = v8::Object::New(isolate);