Add faster, but unsafe version of LoadInternalField.

src/fast-accessor-assembler.cc

Index: src/fast-accessor-assembler.cc
diff --git a/src/fast-accessor-assembler.cc b/src/fast-accessor-assembler.cc
index a261eb01a63a5edfbd3a311b963cac30b1bc464d..41ceed8acbaf1d44a34e4e7e01e9091a412d5778 100644
--- a/src/fast-accessor-assembler.cc
+++ b/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,32 @@ 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);
+    // Deliberately crash.

[vogelheim, 2016/08/08 11:47:22]

Hmm. Actually, we force a debug break (which terminates the program if there's
no debugger attached). So I thought change the comment to, "// Force debug
break." But then, that's exactly what the code actually says, so maybe just drop
the comment.

[Alfonso, 2016/08/08 13:03:28]

Acknowledged.

+    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_);
@@ -162,6 +164,11 @@ FastAccessorAssembler::ValueId FastAccessorAssembler::Call(
     FunctionCallback callback_function, ValueId arg) {
   CHECK_EQ(kBuilding, state_);
 
+  CodeStubAssembler::Label deferred(assembler_.get(),

[vogelheim, 2016/08/08 11:47:22]

This is a good change, but unrelated to what the CL description says. In this
case, I'd either make it a separate CL or, if that's burdensome, just add a
1-line comment to the CL description. (E.g.: "Also use deferred label for
function callbacks, to prevent a stack trace being built in cases where it's not
necessary.")

[Alfonso, 2016/08/08 13:03:28]

Reverting this, even if is the logical thing to do, there's no real gain (in the
benchmarks). I tried 'deferring' some unlikely paths and performance actually
regressed. The benchmarks are slightly biased towards a specific behavior, so
I'd like collect more data in order to have a solid argument to support this
optimization (or not).

+                                    compiler::CodeAssembler::Label::kDeferred);
+  assembler_->Goto(&deferred);
+  assembler_->Bind(&deferred);
+
   // Wrap the FunctionCallback in an ExternalReference.
   ApiFunction callback_api_function(FUNCTION_ADDR(callback_function));
   ExternalReference callback(&callback_api_function,
@@ -193,6 +200,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();