Make classes work in the JITs.

NEW
Assigned to

Status

()

Core
JavaScript Engine: JIT
P1
normal
2 years ago
10 days ago

People

(Reporter: efaust, Assigned: tcampbell)

Tracking

(Depends on: 2 bugs, Blocks: 5 bugs, {perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:p1])

Attachments

(1 obsolete attachment)

(Reporter)

Description

2 years ago
At present, we have classes, but none of the ops are implemented in the jits. We can at least write simple stubs to solve this problem.
(Reporter)

Updated

2 years ago
Depends on: 1169743
(Reporter)

Updated

2 years ago
Depends on: 1169745
(Reporter)

Updated

2 years ago
Depends on: 1169746
Blocks: 1298286
Blocks: 1307395

Updated

11 months ago
Blocks: 1307062
Priority: -- → P2
Keywords: perf
Assignee: efaustbmo → nobody
Status: ASSIGNED → NEW
Priority: P2 → P1
Realistically this will be for next release.
Priority: P1 → P2
Priority: P2 → P1
(Assignee)

Updated

6 months ago
Assignee: nobody → tcampbell
Is there any progress on this bug?

How bad would be the performance if we're going to use `class` instead of `function` and `prototype` in firefox codebase? We already have them in WebExt, and I'm tempted to use them in our devtools, but not if we got some perf issues.
Flags: needinfo?(tcampbell)
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #2)
> Is there any progress on this bug?
> 
> How bad would be the performance if we're going to use `class` instead of
> `function` and `prototype` in firefox codebase? We already have them in
> WebExt, and I'm tempted to use them in our devtools, but not if we got some
> perf issues.

