1167472 - Make classes work in the JITs.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Eric Faust [:efaust]]

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.

Comment 1

[Hannes Verschore [:h4writer]]

Realistically this will be for next release.

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

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.

Comment 3

[Hannes Verschore [:h4writer]]

(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.

Comment 4

[Matteo Ferretti [:zer0] [:matteo]]

(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.

Comment 5

[Ted Campbell [:tcampbell]]

Yep, it's next on my list.

Comment 6

[Ted Campbell [:tcampbell]]

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.

Comment 7

[Ted Campbell [:tcampbell]]

(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

[Kris Maglione [:kmag]]

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

[Andy McKay]

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.

Comment 10

[Ted Campbell [:tcampbell]]

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.

Comment 11

[Steve Fink [:sfink] [:s:]]

(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?

Comment 12

[Kris Maglione [:kmag]]

(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.

Comment 13

[Ted Campbell [:tcampbell]]

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.

Comment 14

[Ted Campbell [:tcampbell]]

Attachment: 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).

Comment 15

[Ted Campbell [:tcampbell]]

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.

Comment 16

[Ted Campbell [:tcampbell]]

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

Comment 17

[Kris Maglione [:kmag]]

(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.

Comment 18

[Ted Campbell [:tcampbell]]

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

Comment 19

[Ted Campbell [:tcampbell]]

Postponing until after merge (start of FF 56).

Comment 20

[Kris Maglione [:kmag]]

(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/

Comment 21

[Matteo Ferretti [:zer0] [:matteo]]

(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!

Comment 22

[Ted Campbell [:tcampbell]]

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.

Comment 23

[Kris Maglione [:kmag]]

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.

Comment 24

[Ted Campbell [:tcampbell]]

Unassigning myself as I am not actively working on or tracking this