Object.propertyIsEnumerable does not work well proxies

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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")
---
Posted patch bug1066834.diff (obsolete) — Splinter Review
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];
}
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8489158 - Flags: review?(jorendorff)
Blocks: 1063921
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.
Attachment #8489158 - Flags: review?(jorendorff)
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.
Attachment #8489158 - Attachment is obsolete: true
Attachment #8498428 - Flags: review?(jorendorff)
Comment on attachment 8498428 [details] [diff] [review]
bug1066834-2.diff

Review of attachment 8498428 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #8498428 - Flags: review?(jorendorff) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61453e5b990d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1082202
You need to log in before you can comment on or make changes to this bug.