Add faster, but unsafe version of LoadInternalField.

src/fast-accessor-assembler.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/fast-accessor-assembler.h"
 
 #include "src/base/logging.h"
 #include "src/code-stub-assembler.h"
 #include "src/code-stubs.h"  // For CallApiCallbackStub.
 #include "src/handles-inl.h"
@@ skipping 25 matching lines @@
   CHECK_EQ(kBuilding, state_);
 
   // For JS functions, the receiver is parameter 0.
   return FromRaw(assembler_->Parameter(0));
 }
 
 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.
   assembler_->Bind(&merge);
   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_);
   return FromRaw(assembler_->LoadBufferObject(FromId(value), offset,
                                               MachineType::IntPtr()));
 }
 
 FastAccessorAssembler::ValueId FastAccessorAssembler::LoadObject(ValueId value,
                                                                  int offset) {
   CHECK_EQ(kBuilding, state_);
@@ skipping 51 matching lines @@
   assembler_->Branch(
       assembler_->WordEqual(FromId(value_id), assembler_->IntPtrConstant(0)),
       FromId(label_id), &pass);
   assembler_->Bind(&pass);
 }
 
 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,
                              ExternalReference::DIRECT_API_CALL, isolate());
 
   // Create & call API callback via stub.
   CallApiCallbackStub stub(isolate(), 1, true, true);
   DCHECK_EQ(5, stub.GetCallInterfaceDescriptor().GetParameterCount());
   DCHECK_EQ(1, stub.GetCallInterfaceDescriptor().GetStackParameterCount());
   // TODO(vogelheim): There is currently no clean way to retrieve the context
@@ skipping 11 matching lines @@
       assembler_->UndefinedConstant(), /* call_data (undefined) */
       assembler_->Parameter(0), /* receiver (same as holder in this case) */
       assembler_->ExternalConstant(callback), /* API callback function */
 
       // JS arguments, on stack:
       FromId(arg));
 
   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();
   state_ = !code.is_null() ? kBuilt : kError;
   Clear();
   return code;
 }
 
 FastAccessorAssembler::ValueId FastAccessorAssembler::FromRaw(Node* node) {
   nodes_.push_back(node);
@@ skipping 23 matching lines @@
 void FastAccessorAssembler::Clear() {
   for (auto label : labels_) {
     delete label;
   }
   nodes_.clear();
   labels_.clear();
 }
 
 }  // namespace internal
 }  // namespace v8