Revision: 5591
Author: [email protected]
Date: Tue Oct 5 06:10:43 2010
Log: Do not shortcut union of keys if lhs is empty.
The problem is other array may have holes, for example
when fixed array comes from JSArray (in case of named interceptor).
If that would prove to be a performance problem, we could
pass an additional argument into UnionOfKeys to hold actual length.
Review URL: http://codereview.chromium.org/3595013
http://code.google.com/p/v8/source/detail?r=5591
Modified:
/branches/bleeding_edge/src/handles.cc
/branches/bleeding_edge/src/objects.cc
/branches/bleeding_edge/test/cctest/test-api.cc
=======================================
--- /branches/bleeding_edge/src/handles.cc Mon Oct 4 05:54:31 2010
+++ /branches/bleeding_edge/src/handles.cc Tue Oct 5 06:10:43 2010
@@ -633,10 +633,21 @@
}
return result;
}
+
+
+static bool ContainsOnlyValidKeys(Handle<FixedArray> array) {
+ int len = array->length();
+ for (int i = 0; i < len; i++) {
+ Object* e = array->get(i);
+ if (!(e->IsString() || e->IsNumber())) return false;
+ }
+ return true;
+}
Handle<FixedArray> GetKeysInFixedArrayFor(Handle<JSObject> object,
KeyCollectionType type) {
+ USE(ContainsOnlyValidKeys);
Handle<FixedArray> content = Factory::empty_fixed_array();
Handle<JSObject> arguments_boilerplate =
Handle<JSObject>(
@@ -664,6 +675,7 @@
Factory::NewFixedArray(current->NumberOfEnumElements());
current->GetEnumElementKeys(*element_keys);
content = UnionOfKeys(content, element_keys);
+ ASSERT(ContainsOnlyValidKeys(content));
// Add the element keys from the interceptor.
if (current->HasIndexedInterceptor()) {
@@ -671,6 +683,7 @@
GetKeysForIndexedInterceptor(object, current);
if (!result.IsEmpty())
content = AddKeysFromJSArray(content,
v8::Utils::OpenHandle(*result));
+ ASSERT(ContainsOnlyValidKeys(content));
}
// We can cache the computed property keys if access checks are
@@ -692,6 +705,7 @@
// Compute the property keys and cache them if possible.
content =
UnionOfKeys(content, GetEnumPropertyKeys(current,
cache_enum_keys));
+ ASSERT(ContainsOnlyValidKeys(content));
// Add the property keys from the interceptor.
if (current->HasNamedInterceptor()) {
@@ -699,6 +713,7 @@
GetKeysForNamedInterceptor(object, current);
if (!result.IsEmpty())
content = AddKeysFromJSArray(content,
v8::Utils::OpenHandle(*result));
+ ASSERT(ContainsOnlyValidKeys(content));
}
// If we only want local properties we bail out after the first
=======================================
--- /branches/bleeding_edge/src/objects.cc Mon Oct 4 07:30:43 2010
+++ /branches/bleeding_edge/src/objects.cc Tue Oct 5 06:10:43 2010
@@ -3601,9 +3601,17 @@
Object* FixedArray::UnionOfKeys(FixedArray* other) {
int len0 = length();
+#ifdef DEBUG
+ if (FLAG_enable_slow_asserts) {
+ for (int i = 0; i < len0; i++) {
+ ASSERT(get(i)->IsString() || get(i)->IsNumber());
+ }
+ }
+#endif
int len1 = other->length();
- // Optimize if either is empty.
- if (len0 == 0) return other;
+ // Optimize if 'other' is empty.
+ // We cannot optimize if 'this' is empty, as other may have holes
+ // or non keys.
if (len1 == 0) return this;
// Compute how many elements are not in this.
@@ -3623,14 +3631,18 @@
FixedArray* result = FixedArray::cast(obj);
WriteBarrierMode mode = result->GetWriteBarrierMode(no_gc);
for (int i = 0; i < len0; i++) {
- result->set(i, get(i), mode);
+ Object* e = get(i);
+ ASSERT(e->IsString() || e->IsNumber());
+ result->set(i, e, mode);
}
// Fill in the extra keys.
int index = 0;
for (int y = 0; y < len1; y++) {
Object* value = other->get(y);
if (!value->IsTheHole() && !HasKey(this, value)) {
- result->set(len0 + index, other->get(y), mode);
+ Object* e = other->get(y);
+ ASSERT(e->IsString() || e->IsNumber());
+ result->set(len0 + index, e, mode);
index++;
}
}
=======================================
--- /branches/bleeding_edge/test/cctest/test-api.cc Tue Oct 5 05:48:32 2010
+++ /branches/bleeding_edge/test/cctest/test-api.cc Tue Oct 5 06:10:43 2010
@@ -11727,3 +11727,31 @@
context->Global()->Set(v8_str("ex"), try_catch.Exception());
ExpectTrue("ex instanceof SyntaxError");
}
+
+
+static v8::Handle<v8::Value> Getter(v8::Local<v8::String> property,
+ const v8::AccessorInfo& info ) {
+ return v8_str("42!");
+}
+
+
+static v8::Handle<v8::Array> Enumerator(const v8::AccessorInfo& info) {
+ v8::Handle<v8::Array> result = v8::Array::New();
+ result->Set(0, v8_str("universalAnswer"));
+ return result;
+}
+
+
+TEST(NamedEnumeratorAndForIn) {
+ v8::HandleScope handle_scope;
+ LocalContext context;
+ v8::Context::Scope context_scope(context.local());
+
+ v8::Handle<v8::ObjectTemplate> tmpl = v8::ObjectTemplate::New();
+ tmpl->SetNamedPropertyHandler(Getter, NULL, NULL, NULL, Enumerator);
+ context->Global()->Set(v8_str("o"), tmpl->NewInstance());
+ v8::Handle<v8::Array> result = v8::Handle<v8::Array>::Cast(CompileRun(
+ "var result = []; for (var k in o) result.push(k); result"));
+ CHECK_EQ(1, result->Length());
+ CHECK_EQ(v8_str("universalAnswer"), result->Get(0));
+}
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
