Creating the result object for Object.getOwnPropertyDescriptor is slow

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: anba, Assigned: till)

Tracking

(Blocks 1 bug)

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Micro benchmark:
---
if (typeof dateNow === "undefined") {
  if (typeof preciseTime === "function") {
    dateNow = function() { return preciseTime() * 1000; }
  } else if (typeof performance === "object" && typeof performance.now === "function") {
    dateNow = performance.now;
  } else {
    dateNow = Date.now;
  }
}

if (typeof getSelfHostedValue === "function") {
  // See attached POC patch.
  // Object.getOwnPropertyDescriptor = getSelfHostedValue("ObjectGetOwnPropDesc");
}

function bench(obj, pk) {
  function f(obj, pk) {
    var r = 0, it = 1000000;
    var t = dateNow();
    for (var i = 0; i < it; ++i) r += Object.getOwnPropertyDescriptor(obj, pk).value;
    if (r !== it) print("invalid result:", r);
    return dateNow() - t;
  }
  var warmup = 5, iter = 10, time = 0;
  for (var i = 0; i < warmup; ++i) f(obj, pk);
  for (var i = 0; i < iter; ++i) time += f(obj, pk);
  return (time / iter)|0;
}

print("object:", bench({a: 1}, "a"));
print("array:", bench([1], 0));
---

JSC: object: 185 ms, array: 190 ms
V8: object: 175 ms, array: 165 ms
Nashorn: object: 65 ms, array: 90 ms

SpiderMonkey: object: 970 ms, array: 920 ms
SpiderMonkey (result object created in self-hosted code): object: 100 ms, array: 80 ms


I've attached a simple (and not quite complete/correct) patch to demonstrate one possibility to speed up Object.getOwnPropertyDescriptor() (using a similar approach like the one in MapIteratorNext()). Alternatively we could explore to attach custom ObjectGroups for property descriptors objects of fully populated Property Descriptors, but I don't know how to create custom ObjectGroups, therefore I haven't prepared a patch to test this alternative. :-)
(Reporter)

Comment 1

3 years ago
Posted patch bug1303335.patch (obsolete) — Splinter Review
Updated patch to self-host parts of {Object,Reflect}.{defineProperty,getOwnPropertyDescriptor}().
Attachment #8791965 - Attachment is obsolete: true
(Reporter)

Comment 2

2 years ago
bug 1344463 will give an additional improvement when the self-hosted {Object,Reflect}.defineProperty() function can use _DefineDataProperty.
Blocks: 1134611
(Assignee)

Comment 3

2 years ago
anba, do you think finishing this patch will be much effort? This would probably be a substantial win on real world content, so would be very nice to have.

If you don't have time, I can take over and drive this to completion.
Flags: needinfo?(andrebargull)
(Reporter)

Comment 4

2 years ago
(In reply to Till Schneidereit [:till] from comment #3)
> anba, do you think finishing this patch will be much effort? This would
> probably be a substantial win on real world content, so would be very nice
> to have.

I had/have the following concerns about this approach:
- It decreases the performance in interpreter-only situations, because we're now executing more JavaScript code. (But you told me in Hawaii that we shouldn't be concerned about this issue.)
- We should consider to inline ToPropertyKey instead of adding the typeof checks.
- Object.getOwnPropertyDescriptor is now creating a temporary array in addition to the descriptor object. I'm not sure if and how this affects GC.
- Maybe we should split the new _DefineProperty() self-hosting function into different functions (DefineGenericProperty, DefineAccessorProperty, etc.), so we don't need to encode additional state in the arguments (see the "When args[4] is |null|, the data descriptor has a value component." comment in SelfHosting.cpp).
- FromPropertyDescriptorToArray (Object.cpp): I'm not sure it's okay to call initDenseElement() here, maybe it needs to be initDenseElementWithType()? Please double-check.


> If you don't have time, I can take over and drive this to completion.

I don't know when I got the time to finish this bug, so if you can take over, it'd be great!
Flags: needinfo?(andrebargull)
(Reporter)

Comment 5

2 years ago
Posted patch bug1303335.patch (obsolete) — Splinter Review
Rebased patch to apply cleanly on inbound.
Attachment #8803490 - Attachment is obsolete: true
Andre are  you still able to work on this bug? Is that patch in near landable shape.
Flags: needinfo?(andrebargull)
(Reporter)

Comment 7

2 years ago
I thought :till wanted to take over (comment #3, comment #4). Did I misunderstood?
Flags: needinfo?(andrebargull) → needinfo?(till)

Comment 8

2 years ago
In IRC chat with Till, he is working to toward getting a fix on this bug by Fx57.  I will keep this as QF:P1 for now.

Updated

2 years ago
Whiteboard: [qf:p1]

Updated

2 years ago
Duplicate of this bug: 1134611
>  } else if (typeof performance === "object" && typeof performance.now === "function") {
>    dateNow = performance.now;

Fwiw, that should be perforance.now.bind(performance); otherwise it will throw if called.
(Reporter)

Comment 11

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #10)
> >  } else if (typeof performance === "object" && typeof performance.now === "function") {
> >    dateNow = performance.now;
> 
> Fwiw, that should be perforance.now.bind(performance); otherwise it will
> throw if called.

Uh-oh, that shows how well I know browser APIs. :-) (Thankfully d8's |performance.now| works even when called as a stand-alone function.)

Updated

2 years ago
Assignee: nobody → till
(Assignee)

Comment 12

2 years ago
Posted patch v2Splinter Review
I finally got around to this. This looks pretty great, and I only did very slight tweaks to it.

Specifically:
- ObjectLookup{G,S}etter had a bug in that they kept going up the inheritance chain after finding a value descriptor instead of returning undefined.
- I introduced a cpp macro TO_PROPERTY_KEY to common-up the various hand-inlined fast paths for ToPropertyKey.
- I very slightly changed asserts in the _DefineProperty intrinsic.

Anba, I'm asking you for review, but only to confirm the small changes I did - I intend to land this with you as the author and r=me.
Attachment #8867113 - Attachment is obsolete: true
Flags: needinfo?(till)
Attachment #8901251 - Flags: review?(andrebargull)
(Reporter)

Comment 13

2 years ago
Comment on attachment 8901251 [details] [diff] [review]
v2

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

Yes, the changes all look correct to me. Thanks for spotting the bug in ObjectLookup{G,S}etter!
Attachment #8901251 - Flags: review?(andrebargull) → review+

Comment 14

2 years ago
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14fc8f5babc
Move parts of Object.getOwnProperty and Object.defineProperty to self-hosted code. r=till
https://hg.mozilla.org/mozilla-central/rev/a14fc8f5babc
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1397026
Depends on: 1397071
Depends on: 1423139
You need to log in before you can comment on or make changes to this bug.