Add support for getting original builtin constructors and prototypes in self-hosted code

RESOLVED FIXED in Firefox 46

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: till, Assigned: till)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Created attachment 8690276 [details] [diff] [review]
Add support for getting original builtin constructors and prototypes in self-hosted code

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)
Created attachment 8690358 [details]
Performance comparison of selfhosted String.prototype.search (bug 887016)

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.
Comment hidden (obsolete)
Created attachment 8690364 [details]
Performance comparison of selfhosted String.prototype.search (bug 887016)

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+

Updated

2 years ago
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.

Updated

2 years ago
Blocks: 1232966
(Assignee)

Comment 12

2 years ago
Created attachment 8701481 [details] [diff] [review]
Add support for getting original builtin constructors and prototypes in self-hosted code

Pretty much what it says on the tin, and quite sraight-forward, I'd hope.
Attachment #8701481 - Flags: review?(efaustbmo)
(Assignee)

Updated

2 years ago
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?
(Assignee)

Comment 15

2 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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0cf4aceb2203
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.