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)

defect
Not set
normal

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.
Arai, can you check if this helps with the RegExp performance issues?
Attachment #8690276 - Flags: feedback?(arai.unmht)
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
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.
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 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+
Blocks: 1234038
till, do you have time to finish this? actually, I think this patch can be landed, and bug 1234038 needs this :)
Flags: needinfo?(till)
Bug 1232966 also wants this.
Blocks: 1232966
Pretty much what it says on the tin, and quite sraight-forward, I'd hope.
Attachment #8701481 - Flags: review?(efaustbmo)
Attachment #8690276 - Attachment is obsolete: true
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+
cf comment 11, can we land this?
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
(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)
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: