Closed
Bug 1379222
Opened 7 years ago
Closed 7 years ago
Avoid calling [[Get]](..., "prototype") when constructing built-ins and callee == newTarget
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file, 1 obsolete file)
32.94 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
We can skip the [[Get]] for the "prototype" property when callee == newTarget is true.
Assignee | ||
Comment 1•7 years ago
|
||
Replaces the GetPrototypeFrom[Callable]Constructor() call in built-in constructors with the new GetPrototypeFromBuiltinConstructor() method. GetPrototypeFromBuiltinConstructor() only calls GetPrototypeFromConstructor() when we're constructing and the callee is different from newTarget. The "prototype" property of built-in constructors is non-writable and non-configurable, so it's okay to skip reading it when not needed. Drive-by changes: - TypedArrayObjectTemplate#makeInstance(): Only call GetBuiltinPrototype when the return value is actually used. - WeakMap_construct: Replace CreateThis() with NewObjectWithClassProto(), which means WeakMaps are now using OBJECT2_BACKGROUND instead of OBJECT4_BACKGROUND. Was there a specific reason WeakMaps used OBJECT4_BACKGROUND for their allocation-kind?
Attachment #8884341 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•7 years ago
|
||
The patch improves this µ-benchmark from 110ms to 85ms for me: var t = Date.now(); for (var i = 0; i < 1000000; ++i) { new Boolean(); } print(Date.now() - t);
Comment 3•7 years ago
|
||
Comment on attachment 8884341 [details] [diff] [review] bug1379222.patch Review of attachment 8884341 [details] [diff] [review]: ----------------------------------------------------------------- Good finds! Thanks for fixing. ::: js/src/jsnum.cpp @@ +503,5 @@ > > + if (!args.isConstructing()) { > + if (args.length() > 0) { > + args.rval().set(args[0]); > + } else { Nit: no {} here ::: js/src/jsobj.cpp @@ +1002,5 @@ > return true; > } > > bool > +js::GetPrototypeFromBuiltinConstructor(JSContext* cx, const CallArgs& args, MutableHandleObject proto) Please move this into jsobj.h and make it MOZ_ALWAYS_INLINE? In most cases this will be just 1 or 2 branches and the call overhead will be more than that. GetPrototypeFromConstructor can stay out of line, as that's the less interesting case.
Attachment #8884341 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 4•7 years ago
|
||
Updated per review comments, carrying r+. (In reply to Jan de Mooij [:jandem] from comment #3) > ::: js/src/jsobj.cpp > @@ +1002,5 @@ > > return true; > > } > > > > bool > > +js::GetPrototypeFromBuiltinConstructor(JSContext* cx, const CallArgs& args, MutableHandleObject proto) > > Please move this into jsobj.h and make it MOZ_ALWAYS_INLINE? In most cases > this will be just 1 or 2 branches and the call overhead will be more than > that. > > GetPrototypeFromConstructor can stay out of line, as that's the less > interesting case. Good suggestion, thanks! This seems to improve the µ-benchmark from comment #2 by another 10ms.
Attachment #8884341 -
Attachment is obsolete: true
Attachment #8884806 -
Flags: review+
Assignee | ||
Comment 5•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=038001c28e64603f3b863d870a32f7e3d72d3eb0
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d0c4418614b Avoid [[Get]] for "prototype" property when calling builtin constructors. r=jandem
Keywords: checkin-needed
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0d0c4418614b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•