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

This reverts commit db16dce2.

Reason for revert:
https://ci.chromium.org/p/v8/builders/ci/V8%20Blink%20Linux%20Debug/8771

Original change's description:
> [api] 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: I6b633e56e59cbfc3fa5d4ee2db53ca2849eecdd7
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2621081
> Reviewed-by: Camillo Bruni <cbruni@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#73655}

Bug: v8:11284
Change-Id: I382915b2c1be1d87d7a7a961d13e1dd5e3951a4f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2786844
Auto-Submit: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#73659}

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).should_cache());
+         !FunctionTemplateInfo::cast(*getter).do_not_cache());
   DCHECK(!setter->IsFunctionTemplateInfo() ||
-         FunctionTemplateInfo::cast(*setter).should_cache());
+         !FunctionTemplateInfo::cast(*setter).do_not_cache());
   if (getter->IsFunctionTemplateInfo() &&
       FunctionTemplateInfo::cast(*getter).BreakAtEntry()) {
     ASSIGN_RETURN_ON_EXCEPTION(
@@ -288,15 +288,11 @@ enum class CachingMode { kLimited, kUnlimited };
 MaybeHandle<JSObject> ProbeInstantiationsCache(
     Isolate* isolate, Handle<NativeContext> native_context, int serial_number,
     CachingMode caching_mode) {
-  DCHECK_NE(serial_number, TemplateInfo::kDoNotCache);
-  if (serial_number == TemplateInfo::kUncached) {
-    return {};
-  }
-
+  DCHECK_LE(1, serial_number);
   if (serial_number <= TemplateInfo::kFastTemplateInstantiationsCacheSize) {
     FixedArray fast_cache =
         native_context->fast_template_instantiations_cache();
-    Handle<Object> object{fast_cache.get(serial_number), isolate};
+    Handle<Object> object{fast_cache.get(serial_number - 1), isolate};
     if (object->IsTheHole(isolate)) return {};
     return Handle<JSObject>::cast(object);
   }
@@ -314,25 +310,17 @@ MaybeHandle<JSObject> ProbeInstantiationsCache(
 
 void CacheTemplateInstantiation(Isolate* isolate,
                                 Handle<NativeContext> native_context,
-                                Handle<TemplateInfo> data,
-                                CachingMode caching_mode,
+                                int serial_number, CachingMode caching_mode,
                                 Handle<JSObject> object) {
-  DCHECK_NE(TemplateInfo::kDoNotCache, data->serial_number());
-
-  int serial_number = data->serial_number();
-  if (serial_number == TemplateInfo::kUncached) {
-    serial_number = isolate->heap()->GetNextTemplateSerialNumber();
-  }
-
+  DCHECK_LE(1, serial_number);
   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, object);
+        FixedArray::SetAndGrow(isolate, fast_cache, serial_number - 1, 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 <=
               TemplateInfo::kSlowTemplateInstantiationsCacheSize)) {
@@ -343,26 +331,18 @@ 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,
-                                  Handle<TemplateInfo> data,
-                                  CachingMode caching_mode) {
-  int serial_number = data->serial_number();
-  if (serial_number < 0) return;
-
+                                  int serial_number, CachingMode caching_mode) {
+  DCHECK_LE(1, serial_number);
   if (serial_number <= TemplateInfo::kFastTemplateInstantiationsCacheSize) {
     FixedArray fast_cache =
         native_context->fast_template_instantiations_cache();
-    DCHECK(!fast_cache.get(serial_number).IsUndefined(isolate));
-    fast_cache.set_undefined(serial_number);
-    data->set_serial_number(TemplateInfo::kUncached);
+    DCHECK(!fast_cache.get(serial_number - 1).IsUndefined(isolate));
+    fast_cache.set_undefined(serial_number - 1);
   } else if (caching_mode == CachingMode::kUnlimited ||
              (serial_number <=
               TemplateInfo::kSlowTemplateInstantiationsCacheSize)) {
@@ -372,7 +352,6 @@ 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);
   }
 }
 
@@ -395,20 +374,20 @@ MaybeHandle<JSObject> InstantiateObject(Isolate* isolate,
   RuntimeCallTimerScope timer(isolate,
                               RuntimeCallCounterId::kInstantiateObject);
   Handle<JSFunction> constructor;
-  bool should_cache = info->should_cache();
+  int serial_number = info->serial_number();
   if (!new_target.is_null()) {
     if (IsSimpleInstantiation(isolate, *info, *new_target)) {
       constructor = Handle<JSFunction>::cast(new_target);
     } else {
       // Disable caching for subclass instantiation.
-      should_cache = false;
+      serial_number = 0;
     }
   }
   // Fast path.
   Handle<JSObject> result;
-  if (should_cache && info->is_cached()) {
+  if (serial_number) {
     if (ProbeInstantiationsCache(isolate, isolate->native_context(),
-                                 info->serial_number(), CachingMode::kLimited)
+                                 serial_number, CachingMode::kLimited)
             .ToHandle(&result)) {
       return isolate->factory()->CopyJSObject(result);
     }
@@ -451,9 +430,9 @@ MaybeHandle<JSObject> InstantiateObject(Isolate* isolate,
     // TODO(dcarney): is this necessary?
     JSObject::MigrateSlowToFast(result, 0, "ApiNatives::InstantiateObject");
     // Don't cache prototypes.
-    if (should_cache) {
-      CacheTemplateInstantiation(isolate, isolate->native_context(), info,
-                                 CachingMode::kLimited, result);
+    if (serial_number) {
+      CacheTemplateInstantiation(isolate, isolate->native_context(),
+                                 serial_number, CachingMode::kLimited, result);
       result = isolate->factory()->CopyJSObject(result);
     }
   }
@@ -488,10 +467,10 @@ MaybeHandle<JSFunction> InstantiateFunction(
     Handle<FunctionTemplateInfo> data, MaybeHandle<Name> maybe_name) {
   RuntimeCallTimerScope timer(isolate,
                               RuntimeCallCounterId::kInstantiateFunction);
-  bool should_cache = data->should_cache();
-  if (should_cache && data->is_cached()) {
+  int serial_number = data->serial_number();
+  if (serial_number) {
     Handle<JSObject> result;
-    if (ProbeInstantiationsCache(isolate, native_context, data->serial_number(),
+    if (ProbeInstantiationsCache(isolate, native_context, serial_number,
                                  CachingMode::kUnlimited)
             .ToHandle(&result)) {
       return Handle<JSFunction>::cast(result);
@@ -538,16 +517,18 @@ MaybeHandle<JSFunction> InstantiateFunction(
 
   Handle<JSFunction> function = ApiNatives::CreateApiFunction(
       isolate, native_context, data, prototype, function_type, maybe_name);
-  if (should_cache) {
+  if (serial_number) {
     // Cache the function.
-    CacheTemplateInstantiation(isolate, native_context, data,
+    CacheTemplateInstantiation(isolate, native_context, serial_number,
                                CachingMode::kUnlimited, function);
   }
   MaybeHandle<JSObject> result = ConfigureInstance(isolate, function, data);
   if (result.is_null()) {
     // Uncache on error.
-    UncacheTemplateInstantiation(isolate, native_context, data,
-                                 CachingMode::kUnlimited);
+    if (serial_number) {
+      UncacheTemplateInstantiation(isolate, native_context, serial_number,
+                                   CachingMode::kUnlimited);
+    }
     return MaybeHandle<JSFunction>();
   }
   data->set_published(true);

src/api/api.cc

@@ -1093,13 +1093,9 @@ void Context::SetAlignedPointerInEmbedderData(int index, void* value) {
 
 // --- T e m p l a t e ---
 
-static void InitializeTemplate(i::TemplateInfo that, int type,
-                               bool do_not_cache) {
+static void InitializeTemplate(i::TemplateInfo that, int type) {
   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,
@@ -1109,18 +1105,15 @@ 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(i::TemplateInfo::kDoNotCache);
+    templ->set_serial_number(0);
+    if (templ->IsFunctionTemplateInfo()) {
+      i::Handle<i::FunctionTemplateInfo>::cast(templ)->set_do_not_cache(true);
+    }
   }
-
   i::ApiNatives::AddDataProperty(isolate, templ, Utils::OpenHandle(*name),
                                  value_obj,
                                  static_cast<i::PropertyAttributes>(attribute));
@@ -1152,9 +1145,8 @@ 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,
-                                       bool do_not_cache) {
-  InitializeTemplate(info, Consts::FUNCTION_TEMPLATE, do_not_cache);
+static void InitializeFunctionTemplate(i::FunctionTemplateInfo info) {
+  InitializeTemplate(info, Consts::FUNCTION_TEMPLATE);
   info.set_flag(0);
 }
 
@@ -1227,8 +1219,14 @@ static Local<FunctionTemplate> FunctionTemplateNew(
     // Disallow GC until all fields of obj have acceptable types.
     i::DisallowGarbageCollection no_gc;
     i::FunctionTemplateInfo raw = *obj;
-    InitializeFunctionTemplate(raw, do_not_cache);
+    InitializeFunctionTemplate(raw);
     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);
@@ -1444,8 +1442,13 @@ 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, do_not_cache);
+    InitializeTemplate(raw, Consts::OBJECT_TEMPLATE);
     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();
-  set_next_template_serial_number(Smi::FromInt(next_serial_number + 1));
+  int next_serial_number = next_template_serial_number().value() + 1;
+  set_next_template_serial_number(Smi::FromInt(next_serial_number));
   return next_serial_number;
 }
 

src/objects/templates-inl.h

@@ -34,6 +34,7 @@ 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)
@@ -103,11 +104,6 @@ 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,16 +27,6 @@ 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)
 };
 
@@ -121,6 +111,10 @@ 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)
 
@@ -134,6 +128,8 @@ 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,6 +33,7 @@ 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

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