From 1729e3c0ddf0c7a0f912ef38355d38afe284bf04 Mon Sep 17 00:00:00 2001 From: "svenpanne@chromium.org" Date: Wed, 7 Mar 2012 13:24:44 +0000 Subject: [PATCH] Make the runtime entry for setting/changing accessors "atomic". Previously, there were 1 or 2 calls to the runtime when accessors were changed or set. This doesn't really work well with property attributes, leading to some hacks and complicates things even further when trying to share maps in presence of accessors. Therefore, the runtime entry now takes the full triple (getter, setter, attributes), where the getter and/or the setter can be null in case they shouldn't be changed. For now, we do basically the same on the native side as we did before on the JavaScript side, but this will change in future CLs, the current CL is already large enough. Note that object literals with a getter and a setter for the same property still do 2 calls, but this is a little bit more tricky to fix and will be handled in a separate CL. Review URL: https://chromiumcodereview.appspot.com/9616016 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@10956 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/full-codegen-arm.cc | 14 +++-- src/ia32/full-codegen-ia32.cc | 11 ++-- src/messages.js | 5 +- src/mips/full-codegen-mips.cc | 14 +++-- src/objects.h | 2 + src/regexp.js | 81 +++++++++----------------- src/runtime.cc | 77 ++++++++++++++++-------- src/v8natives.js | 15 ++--- src/x64/full-codegen-x64.cc | 11 ++-- test/mjsunit/object-define-property.js | 4 +- 10 files changed, 124 insertions(+), 110 deletions(-) diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index 8639698051..303f0b086e 100644 --- a/src/arm/full-codegen-arm.cc +++ b/src/arm/full-codegen-arm.cc @@ -1498,11 +1498,15 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) { __ ldr(r0, MemOperand(sp)); __ push(r0); VisitForStackValue(key); - __ mov(r1, Operand(property->kind() == ObjectLiteral::Property::SETTER ? - Smi::FromInt(1) : - Smi::FromInt(0))); - __ push(r1); - VisitForStackValue(value); + if (property->kind() == ObjectLiteral::Property::GETTER) { + VisitForStackValue(value); + __ LoadRoot(r1, Heap::kNullValueRootIndex); + __ push(r1); + } else { + __ LoadRoot(r1, Heap::kNullValueRootIndex); + __ push(r1); + VisitForStackValue(value); + } __ mov(r0, Operand(Smi::FromInt(NONE))); __ push(r0); __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5); diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index 86ca138ad2..b55f4280f1 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/src/ia32/full-codegen-ia32.cc @@ -1519,10 +1519,13 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) { case ObjectLiteral::Property::GETTER: __ push(Operand(esp, 0)); // Duplicate receiver. VisitForStackValue(key); - __ push(Immediate(property->kind() == ObjectLiteral::Property::SETTER ? - Smi::FromInt(1) : - Smi::FromInt(0))); - VisitForStackValue(value); + if (property->kind() == ObjectLiteral::Property::GETTER) { + VisitForStackValue(value); + __ push(Immediate(isolate()->factory()->null_value())); + } else { + __ push(Immediate(isolate()->factory()->null_value())); + VisitForStackValue(value); + } __ push(Immediate(Smi::FromInt(NONE))); __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5); break; diff --git a/src/messages.js b/src/messages.js index 0afc0372d9..f39d59c28a 100644 --- a/src/messages.js +++ b/src/messages.js @@ -1,4 +1,4 @@ -// Copyright 2011 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -769,8 +769,7 @@ function DefineOneShotAccessor(obj, name, fun) { hasBeenSet = true; value = v; }; - %DefineOrRedefineAccessorProperty(obj, name, GETTER, getter, DONT_ENUM); - %DefineOrRedefineAccessorProperty(obj, name, SETTER, setter, DONT_ENUM); + %DefineOrRedefineAccessorProperty(obj, name, getter, setter, DONT_ENUM); } function CallSite(receiver, fun, pos) { diff --git a/src/mips/full-codegen-mips.cc b/src/mips/full-codegen-mips.cc index 5559788b36..e259fc4600 100644 --- a/src/mips/full-codegen-mips.cc +++ b/src/mips/full-codegen-mips.cc @@ -1500,11 +1500,15 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) { __ lw(a0, MemOperand(sp)); __ push(a0); VisitForStackValue(key); - __ li(a1, Operand(property->kind() == ObjectLiteral::Property::SETTER ? - Smi::FromInt(1) : - Smi::FromInt(0))); - __ push(a1); - VisitForStackValue(value); + if (property->kind() == ObjectLiteral::Property::GETTER) { + VisitForStackValue(value); + __ LoadRoot(a1, Heap::kNullValueRootIndex); + __ push(a1); + } else { + __ LoadRoot(a1, Heap::kNullValueRootIndex); + __ push(a1); + VisitForStackValue(value); + } __ li(a0, Operand(Smi::FromInt(NONE))); __ push(a0); __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5); diff --git a/src/objects.h b/src/objects.h index 76958c199b..65cd6fb3fc 100644 --- a/src/objects.h +++ b/src/objects.h @@ -854,6 +854,8 @@ class JSReceiver; class Object : public MaybeObject { public: // Type testing. + bool IsObject() { return true; } + #define IS_TYPE_FUNCTION_DECL(type_) inline bool Is##type_(); OBJECT_TYPE_LIST(IS_TYPE_FUNCTION_DECL) HEAP_OBJECT_TYPE_LIST(IS_TYPE_FUNCTION_DECL) diff --git a/src/regexp.js b/src/regexp.js index b724f68183..ace0be1564 100644 --- a/src/regexp.js +++ b/src/regexp.js @@ -1,4 +1,4 @@ -// Copyright 2006-2009 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -421,18 +421,12 @@ function SetUpRegExp() { LAST_INPUT(lastMatchInfo) = ToString(string); }; - %DefineOrRedefineAccessorProperty($RegExp, 'input', GETTER, RegExpGetInput, - DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'input', SETTER, RegExpSetInput, - DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$_', GETTER, RegExpGetInput, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$_', SETTER, RegExpSetInput, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$input', GETTER, RegExpGetInput, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$input', SETTER, RegExpSetInput, - DONT_ENUM | DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, 'input', RegExpGetInput, + RegExpSetInput, DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, '$_', RegExpGetInput, + RegExpSetInput, DONT_ENUM | DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, '$input', RegExpGetInput, + RegExpSetInput, DONT_ENUM | DONT_DELETE); // The properties multiline and $* are aliases for each other. When this // value is set in SpiderMonkey, the value it is set to is coerced to a @@ -446,13 +440,10 @@ function SetUpRegExp() { var RegExpGetMultiline = function() { return multiline; }; var RegExpSetMultiline = function(flag) { multiline = flag ? true : false; }; - %DefineOrRedefineAccessorProperty($RegExp, 'multiline', GETTER, - RegExpGetMultiline, DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'multiline', SETTER, + %DefineOrRedefineAccessorProperty($RegExp, 'multiline', RegExpGetMultiline, RegExpSetMultiline, DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$*', GETTER, RegExpGetMultiline, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$*', SETTER, RegExpSetMultiline, + %DefineOrRedefineAccessorProperty($RegExp, '$*', RegExpGetMultiline, + RegExpSetMultiline, DONT_ENUM | DONT_DELETE); @@ -460,44 +451,28 @@ function SetUpRegExp() { // Static properties set by a successful match. - %DefineOrRedefineAccessorProperty($RegExp, 'lastMatch', GETTER, - RegExpGetLastMatch, DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'lastMatch', SETTER, NoOpSetter, + %DefineOrRedefineAccessorProperty($RegExp, 'lastMatch', RegExpGetLastMatch, + NoOpSetter, DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, '$&', RegExpGetLastMatch, + NoOpSetter, DONT_ENUM | DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, 'lastParen', RegExpGetLastParen, + NoOpSetter, DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, '$+', RegExpGetLastParen, + NoOpSetter, DONT_ENUM | DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, 'leftContext', + RegExpGetLeftContext, NoOpSetter, DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$&', GETTER, RegExpGetLastMatch, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$&', SETTER, NoOpSetter, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'lastParen', GETTER, - RegExpGetLastParen, DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'lastParen', SETTER, NoOpSetter, + %DefineOrRedefineAccessorProperty($RegExp, '$`', RegExpGetLeftContext, + NoOpSetter, DONT_ENUM | DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, 'rightContext', + RegExpGetRightContext, NoOpSetter, DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$+', GETTER, RegExpGetLastParen, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$+', SETTER, NoOpSetter, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'leftContext', GETTER, - RegExpGetLeftContext, DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'leftContext', SETTER, NoOpSetter, - DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$`', GETTER, RegExpGetLeftContext, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$`', SETTER, NoOpSetter, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'rightContext', GETTER, - RegExpGetRightContext, DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, 'rightContext', SETTER, NoOpSetter, - DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, "$'", GETTER, - RegExpGetRightContext, - DONT_ENUM | DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, "$'", SETTER, NoOpSetter, - DONT_ENUM | DONT_DELETE); + %DefineOrRedefineAccessorProperty($RegExp, "$'", RegExpGetRightContext, + NoOpSetter, DONT_ENUM | DONT_DELETE); for (var i = 1; i < 10; ++i) { - %DefineOrRedefineAccessorProperty($RegExp, '$' + i, GETTER, - RegExpMakeCaptureGetter(i), DONT_DELETE); - %DefineOrRedefineAccessorProperty($RegExp, '$' + i, SETTER, NoOpSetter, + %DefineOrRedefineAccessorProperty($RegExp, '$' + i, + RegExpMakeCaptureGetter(i), NoOpSetter, DONT_DELETE); } } diff --git a/src/runtime.cc b/src/runtime.cc index a96152d8d1..9209b9e169 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -995,23 +995,14 @@ enum PropertyDescriptorIndices { DESCRIPTOR_SIZE }; -// Returns an array with the property description: -// if args[1] is not a property on args[0] -// returns undefined -// if args[1] is a data property on args[0] -// [false, value, Writeable, Enumerable, Configurable] -// if args[1] is an accessor on args[0] -// [true, GetFunction, SetFunction, Enumerable, Configurable] -RUNTIME_FUNCTION(MaybeObject*, Runtime_GetOwnProperty) { - ASSERT(args.length() == 2); + +static MaybeObject* GetOwnProperty(Isolate* isolate, + Handle obj, + Handle name) { Heap* heap = isolate->heap(); - HandleScope scope(isolate); Handle elms = isolate->factory()->NewFixedArray(DESCRIPTOR_SIZE); Handle desc = isolate->factory()->NewJSArrayWithElements(elms); LookupResult result(isolate); - CONVERT_ARG_HANDLE_CHECKED(JSObject, obj, 0); - CONVERT_ARG_HANDLE_CHECKED(String, name, 1); - // This could be an element. uint32_t index; if (name->AsArrayIndex(&index)) { @@ -1145,6 +1136,22 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetOwnProperty) { } +// Returns an array with the property description: +// if args[1] is not a property on args[0] +// returns undefined +// if args[1] is a data property on args[0] +// [false, value, Writeable, Enumerable, Configurable] +// if args[1] is an accessor on args[0] +// [true, GetFunction, SetFunction, Enumerable, Configurable] +RUNTIME_FUNCTION(MaybeObject*, Runtime_GetOwnProperty) { + ASSERT(args.length() == 2); + HandleScope scope(isolate); + CONVERT_ARG_HANDLE_CHECKED(JSObject, obj, 0); + CONVERT_ARG_HANDLE_CHECKED(String, name, 1); + return GetOwnProperty(isolate, obj, name); +} + + RUNTIME_FUNCTION(MaybeObject*, Runtime_PreventExtensions) { ASSERT(args.length() == 1); CONVERT_ARG_CHECKED(JSObject, obj, 0); @@ -4307,6 +4314,12 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_KeyedGetProperty) { args.at(1)); } + +static bool IsValidAccessor(Handle obj) { + return obj->IsUndefined() || obj->IsSpecFunction() || obj->IsNull(); +} + + // Implements part of 8.12.9 DefineOwnProperty. // There are 3 cases that lead here: // Step 4b - define a new accessor property. @@ -4317,18 +4330,37 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DefineOrRedefineAccessorProperty) { ASSERT(args.length() == 5); HandleScope scope(isolate); CONVERT_ARG_HANDLE_CHECKED(JSObject, obj, 0); - CONVERT_ARG_CHECKED(String, name, 1); - CONVERT_SMI_ARG_CHECKED(flag, 2); - Object* fun = args[3]; + RUNTIME_ASSERT(!obj->IsNull()); + CONVERT_ARG_HANDLE_CHECKED(String, name, 1); + CONVERT_ARG_HANDLE_CHECKED(Object, getter, 2); + RUNTIME_ASSERT(IsValidAccessor(getter)); + CONVERT_ARG_HANDLE_CHECKED(Object, setter, 3); + RUNTIME_ASSERT(IsValidAccessor(setter)); CONVERT_SMI_ARG_CHECKED(unchecked, 4); - RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0); PropertyAttributes attr = static_cast(unchecked); - RUNTIME_ASSERT(!obj->IsNull()); - RUNTIME_ASSERT(fun->IsSpecFunction() || fun->IsUndefined()); - AccessorComponent component = flag == 0 ? ACCESSOR_GETTER : ACCESSOR_SETTER; - return obj->DefineAccessor(name, component, fun, attr); + // TODO(svenpanne) Define getter/setter/attributes in a single step. + if (getter->IsNull() && setter->IsNull()) { + JSArray* array; + { MaybeObject* maybe_array = GetOwnProperty(isolate, obj, name); + if (!maybe_array->To(&array)) return maybe_array; + } + Object* current = FixedArray::cast(array->elements())->get(GETTER_INDEX); + getter = Handle(current, isolate); + } + if (!getter->IsNull()) { + MaybeObject* ok = + obj->DefineAccessor(*name, ACCESSOR_GETTER, *getter, attr); + if (ok->IsFailure()) return ok; + } + if (!setter->IsNull()) { + MaybeObject* ok = + obj->DefineAccessor(*name, ACCESSOR_SETTER, *setter, attr); + if (ok->IsFailure()) return ok; + } + + return isolate->heap()->undefined_value(); } // Implements part of 8.12.9 DefineOwnProperty. @@ -4342,9 +4374,8 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DefineOrRedefineDataProperty) { HandleScope scope(isolate); CONVERT_ARG_HANDLE_CHECKED(JSObject, js_object, 0); CONVERT_ARG_HANDLE_CHECKED(String, name, 1); - Handle obj_value = args.at(2); + CONVERT_ARG_HANDLE_CHECKED(Object, obj_value, 2); CONVERT_SMI_ARG_CHECKED(unchecked, 3); - RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0); PropertyAttributes attr = static_cast(unchecked); diff --git a/src/v8natives.js b/src/v8natives.js index 8906f9eca2..26724a2419 100644 --- a/src/v8natives.js +++ b/src/v8natives.js @@ -1,4 +1,4 @@ -// Copyright 2011 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -834,10 +834,6 @@ function DefineObjectProperty(obj, p, desc, should_throw) { } %DefineOrRedefineDataProperty(obj, p, value, flag); - } else if (IsGenericDescriptor(desc)) { - // Step 12 - updating an existing accessor property with generic - // descriptor. Changing flags only. - %DefineOrRedefineAccessorProperty(obj, p, GETTER, current.getGet(), flag); } else { // There are 3 cases that lead here: // Step 4b - defining a new accessor property. @@ -845,12 +841,9 @@ function DefineObjectProperty(obj, p, desc, should_throw) { // property. // Step 12 - updating an existing accessor property with an accessor // descriptor. - if (desc.hasGetter()) { - %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag); - } - if (desc.hasSetter()) { - %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag); - } + var getter = desc.hasGetter() ? desc.getGet() : null; + var setter = desc.hasSetter() ? desc.getSet() : null; + %DefineOrRedefineAccessorProperty(obj, p, getter, setter, flag); } return true; } diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index 7a60adc0b3..49adf6af66 100644 --- a/src/x64/full-codegen-x64.cc +++ b/src/x64/full-codegen-x64.cc @@ -1459,10 +1459,13 @@ void FullCodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) { case ObjectLiteral::Property::GETTER: __ push(Operand(rsp, 0)); // Duplicate receiver. VisitForStackValue(key); - __ Push(property->kind() == ObjectLiteral::Property::SETTER ? - Smi::FromInt(1) : - Smi::FromInt(0)); - VisitForStackValue(value); + if (property->kind() == ObjectLiteral::Property::GETTER) { + VisitForStackValue(value); + __ PushRoot(Heap::kNullValueRootIndex); + } else { + __ PushRoot(Heap::kNullValueRootIndex); + VisitForStackValue(value); + } __ Push(Smi::FromInt(NONE)); __ CallRuntime(Runtime::kDefineOrRedefineAccessorProperty, 5); break; diff --git a/test/mjsunit/object-define-property.js b/test/mjsunit/object-define-property.js index 9384a357ba..fdaf82d105 100644 --- a/test/mjsunit/object-define-property.js +++ b/test/mjsunit/object-define-property.js @@ -1,4 +1,4 @@ -// Copyright 2010 the V8 project authors. All rights reserved. +// Copyright 2012 the V8 project authors. All rights reserved. // Redistribution and use in source and binary forms, with or without // modification, are permitted provided that the following conditions are // met: @@ -503,7 +503,7 @@ try { // Defining properties null should fail even when we have // other allowed values try { - %DefineOrRedefineAccessorProperty(null, 'foo', 0, func, 0); + %DefineOrRedefineAccessorProperty(null, 'foo', func, null, 0); } catch (e) { assertTrue(/illegal access/.test(e)); }