Add faster, but unsafe version of LoadInternalField.

LoadInternalField performs some redundant checks in the context of
fast accessors.
This improves the speedup from 20% to 60% for next/previousSibling in
the Dromaeo DOM traversal benchmarks.

BUG=chromium:508898

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

include/v8-experimental.h

@@ -31,6 +31,7 @@ class V8_EXPORT FastAccessorBuilder {
   ValueId IntegerConstant(int int_constant);
   ValueId GetReceiver();
   ValueId LoadInternalField(ValueId value_id, int field_no);
+  ValueId LoadInternalFieldUnchecked(ValueId value_id, int field_no);
   ValueId LoadValue(ValueId value_id, int offset);
   ValueId LoadObject(ValueId value_id, int offset);
   void ReturnValue(ValueId value_id);

src/api-experimental.cc

@@ -76,6 +76,10 @@ FastAccessorBuilder::ValueId FastAccessorBuilder::LoadInternalField(
   return FromApi(this)->LoadInternalField(value, field_no);
 }
 
+FastAccessorBuilder::ValueId FastAccessorBuilder::LoadInternalFieldUnchecked(
+    ValueId value, int field_no) {
+  return FromApi(this)->LoadInternalFieldUnchecked(value, field_no);
+}
 
 FastAccessorBuilder::ValueId FastAccessorBuilder::LoadValue(ValueId value_id,
                                                             int offset) {

src/fast-accessor-assembler.cc

@@ -43,47 +43,23 @@ FastAccessorAssembler::ValueId FastAccessorAssembler::LoadInternalField(
     ValueId value, int field_no) {
   CHECK_EQ(kBuilding, state_);
 
-  // Determine the 'value' object's instance type.
-  Node* object_map = assembler_->LoadObjectField(
-      FromId(value), Internals::kHeapObjectMapOffset, MachineType::Pointer());
-  Node* instance_type = assembler_->WordAnd(
-      assembler_->LoadObjectField(object_map,
-                                  Internals::kMapInstanceTypeAndBitFieldOffset,
-                                  MachineType::Uint16()),
-      assembler_->IntPtrConstant(0xff));
-
-  // Check whether we have a proper JSObject.
   CodeStubAssembler::Variable result(assembler_.get(),
                                      MachineRepresentation::kTagged);
-  CodeStubAssembler::Label is_jsobject(assembler_.get());
-  CodeStubAssembler::Label maybe_api_object(assembler_.get());
-  CodeStubAssembler::Label is_not_jsobject(assembler_.get());
+  LabelId is_not_jsobject = MakeLabel();
   CodeStubAssembler::Label merge(assembler_.get(), &result);
-  assembler_->Branch(
-      assembler_->WordEqual(
-          instance_type, assembler_->IntPtrConstant(Internals::kJSObjectType)),
-      &is_jsobject, &maybe_api_object);
 
-  // JSObject? Then load the internal field field_no.
-  assembler_->Bind(&is_jsobject);
+  CheckIsJSObjectOrJump(value, is_not_jsobject);
+
   Node* internal_field = assembler_->LoadObjectField(
       FromId(value), JSObject::kHeaderSize + kPointerSize * field_no,
       MachineType::Pointer());
+
   result.Bind(internal_field);
   assembler_->Goto(&merge);
 
-  assembler_->Bind(&maybe_api_object);
-  assembler_->Branch(
-      assembler_->WordEqual(instance_type, assembler_->IntPtrConstant(
-                                               Internals::kJSApiObjectType)),
-      &is_jsobject, &is_not_jsobject);
-
-  // No JSObject? Return undefined.
-  // TODO(vogelheim): Check whether this is the appropriate action, or whether
-  //                  the method should take a label instead.
-  assembler_->Bind(&is_not_jsobject);
-  Node* fail_value = assembler_->UndefinedConstant();
-  result.Bind(fail_value);
+  // Return null, mimicking the C++ counterpart.
+  SetLabel(is_not_jsobject);
+  result.Bind(assembler_->NullConstant());
   assembler_->Goto(&merge);
 
   // Return.
@@ -91,6 +67,31 @@ FastAccessorAssembler::ValueId FastAccessorAssembler::LoadInternalField(
   return FromRaw(result.value());
 }
 
+FastAccessorAssembler::ValueId
+FastAccessorAssembler::LoadInternalFieldUnchecked(ValueId value, int field_no) {
+  CHECK_EQ(kBuilding, state_);
+
+  // Defensive debug checks.
+  if (FLAG_debug_code) {
+    LabelId is_jsobject = MakeLabel();
+    LabelId is_not_jsobject = MakeLabel();
+    CheckIsJSObjectOrJump(value, is_not_jsobject);
+    assembler_->Goto(FromId(is_jsobject));
+
+    SetLabel(is_not_jsobject);
+    assembler_->DebugBreak();
+    assembler_->Goto(FromId(is_jsobject));
+
+    SetLabel(is_jsobject);
+  }
+
+  Node* result = assembler_->LoadObjectField(
+      FromId(value), JSObject::kHeaderSize + kPointerSize * field_no,
+      MachineType::Pointer());
+
+  return FromRaw(result);
+}
+
 FastAccessorAssembler::ValueId FastAccessorAssembler::LoadValue(ValueId value,
                                                                 int offset) {
   CHECK_EQ(kBuilding, state_);
@@ -193,6 +194,40 @@ FastAccessorAssembler::ValueId FastAccessorAssembler::Call(
   return FromRaw(call);
 }
 
+void FastAccessorAssembler::CheckIsJSObjectOrJump(ValueId value_id,
+                                                  LabelId label_id) {
+  CHECK_EQ(kBuilding, state_);
+
+  // Determine the 'value' object's instance type.
+  Node* object_map = assembler_->LoadObjectField(
+      FromId(value_id), Internals::kHeapObjectMapOffset,
+      MachineType::Pointer());
+
+  Node* instance_type = assembler_->WordAnd(
+      assembler_->LoadObjectField(object_map,
+                                  Internals::kMapInstanceTypeAndBitFieldOffset,
+                                  MachineType::Uint16()),
+      assembler_->IntPtrConstant(0xff));
+
+  CodeStubAssembler::Label is_jsobject(assembler_.get());
+
+  // Check whether we have a proper JSObject.
+  assembler_->GotoIf(
+      assembler_->WordEqual(
+          instance_type, assembler_->IntPtrConstant(Internals::kJSObjectType)),
+      &is_jsobject);
+
+  // JSApiObject?.
+  assembler_->GotoUnless(
+      assembler_->WordEqual(instance_type, assembler_->IntPtrConstant(
+                                               Internals::kJSApiObjectType)),
+      FromId(label_id));
+
+  // Continue.
+  assembler_->Goto(&is_jsobject);
+  assembler_->Bind(&is_jsobject);
+}
+
 MaybeHandle<Code> FastAccessorAssembler::Build() {
   CHECK_EQ(kBuilding, state_);
   Handle<Code> code = assembler_->GenerateCode();

src/fast-accessor-assembler.h

@@ -54,6 +54,15 @@ class FastAccessorAssembler {
   ValueId IntegerConstant(int int_constant);
   ValueId GetReceiver();
   ValueId LoadInternalField(ValueId value_id, int field_no);
+
+  // Loads internal field and assumes the object is indeed a valid API object
+  // with the proper internal fields present.
+  // The intended use is to call this on an object whose structure has already
+  // been checked previously, e.g. the accessor's receiver, which is map-checked
+  // before the fast accessor is called on it. Using this on an arbitrary object
+  // will result in unsafe memory accesses.
+  ValueId LoadInternalFieldUnchecked(ValueId value_id, int field_no);
+
   ValueId LoadValue(ValueId value_id, int offset);
   ValueId LoadObject(ValueId value_id, int offset);
 
@@ -77,6 +86,8 @@ class FastAccessorAssembler {
   compiler::Node* FromId(ValueId value) const;
   CodeStubAssembler::Label* FromId(LabelId value) const;
 
+  void CheckIsJSObjectOrJump(ValueId value, LabelId label_id);
+
   void Clear();
   Zone* zone() { return &zone_; }
   Isolate* isolate() const { return isolate_; }

test/cctest/test-api-fast-accessor-builder.cc

@@ -113,33 +113,40 @@ TEST(FastAccessor) {
   ExpectInt32("barf()", 124);                 // Call via warmed-up callsite.
 }
 
-
 void AddInternalFieldAccessor(v8::Isolate* isolate,
                               v8::Local<v8::Template> templ, const char* name,
-                              int field_no) {
+                              int field_no, bool useUncheckedLoader) {
   auto builder = v8::experimental::FastAccessorBuilder::New(isolate);
-  builder->ReturnValue(
-      builder->LoadInternalField(builder->GetReceiver(), field_no));
+
+  if (useUncheckedLoader) {
+    builder->ReturnValue(
+        builder->LoadInternalFieldUnchecked(builder->GetReceiver(), field_no));
+  } else {
+    builder->ReturnValue(
+        builder->LoadInternalField(builder->GetReceiver(), field_no));
+  }
+
   templ->SetAccessorProperty(v8_str(name),
                              v8::FunctionTemplate::NewWithFastHandler(
                                  isolate, NativePropertyAccessor, builder));
 }
 
-
-// "Fast" accessor that accesses an internal field.
-TEST(FastAccessorWithInternalField) {
+void checkLoadInternalField(bool useUncheckedLoader, bool emitDebugChecks) {
   // Crankshaft support for fast accessors is not implemented; crankshafted
   // code uses the slow accessor which breaks this test's expectations.
   v8::internal::FLAG_always_opt = false;
+
+  // De/activate debug checks.
+  v8::internal::FLAG_debug_code = emitDebugChecks;
   LocalContext env;
   v8::Isolate* isolate = env->GetIsolate();
   v8::HandleScope scope(isolate);
 
   v8::Local<v8::ObjectTemplate> foo = v8::ObjectTemplate::New(isolate);
   foo->SetInternalFieldCount(3);
-  AddInternalFieldAccessor(isolate, foo, "field0", 0);
-  AddInternalFieldAccessor(isolate, foo, "field1", 1);
-  AddInternalFieldAccessor(isolate, foo, "field2", 2);
+  AddInternalFieldAccessor(isolate, foo, "field0", 0, useUncheckedLoader);
+  AddInternalFieldAccessor(isolate, foo, "field1", 1, useUncheckedLoader);
+  AddInternalFieldAccessor(isolate, foo, "field2", 2, useUncheckedLoader);
 
   // Create an instance w/ 3 internal fields, put in a string, a Smi, nothing.
   v8::Local<v8::Object> obj = foo->NewInstance(env.local()).ToLocalChecked();
@@ -158,6 +165,15 @@ TEST(FastAccessorWithInternalField) {
   ExpectUndefined("field2()");
 }
 
+// "Fast" accessor that accesses an internal field.
+TEST(FastAccessorWithInternalField) { checkLoadInternalField(false, false); }
+
+// "Fast" accessor that accesses an internal field using the fast(er)
+// implementation of LoadInternalField.
+TEST(FastAccessorLoadInternalFieldUnchecked) {
+  checkLoadInternalField(true, false);
+  checkLoadInternalField(true, true);
+}
 
 // "Fast" accessor with control flow via ...OrReturnNull methods.
 TEST(FastAccessorOrReturnNull) {