For now I would say to not use it in performance critical code. Ted is going to look into implementing this in this or next release. He is finishing another bug first.
(In reply to Hannes Verschore [:h4writer] from comment #3)

> For now I would say to not use it in performance critical code. Ted is going
> to look into implementing this in this or next release. He is finishing
> another bug first.

Thanks, it's as I thought. I'll keep on eye on the progress of this bug to understand when classes could be used without perf issues.
Flags: needinfo?(tcampbell)
(Assignee)

Comment 5

5 months ago
Yep, it's next on my list.

Updated

3 months ago
Blocks: 1363905

Updated

3 months ago
Whiteboard: [qf:p1]
(Assignee)

Comment 6

3 months ago
Can you clarify the reasoning for this to be qf:p1?  This bug got sidelined in favor of other qf JIT work. I can get back to it if it is a blocker so I'd like to understand a bit more of it and what level of features and performance are needed.
Flags: needinfo?(amckay)
(Assignee)

Comment 7

3 months ago
(For spidermonkey devs..)
Currently bytecode supported is as follows:

Property Accessors - No baseline/ion support
    - JSOP_GETPROP_SUPER
    - JSOP_STRICTSETPROP_SUPER
    - JSOP_SETPROP_SUPER
    - JSOP_GETELEM_SUPER
    - JSOP_SETELEM_SUPER
    - JSOP_STRICTSETELEM_SUPER

Implicit Objects - No baseline/ion support
    - JSOP_SUPERFUN
    - JSOP_SUPERBASE
    - JSOP_INITHOMEOBJECT
    - JSOP_NEWTARGET
    - JSOP_CLASSHERITAGE

Default Constructor - No baseline/ion support
    - JSOP_CLASSCONSTRUCTOR
    - JSOP_DERIVEDCONSTRUCTOR

Super Calls - Supported code needs review
    - JSOP_SUPERFUN
    - JSOP_SUPERCALL
    - JSOP_SPREADSUPERCALL

Other - Supported
    - JSOP_IS_CONSTRUCTING


There are a number of WIP patches in the depends of this bug that have bits and pieces.

Comment 8

3 months ago
At this point, there are several modules in-tree that make heavy uses of classes and need to be loaded at startup. These modules all seem to take considerably longer to load than most other modules that we load during startup. It's hard to say for certain whether the lack of JIT support for classes is responsible, but it seems likely.

So unless we get at least baseline support soon, those modules will likely need to be rewritten not to use ES6 classes.

Comment 9

3 months ago
To echo Kris, we've got a lot of ES6 code in WebExtensions and we feel we could get a benefit by having them go through the jit. WebExtensions are part of 57, but also this is impacting us for screenshots which is causing performance regressions on startup in 54 and 55. 

Not sure how much this will impact performance, and making it a [qf:p1] as opposed to just [qf] might have been a presumptous on my part.
Flags: needinfo?(amckay)
(Assignee)

Comment 10

3 months ago
Thanks for the follow-up. I'll do some work on this bug and hopefully we can get Baseline supporting classes and then re-evaluate the priority of Ion support as a part 2.
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #8)
> At this point, there are several modules in-tree that make heavy uses of
> classes and need to be loaded at startup. These modules all seem to take
> considerably longer to load than most other modules that we load during
> startup. It's hard to say for certain whether the lack of JIT support for
> classes is responsible, but it seems likely.

Just passing by, but this sounds a little odd. How would the JIT affect load time? Does "load during startup" include executing the code a bunch?
(In reply to Steve Fink [:sfink] [:s:] from comment #11)
Just passing by, but this sounds a little odd. How would the JIT affect load
> time? Does "load during startup" include executing the code a bunch?

It's a matter of executing the root scope more than loading. The actual load and parse times are fairly well-controlled by the startup cache.

I can't say for certain that the JIT would have a huge impact on that, but I can say for certain that compiling with noScriptRval by default in the subscript loader did have a huge impact on the load time of the scripts that it loaded (bug 1356810). And given that one of the main impacts of compiling with an rval is disabling the JIT for the top-level scope, it seems pretty likely that defining a class in the top-level scope would have a similar impact.
(Assignee)

Comment 13

3 months ago
As an clarification to the question of "Can I use classes in production?":

- There is a distinction between performance of a class declaration and the invocation of methods on a class object. - Using |super| keyword (including the |super()| implied by derived class default constructors) will force interpreter mode for that function and be slow. This should be fixed soon to at least support Baseline JIT.
- If your method is basically a normal JS function, it is probably already supported by our JITs and should run similar to a |Foo.prototype.bar = function() { }|
- The body containing a class definition will be slow. This means any top-level loops will be slow. In general you are probably better moving slow loops into helper functions. This is unlikely to change soon unless there is a compelling reason.
(Assignee)

Comment 14

3 months ago
Created attachment 8875033 [details] [diff] [review]
WIP-ROLLUP-Class-support-in-Baseline-JIT.patch

Work-in-Progress support of |super| keyword in the JITs (with a few other fixup patches merged in).
(Assignee)

Comment 15

3 months ago
Comment on attachment 8875033 [details] [diff] [review]
WIP-ROLLUP-Class-support-in-Baseline-JIT.patch

Gary, are you able to give this a quick fuzz run? It unblocks a few code-paths in the JITs that weren't well tested before.
Attachment #8875033 - Flags: feedback?(gary)
(Assignee)

Updated

3 months ago
Flags: needinfo?(choller)
(Assignee)

Comment 16

3 months ago
Comment on attachment 8875033 [details] [diff] [review]
WIP-ROLLUP-Class-support-in-Baseline-JIT.patch

I'm cancelling my fuzz request. I've done more cleanup and review on my end and have less concerns about this patch which covers Baseline.

For Ion support, this might be another story, but I don't have code ready for it yet
Flags: needinfo?(choller)
Attachment #8875033 - Flags: feedback?(gary)
(In reply to Ted Campbell [:tcampbell] from comment #13)
> - The body containing a class definition will be slow. This means any
> top-level loops will be slow. In general you are probably better moving slow
> loops into helper functions. This is unlikely to change soon unless there is
> a compelling reason.

OK. In that case there are a few JSMs with top-level loops and classes that we're going to have to go through and change, particularly in the add-on manager. But having JIT support for methods with super calls should still help a lot.

Updated

3 months ago
See Also: → bug 1371047
(Assignee)

Updated

2 months ago
Attachment #8875033 - Attachment is obsolete: true
(Assignee)

Comment 18

2 months ago
Good news. Class support in Baseline is coming. Pending reviews and landings, we might be able to sneak in to FF55. This includes super calls as well as top-level declarations no longer blocking optimization.

Try build if you want a preview: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b61b9aee8911c03fe7efb58b9fa6603943ffb6d6
(Assignee)

Comment 19

2 months ago
Postponing until after merge (start of FF 56).
(In reply to Ted Campbell [:tcampbell] from comment #18)
> Good news. Class support in Baseline is coming. Pending reviews and
> landings, we might be able to sneak in to FF55. This includes super calls as
> well as top-level declarations no longer blocking optimization.

\o/
(In reply to Ted Campbell [:tcampbell] from comment #18)
> Good news. Class support in Baseline is coming. Pending reviews and
> landings, we might be able to sneak in to FF55. This includes super calls as
> well as top-level declarations no longer blocking optimization.
> 
> Try build if you want a preview:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b61b9aee8911c03fe7efb58b9fa6603943ffb6d6

Kudos to you, Ted!
(Assignee)

Comment 22

2 months ago
Most of the missing Baseline support has landed in nightly. Can you give any feedback on if this has helped the root scope performance issues? Support for |super.prop| isn't fully complete, but is on the way.
Flags: needinfo?(kmaglione+bmo)
It's hard to say for certain, since a lot of other changes have landed since I last profiled this in-depth, but it looks like there's been a significant improvement. In particular, for XPIProvider.jsm, I used to consistently see load times from 10-15ms. In a run that I just did, that's down to 3.6ms (which is a much bigger improvement than we saw from bug 1363925).

Other module load times seem to have improved too, but many of them have been significantly refactored recently, so I can't really make a meaningful comparison at the moment.

(In reply to Ted Campbell [:tcampbell] from comment #22)
> Support for |super.prop| isn't fully complete, but is on the way.

That's good to know. The biggest chunks of time I'm seeing now when loading these modules are stacks using Map subclasses that override get() and call super.get(), where we're repeatedly entering baseline and then falling back to the interpreter.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

2 months ago
Depends on: 1377051
(Assignee)

Updated

2 months ago
Depends on: 1378186
(Assignee)

Updated

2 months ago
Depends on: 1378189
Depends on: 1382612
You need to log in before you can comment on or make changes to this bug.