1303335 - Creating the result object for Object.getOwnPropertyDescriptor is slow

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Attachment: proof-of-concept: selfhosted ObjectGetOwnPropDesc

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. :-)

Comment 1

[André Bargull [:anba]]

Attachment: bug1303335.patch

Updated patch to self-host parts of {Object,Reflect}.{defineProperty,getOwnPropertyDescriptor}().

Comment 2

[André Bargull [:anba]]

bug 1344463 will give an additional improvement when the self-hosted {Object,Reflect}.defineProperty() function can use _DefineDataProperty.

Comment 3

[Till Schneidereit [:till]]

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.

Comment 4

[André Bargull [:anba]]

(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!

Comment 5

[André Bargull [:anba]]

Attachment: bug1303335.patch

Rebased patch to apply cleanly on inbound.

Comment 6

[Naveed Ihsanullah [:naveed]]

Andre are  you still able to work on this bug? Is that patch in near landable shape.

Comment 7

[André Bargull [:anba]]

I thought :till wanted to take over (comment #3, comment #4). Did I misunderstood?

Comment 8

[Jean Gong :jgong]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

>  } 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.

Comment 11

[André Bargull [:anba]]

(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.)

Comment 12

[Till Schneidereit [:till]]

Attachment: v2

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.

Comment 13

[André Bargull [:anba]]

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!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a14fc8f5babc
Move parts of Object.getOwnProperty and Object.defineProperty to self-hosted code. r=till

Comment 15

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a14fc8f5babc