Closed
Bug 1303335
Opened 8 years ago
Closed 7 years ago
Creating the result object for Object.getOwnPropertyDescriptor is slow
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
People
(Reporter: anba, Assigned: till)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
24.11 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Updated patch to self-host parts of {Object,Reflect}.{defineProperty,getOwnPropertyDescriptor}().
Attachment #8791965 -
Attachment is obsolete: true
Reporter | ||
Comment 2•7 years ago
|
||
bug 1344463 will give an additional improvement when the self-hosted {Object,Reflect}.defineProperty() function can use _DefineDataProperty.
Assignee | ||
Comment 3•7 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•7 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•7 years ago
|
||
Rebased patch to apply cleanly on inbound.
Attachment #8803490 -
Attachment is obsolete: true
Comment 6•7 years ago
|
||
Andre are you still able to work on this bug? Is that patch in near landable shape.
Updated•7 years ago
|
Flags: needinfo?(andrebargull)
Reporter | ||
Comment 7•7 years ago
|
||
I thought :till wanted to take over (comment #3, comment #4). Did I misunderstood?
Flags: needinfo?(andrebargull) → needinfo?(till)
Comment 8•7 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•7 years ago
|
Whiteboard: [qf:p1]
Comment 10•7 years ago
|
||
> } 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•7 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•7 years ago
|
Assignee: nobody → till
Assignee | ||
Comment 12•7 years ago
|
||
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•7 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•7 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
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a14fc8f5babc
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•