Refactor the check for generating inline constructors

Moved all the logic to a function on SharedFunctionInfo (including the flag check) to make things more readable.

Changed the check for setter to do a lookup for a named setter for each of the properties assigned in the constructor.

Added tests using accessors and interseptors set through the API.

Added fast case objects to the mjsunit test.

TEST=test/mjsunit/setter-on-constructor-prototype.js
TEST=test/cctest/test-api/SetterOnConstructorPrototype
TEST=test/cctest/test-api/InterceptorOnConstructorPrototype
Review URL: http://codereview.chromium.org/619006

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@3893 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/heap.cc

@@ -2389,10 +2389,7 @@ Object* Heap::AllocateInitialMap(JSFunction* fun) {
   // the inline_new flag so we only change the map if we generate a
   // specialized construct stub.
   ASSERT(in_object_properties <= Map::kMaxPreAllocatedPropertyFields);
-  if (FLAG_inline_new
-      && fun->shared()->has_only_simple_this_property_assignments()
-      && fun->shared()->this_property_assignments_count() > 0
-      && !JSObject::cast(prototype)->HasSetter()) {
+  if (fun->shared()->CanGenerateInlineConstructor(prototype)) {
     int count = fun->shared()->this_property_assignments_count();
     if (count > in_object_properties) {
       count = in_object_properties;

src/objects.cc

@@ -4825,6 +4825,46 @@ int SharedFunctionInfo::CalculateInObjectProperties() {
 }
 
 
+bool SharedFunctionInfo::CanGenerateInlineConstructor(Object* prototype) {
+  // Check the basic conditions for generating inline constructor code.
+  if (!FLAG_inline_new
+      || !has_only_simple_this_property_assignments()
+      || !this_property_assignments_count() > 0) {
+    return false;
+  }
+
+  // If the prototype is null inline constructors cause no problems.
+  if (!prototype->IsJSObject()) {
+    ASSERT(prototype->IsNull());
+    return true;
+  }
+
+  // Traverse the proposed prototype chain looking for setters for properties of
+  // the same names as are set by the inline constructor..
+  for (Object* obj = prototype;
+       obj != Heap::null_value();
+       obj = obj->GetPrototype()) {
+    JSObject* js_object = JSObject::cast(obj);
+    if (!js_object->HasFastProperties()) {
+      // Only allow fast case objects, as the map check in the inline
+      // constructor which check for changes to the prototype chain cannot
+      // handle dictionary case objects.
+      return false;
+    }
+    for (int i = 0; i < this_property_assignments_count(); i++) {
+      LookupResult result;
+      String* name = GetThisPropertyAssignmentName(i);
+      js_object->LocalLookupRealNamedProperty(name, &result);
+      if (result.IsValid() && result.type() == CALLBACKS) {
+        return false;
+      }
+    }
+  }
+
+  return true;
+}
+
+
 void SharedFunctionInfo::SetThisPropertyAssignmentsInfo(
     bool only_simple_this_property_assignments,
     FixedArray* assignments) {
@@ -4880,7 +4920,6 @@ Object* SharedFunctionInfo::GetThisPropertyAssignmentConstant(int index) {
 }
 
 
-
 // Support function for printing the source code to a StringStream
 // without any allocation in the heap.
 void SharedFunctionInfo::SourceCodePrint(StringStream* accumulator,
@@ -7235,61 +7274,6 @@ Object* JSObject::PrepareElementsForSort(uint32_t limit) {
 }
 
 
-static bool CallbacksObjectHasSetter(Object* callbacks) {
-  if (!callbacks->IsFixedArray()) {
-    ASSERT(callbacks->IsAccessorInfo() || callbacks->IsProxy());
-    return true;
-  } else {
-    Object* setter = (FixedArray::cast(callbacks))->get(kSetterIndex);
-    if (setter->IsJSFunction()) {
-      return true;
-    }
-  }
-
-  return false;
-}
-
-
-bool JSObject::HasSetter() {
-  for (Object* obj = this;
-       obj != Heap::null_value();
-       obj = JSObject::cast(obj)->GetPrototype()) {
-    JSObject* js_object = JSObject::cast(obj);
-    if (js_object->HasFastProperties()) {
-      DescriptorArray* descs = js_object->map()->instance_descriptors();
-      for (int i = 0; i < descs->number_of_descriptors(); i++) {
-        PropertyDetails details = descs->GetDetails(i);
-        if (details.type() == CALLBACKS) {
-          Object* callbacks = descs->GetCallbacksObject(i);
-          if (CallbacksObjectHasSetter(callbacks)) {
-            return true;
-          }
-        }
-      }
-    } else {
-      StringDictionary* dict = js_object->property_dictionary();
-      int capacity = dict->Capacity();
-      for (int i = 0; i < capacity; i++) {
-        Object* k = dict->KeyAt(i);
-        if (dict->IsKey(k)) {
-          PropertyType type = dict->DetailsAt(i).type();
-          ASSERT(type != FIELD);
-          if (type == CALLBACKS) {
-            Object* callbacks = dict->ValueAt(i);
-            if (CallbacksObjectHasSetter(callbacks)) {
-              return true;
-            }
-          }
-        }
-      }
-    }
-  }
-
-  return false;
-}
-
-
-
 Object* PixelArray::SetValue(uint32_t index, Object* value) {
   uint8_t clamped_value = 0;
   if (index < static_cast<uint32_t>(length())) {

src/objects.h

@@ -1540,8 +1540,6 @@ class JSObject: public HeapObject {
 
   Object* GetElementWithInterceptor(JSObject* receiver, uint32_t index);
 
-  bool HasSetter();
-
  private:
   Object* SetElementWithInterceptor(uint32_t index, Object* value);
   Object* SetElementWithoutInterceptor(uint32_t index, Object* value);
@@ -3236,6 +3234,10 @@ class SharedFunctionInfo: public HeapObject {
   inline bool try_full_codegen();
   inline void set_try_full_codegen(bool flag);
 
+  // Check whether a inlined constructor can be generated with the given
+  // prototype.
+  bool CanGenerateInlineConstructor(Object* prototype);
+
   // For functions which only contains this property assignments this provides
   // access to the names for the properties assigned.
   DECL_ACCESSORS(this_property_assignments, Object)

src/runtime.cc

@@ -4810,10 +4810,11 @@ static Object* Runtime_NewClosure(Arguments args) {
 
 
 static Code* ComputeConstructStub(Handle<JSFunction> function) {
-  if (FLAG_inline_new
-      && function->shared()->has_only_simple_this_property_assignments()
-      && (!function->has_instance_prototype()
-          || !JSObject::cast(function->instance_prototype())->HasSetter())) {
+  Handle<Object> prototype = Factory::null_value();
+  if (function->has_instance_prototype()) {
+    prototype = Handle<Object>(function->instance_prototype());
+  }
+  if (function->shared()->CanGenerateInlineConstructor(*prototype)) {
     ConstructStubCompiler compiler;
     Object* code = compiler.CompileConstructStub(function->shared());
     if (code->IsFailure()) {

test/cctest/test-api.cc

@@ -9599,3 +9599,106 @@ THREADED_TEST(ScriptLineNumber) {
   CHECK_EQ(0, f->GetScriptLineNumber());
   CHECK_EQ(2, g->GetScriptLineNumber());
 }
+
+
+static v8::Handle<Value> GetterWhichReturns42(Local<String> name,
+                                              const AccessorInfo& info) {
+  return v8_num(42);
+}
+
+
+static void SetterWhichSetsYOnThisTo23(Local<String> name,
+                                       Local<Value> value,
+                                       const AccessorInfo& info) {
+  info.This()->Set(v8_str("y"), v8_num(23));
+}
+
+
+THREADED_TEST(SetterOnConstructorPrototype) {
+  v8::HandleScope scope;
+  Local<ObjectTemplate> templ = ObjectTemplate::New();
+  templ->SetAccessor(v8_str("x"),
+                     GetterWhichReturns42,
+                     SetterWhichSetsYOnThisTo23);
+  LocalContext context;
+  context->Global()->Set(v8_str("P"), templ->NewInstance());
+  CompileRun("function C1() {"
+             "  this.x = 23;"
+             "};"
+             "C1.prototype = P;"
+             "function C2() {"
+             "  this.x = 23"
+             "};"
+             "C2.prototype = { };"
+             "C2.prototype.__proto__ = P;"
+             ""
+             ""
+             "");
+
+  v8::Local<v8::Script> script;
+  script = v8::Script::Compile(v8_str("new C1();"));
+  for (int i = 0; i < 10; i++) {
+    v8::Handle<v8::Object> c1 = v8::Handle<v8::Object>::Cast(script->Run());
+    CHECK_EQ(42, c1->Get(v8_str("x"))->Int32Value());
+    CHECK_EQ(23, c1->Get(v8_str("y"))->Int32Value());
+  }
+
+  script = v8::Script::Compile(v8_str("new C2();"));
+  for (int i = 0; i < 10; i++) {
+    v8::Handle<v8::Object> c2 = v8::Handle<v8::Object>::Cast(script->Run());
+    CHECK_EQ(42, c2->Get(v8_str("x"))->Int32Value());
+    CHECK_EQ(23, c2->Get(v8_str("y"))->Int32Value());
+  }
+}
+
+
+static v8::Handle<Value> NamedPropertyGetterWhichReturns42(
+    Local<String> name, const AccessorInfo& info) {
+  return v8_num(42);
+}
+
+
+static v8::Handle<Value> NamedPropertySetterWhichSetsYOnThisTo23(
+    Local<String> name, Local<Value> value, const AccessorInfo& info) {
+  if (name->Equals(v8_str("x"))) {
+    info.This()->Set(v8_str("y"), v8_num(23));
+  }
+  return v8::Handle<Value>();
+}
+
+
+THREADED_TEST(InterceptorOnConstructorPrototype) {
+  v8::HandleScope scope;
+  Local<ObjectTemplate> templ = ObjectTemplate::New();
+  templ->SetNamedPropertyHandler(NamedPropertyGetterWhichReturns42,
+                                 NamedPropertySetterWhichSetsYOnThisTo23);
+  LocalContext context;
+  context->Global()->Set(v8_str("P"), templ->NewInstance());
+  CompileRun("function C1() {"
+             "  this.x = 23;"
+             "};"
+             "C1.prototype = P;"
+             "function C2() {"
+             "  this.x = 23"
+             "};"
+             "C2.prototype = { };"
+             "C2.prototype.__proto__ = P;"
+             ""
+             ""
+             "");
+
+  v8::Local<v8::Script> script;
+  script = v8::Script::Compile(v8_str("new C1();"));
+  for (int i = 0; i < 10; i++) {
+    v8::Handle<v8::Object> c1 = v8::Handle<v8::Object>::Cast(script->Run());
+    CHECK_EQ(23, c1->Get(v8_str("x"))->Int32Value());
+    CHECK_EQ(42, c1->Get(v8_str("y"))->Int32Value());
+  }
+
+  script = v8::Script::Compile(v8_str("new C2();"));
+  for (int i = 0; i < 10; i++) {
+    v8::Handle<v8::Object> c2 = v8::Handle<v8::Object>::Cast(script->Run());
+    CHECK_EQ(23, c2->Get(v8_str("x"))->Int32Value());
+    CHECK_EQ(42, c2->Get(v8_str("y"))->Int32Value());
+  }
+}

test/mjsunit/setter-on-constructor-prototype.js

@@ -25,66 +25,87 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
+// Flags: --allow-natives-syntax
 
-function C1() {
-  this.x = 23;
+function RunTest(ensure_fast_case) {
+  function C1() {
+    this.x = 23;
+  };
+  C1.prototype = { set x(value) { this.y = 23; } };
+  if (ensure_fast_case) {
+    %ToFastProperties(C1.prototype);
+  }
+  
+  for (var i = 0; i < 10; i++) {
+    var c1 = new C1();
+    assertEquals("undefined", typeof c1.x);
+    assertEquals(23, c1.y);
+  }
+  
+  
+  function C2() {
+    this.x = 23;
+  };
+  C2.prototype = { };
+  C2.prototype.__proto__ = { set x(value) { this.y = 23; } };
+  if (ensure_fast_case) {
+    %ToFastProperties(C2.prototype.__proto__)
+  }
+  
+  for (var i = 0; i < 10; i++) {
+    var c2 = new C2();
+    assertEquals("undefined", typeof c2.x);
+    assertEquals(23, c2.y);
+  }
+  
+  
+  function C3() {
+    this.x = 23;
+  };
+  C3.prototype = { };
+  C3.prototype.__defineSetter__('x', function(value) { this.y = 23; });
+  if (ensure_fast_case) {
+    %ToFastProperties(C3.prototype);
+  }
+  
+  for (var i = 0; i < 10; i++) {
+    var c3 = new C3();
+    assertEquals("undefined", typeof c3.x);
+    assertEquals(23, c3.y);
+  }
+  
+  
+  function C4() {
+    this.x = 23;
+  };
+  C4.prototype = { };
+  C4.prototype.__proto__ = {  };
+  C4.prototype.__proto__.__defineSetter__('x', function(value) { this.y = 23; });
+  if (ensure_fast_case) {
+    %ToFastProperties(C4.prototype.__proto__);
+  }
+  
+  for (var i = 0; i < 10; i++) {
+    var c4 = new C4();
+    assertEquals("undefined", typeof c4.x);
+    assertEquals(23, c4.y);
+  }
+  
+  
+  function D() {
+    this.x = 23;
+  };
+  D.prototype = 1;
+  if (ensure_fast_case) {
+    %ToFastProperties(D.prototype);
+  }
+  
+  for (var i = 0; i < 10; i++) {
+    var d = new D();
+    assertEquals(23, d.x);
+    assertEquals("undefined", typeof d.y);
+  }
 }
-C1.prototype = { set x(value) { this.y = 23; } }
 
-for (var i = 0; i < 10; i++) {
-  var c1 = new C1();
-  assertEquals("undefined", typeof c1.x);
-  assertEquals(23, c1.y);
-}
-
-
-function C2() {
-  this.x = 23;
-}
-C2.prototype = { }
-C2.prototype.__proto__ = { set x(value) { this.y = 23; } }
-
-for (var i = 0; i < 10; i++) {
-  var c2 = new C2();
-  assertEquals("undefined", typeof c2.x);
-  assertEquals(23, c2.y);
-}
-
-
-function C3() {
-  this.x = 23;
-}
-C3.prototype = { }
-C3.prototype.__defineSetter__('x', function(value) { this.y = 23; })
-
-for (var i = 0; i < 10; i++) {
-  var c3 = new C3();
-  assertEquals("undefined", typeof c3.x);
-  assertEquals(23, c3.y);
-}
-
-
-function C4() {
-  this.x = 23;
-}
-C4.prototype = { }
-C4.prototype.__proto__ = {  }
-C4.prototype.__proto__.__defineSetter__('x', function(value) { this.y = 23; })
-
-for (var i = 0; i < 10; i++) {
-  var c4 = new C4();
-  assertEquals("undefined", typeof c4.x);
-  assertEquals(23, c4.y);
-}
-
-
-function D() {
-  this.x = 23;
-}
-D.prototype = 1;
-
-for (var i = 0; i < 10; i++) {
-  var d = new D();
-  assertEquals(23, d.x);
-  assertEquals("undefined", typeof d.y);
-}
+RunTest(false);
+//RunTest(true);