Closed
Bug 1226762
Opened 9 years ago
Closed 9 years ago
Add support for getting original builtin constructors and prototypes in self-hosted code
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: till, Assigned: till)
References
Details
Attachments
(2 files, 5 obsolete files)
Various spec algorithms require access to the constructor or prototype of some builtin class. The patch here adds support for both to self-hosted code, in the form of GetBuiltinConstructor(name) and GetBuiltinPrototype(name). The latter isn't strictly required because it just uses the former and then gets the prototype, but it's convenient to have.
As the results are cached by JS code, this shouldn't need a JIT fast path to be useful.
Assignee | ||
Comment 1•9 years ago
|
||
Arai, can you check if this helps with the RegExp performance issues?
Attachment #8690276 -
Flags: feedback?(arai.unmht)
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 5•9 years ago
|
||
okay, I think I got it. as you mentioned in comment #0, I should cache it into global object's property when first time StringHasNoSearch is called.
tested with following code:
var cache = {
ObjectProto: null,
StringProto: null,
};
function StringHasNoSearch() {
if (!cache.ObjectProto) {
cache.ObjectProto = GetBuiltinConstructorImpl("Object").prototype;
cache.StringProto = GetBuiltinConstructorImpl("String").prototype;
}
if (cache.StringProto.__proto__ != cache.ObjectProto)
return false;
if (std_search in cache.StringProto)
return false;
return true;
}
Now it gets faster, thank you! :)
With this way, GetBuiltinPrototype is not necessary, as GetBuiltinConstructorImpl is called only once per each prototype.
Attachment #8690355 -
Attachment is obsolete: true
Attachment #8690356 -
Attachment is obsolete: true
Comment 6•9 years ago
|
||
ohhhhhhh, sorry, I should've checked your patch more thoroughly.
there are new functions in Utilities.js!
I'll fix my patches to use it.
Comment hidden (obsolete) |
Comment 8•9 years ago
|
||
Re-calculated the performance, with original attachment 8690276 [details] [diff] [review], looks like previous performance results are wrong, and it now works perfectly (I guess I chose wrong binary...?)
StringHasNoSearch is not changed.
function StringHasNoSearch() {
var ObjectProto = GetBuiltinPrototype("Object");
var StringProto = GetBuiltinPrototype("String");
if (StringProto.__proto__ != ObjectProto)
return false;
if (std_search in StringProto)
return false;
return true;
}
Attachment #8690358 -
Attachment is obsolete: true
Attachment #8690362 -
Attachment is obsolete: true
Comment 9•9 years ago
|
||
Comment on attachment 8690276 [details] [diff] [review]
Add support for getting original builtin constructors and prototypes in self-hosted code
Review of attachment 8690276 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you again, and sorry for so many wrong comments.
This patch helps a lot :)
Attachment #8690276 -
Flags: feedback?(arai.unmht) → feedback+
Comment 10•9 years ago
|
||
till, do you have time to finish this?
actually, I think this patch can be landed, and bug 1234038 needs this :)
Flags: needinfo?(till)
Comment 11•9 years ago
|
||
Bug 1232966 also wants this.
Assignee | ||
Comment 12•9 years ago
|
||
Pretty much what it says on the tin, and quite sraight-forward, I'd hope.
Attachment #8701481 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8690276 -
Attachment is obsolete: true
Comment 13•9 years ago
|
||
Comment on attachment 8701481 [details] [diff] [review]
Add support for getting original builtin constructors and prototypes in self-hosted code
Review of attachment 8701481 [details] [diff] [review]:
-----------------------------------------------------------------
This indeed looks very straightforward.
::: js/src/vm/SelfHosting.cpp
@@ +173,5 @@
> + if (!atom)
> + return false;
> + }
> + RootedId id(cx, AtomToId(atom));
> + JSProtoKey key = JS_IdToProtoKey(cx, id);
ah the dance of the types...
Attachment #8701481 -
Flags: review?(efaustbmo) → review+
Comment 14•9 years ago
|
||
cf comment 11, can we land this?
Assignee | ||
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cf4aceb220302ea48ff153531053dd62826c691
Bug 1226762 - Add support for getting original builtin constructors and prototypes in self-hosted code. r=efaust,f=arai
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Lars T Hansen [:lth] from comment #14)
> cf comment 11, can we land this?
Yes, sorry for not doing so earlier.
Flags: needinfo?(till)
Comment 17•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•