The Object.propertyIsEnumerable code must not use JSObject::lookupGeneric. Possibly just use the intrinsic function from bug 1063921 instead? Expected: Call only "getOwnPropertyDescriptor" trap Actual: Calls "has" trap --- var p = new Proxy({}, new Proxy({}, { get(t, pk) { print(`trap:${pk}`); return t[pk]; }})); Object.prototype.propertyIsEnumerable.call(p, "foo"); --- Expected: Don't call any trap Actual: Calls "has" trap --- var p = new Proxy({}, new Proxy({}, { get(t, pk) { print(`trap:${pk}`); return t[pk]; }})); Object.prototype.propertyIsEnumerable.call(Object.create(p), "foo") ---

propertyIsEnumerable may be used in bug 1063921, therefore I've tried not to regress or possibly even improve performance. Time spent in ms for `perf(1000, 10000, true / false)`: Orginal: 1750 GetOwnPropertyDescriptor for all object kinds: 1850 HasOwnProperty to retrieve shape for non-proxy objects: 1450 Above with additional NoGC HasOwnProperty (version in patch): 1350 function perf(propertyCount, iterations, enumerable) { var o = {}; var propertyKeys = []; for (var i = 0; i < propertyCount; ++i) { propertyKeys[i] = "p" + i; Object.defineProperty(o, propertyKeys[i], {value: i, enumerable: enumerable}); } var c = 0; var d = Date.now(); for (var j = 0; j < iterations; ++j) { for (var i = 0; i < propertyCount; ++i) { c += o.propertyIsEnumerable(propertyKeys[i]) ? 1 : 0; } } return [Date.now() - d, c]; }

Comment on attachment 8489158 [details] [diff] [review] bug1066834.diff Review of attachment 8489158 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch! Sorry for the slow review here. ::: js/src/builtin/Object.cpp @@ +74,5 @@ > + args.rval().setBoolean((attrs & JSPROP_ENUMERATE) != 0); > + return true; > + } > + } > + } This looks like an optimization, but I think if we were to optimize this we might want to do it in the JIT instead. (In fact it's *possible* this isn't actually faster, though I'm not placing any bets.) In any case, it seems premature to me, so please remove it. @@ +80,3 @@ > /* Step 1. */ > + RootedId idRoot(cx); > + if (!ValueToId<CanGC>(cx, idValue, &idRoot)) ...and rename this back to 'id'. @@ +90,5 @@ > + if (obj->is<ProxyObject>()) { > + /* Step 3. */ > + Rooted<PropertyDescriptor> desc(cx); > + if (!Proxy::getOwnPropertyDescriptor(cx, obj, idRoot, &desc)) > + return false; Actually how about we replace everything starting from 'if (obj->is<ProxyObject>())' with /* Step 3. */ Rooted<PropertyDescriptor> desc(cx); if (!GetOwnPropertyDescriptor(cx, obj, id, &desc)) return false; /* Steps 4-5. */ args.rval().setBoolean(desc.object() && desc.isEnumerable()); return true; This should work for proxies, ordinary objects, and weirdo objects alike, saving another 25 or so lines of code. I just don't think this method has to be fast, and the simplest possible code won't even be all that slow. ::: js/src/tests/ecma_6/Object/propertyIsEnumerable-proxy.js @@ +4,5 @@ > +function logProxy(object) { > + var log = []; > + var handler = { > + getOwnPropertyDescriptor(target, propertyKey) { > + log.push(`getOwnPropertyDescriptor:${String(propertyKey)}`); I would just log.push(propertyKey); here, but it's OK either way.

André, I cleared the review flag, but if you have a moment please send a new patch along, r?me, and we'll land it.

(In reply to Jason Orendorff [:jorendorff] from comment #2) > I just don't think this method has to be fast, and the simplest possible > code won't even be all that slow. There was a visible performance difference when I tested it (see numbers in comment 1). And normally it'd even say propertyIsEnumerable is kind of exotic and seldom used, so that performance doesn't actually matter, but for bug 1063921 a fast propertyIsEnumerable implementation could be useful. Disclaimer: I don't know the total amount of time spent in propertyIsEnumerable when it's used (or rather will be used) in Object.assign, maybe these micro-opts won't pay off! Well, either way is fine by me - to keep the somewhat cryptic optimizations or to change it to use the more simple GetOwnPropertyDescriptor method. Your choice! :-D

(In reply to André Bargull from comment #4) > There was a visible performance difference when I tested it (see numbers in > comment 1). And normally it'd even say propertyIsEnumerable is kind of > exotic and seldom used, so that performance doesn't actually matter, but for > bug 1063921 a fast propertyIsEnumerable implementation could be useful. Oh, I see! Sorry for not reading. In this case, let's keep your new NoGC fast path, but rewrite the fallback code to be as simple as possible. It's OK for the slow path to be slow. Please also hang a comment on the fast path noting that it's a pure optimization.

Comment on attachment 8498428 [details] [diff] [review] bug1066834-2.diff Review of attachment 8498428 [details] [diff] [review]: ----------------------------------------------------------------- Great!