Create a choke-point for class initialization

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla29
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Assignee)

Description

4 years ago
I have various patches in my queue for this, and it seemed self-contained enough to split out.

At the moment, we have a number of different points where we may potentially invoke a routine to initialize a standard class. In order to implement bug 959012, we need this all to funnel through GlobalObject::ensureConstructor. These patches do that.
(Assignee)

Updated

4 years ago
Depends on: 959334
(Assignee)

Comment 2

4 years ago
Created attachment 8364722 [details] [diff] [review]
Part 1 - When resolving iterator classes, use JSProto_Iterator instead of JSProto_GeneratorFunction. v1

The former triggers all of the appropriate initialization, whereas the latter
is 'imaginary' in jsprototypes.h, which means that the is no initialization
function associated with it.
Attachment #8364722 - Flags: review?(jorendorff)
(Assignee)

Comment 3

4 years ago
Created attachment 8364723 [details] [diff] [review]
Part 2 - Don't directly invoke init functions in GlobalObject::initStandardClasses. v1
Attachment #8364723 - Flags: review?(jorendorff)
(Assignee)

Comment 4

4 years ago
Created attachment 8364724 [details] [diff] [review]
Part 3 - Stop manually invoking initialization routines in GlobalObject.h. v1
Attachment #8364724 - Flags: review?(jorendorff)
(Assignee)

Comment 5

4 years ago
Created attachment 8364725 [details] [diff] [review]
Part 4 - Remove init functions from JSStdName, and pipe everything through ensureConstructor. v1
Attachment #8364725 - Flags: review?(jorendorff)
(Assignee)

Comment 6

4 years ago
Created attachment 8364726 [details] [diff] [review]
Part 5 - Don't call initIteratorClasses directly, and use ensureConstructor. v1

This isn't strictly necessary at this point. The basic issue is that there are
a few new classes (iterator stuff, intl stuff, and typed objects) that use this
kind of constructor bootstrap scheme, and each of them will need something like
this to move to ClassSpecs. But that doesn't actually need to happen until we
convert them. I wrote this patch before I realized this, and I believe it's
correct, so I figured I'd get it landed along with everything. But I'm also ok
to skip it for now.
Attachment #8364726 - Flags: review?(jorendorff)
(Assignee)

Comment 7

4 years ago
Given that we've all talked about this now, switching some of these over to luke to spread the load.
(Assignee)

Updated

4 years ago
Attachment #8364722 - Flags: review?(jorendorff) → review?(luke)
(Assignee)

Updated

4 years ago
Attachment #8364723 - Flags: review?(jorendorff) → review?(luke)
(Assignee)

Updated

4 years ago
Attachment #8364724 - Flags: review?(jorendorff) → review?(luke)
(Assignee)

Updated

4 years ago
Attachment #8364725 - Flags: review?(jorendorff) → review?(luke)
(Assignee)

Updated

4 years ago
Attachment #8364726 - Flags: review?(jorendorff) → review?(luke)

Updated

4 years ago
Attachment #8364722 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #8364723 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #8364724 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #8364725 - Flags: review?(luke) → review+

Updated

4 years ago
Attachment #8364726 - Flags: review?(luke) → review+
(Assignee)

Comment 10

4 years ago
Oh I see. Rooting analysis is considered to be a separate platform:

https://tbpl.mozilla.org/?tree=Try&rev=1f20ebbd5d06
(Assignee)

Comment 11

4 years ago
Created attachment 8368748 [details] [diff] [review]
Part 6 - Make various getOrCreate methods static to efficiently fix hazards. v1
Attachment #8368748 - Flags: review?(jwalden+bmo)
Comment on attachment 8368748 [details] [diff] [review]
Part 6 - Make various getOrCreate methods static to efficiently fix hazards. v1

Review of attachment 8368748 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/GlobalObject.h
@@ +471,5 @@
>    public:
> +    static JSObject *getOrCreateIteratorPrototype(JSContext *cx,
> +                                                  Handle<GlobalObject*> global)
> +    {
> +        if (!global->ensureConstructor(cx, JSProto_Iterator))

Does ensureConstructor presumably create a Rooted for the global?  Seems like it should be static too, to avoid that.  Followup.
Attachment #8368748 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Updated

4 years ago
Blocks: 966845
You need to log in before you can comment on or make changes to this bug.