857845 - Remove JaegerMonkey from SpiderMonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Now that BaselineCompiler has landed, it's probably time to remove JaegerMonkey, possibly in the next merge window (Firefox 24 - note that this is likely an ESR release).

Filing this bug so we can track it. :)

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

JM meta bug 536277 and JM meta bug 549412 for fuzz bugs can then be resolved.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Perhaps this bug could involve disabling it by default first, then removing it. (Feel free to file a new bug / change this if this is not optimal)

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> Perhaps this bug could involve disabling it by default first, then removing
> it. (Feel free to file a new bug / change this if this is not optimal)

JM does not compile anything if baseline is enabled, so it's effectively already disabled on all tier-1 platforms AFAIK (even ARMv6).

(There are some places outside JM where we check cx->methodJitEnabled, so right now flipping the pref will affect perf a bit, but these should be easy to fix.)

Comment 4

[Nicholas Nethercote [inactive]]

This is a MemShrink:P1 because removing JM will remove a lot of the type inference, which takes up lots of memory.

Comment 5

[Cameron Kaiser [:spectre]]

There was some discussion about making JM+TI, at least for 24, a compile-only option, because when JM+TI is removed from the code SPARC, MIPS and PowerPC (we use an internal out-of-the-tree implementation) will not have JIT support. I'd like to ask if that is still possible, similar to the legacy tracejit in 10ESR.

I'm trudging through a PowerPC backend for Ion, but if JM is still in 24ESR that buys me some time. It's a considerably larger undertaking. I don't know if anyone in the SPARC or MIPS camps are working on their own implementation.

I certainly wouldn't object to its complete removal in 25; I know it's become a maintainability beast.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: WIP

First cut.

  96 files changed, 62 insertions(+), 42804 deletions(-)

Builds, passes tests.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 1 (261e3e928ce9)

There's more code we can remove (browser and shell prefs, about:memory stuff, JSOPTION_METHODJIT, analysis code) and we should also rename some methods, but we can do that in follow-up patches I think. 1.5 MB is big enough already.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 749134 [details] [diff] [review]
Part 1 (261e3e928ce9)

What was the change in the opt file size?

Comment 9

[Cameron Kaiser [:spectre]]

What is the landing timeframe? (See my question in comment 5.)

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Cameron Kaiser from comment #9)
> What is the landing timeframe? (See my question in comment 5.)

I'm sorry but we were discussing this yesterday and we want to remove JM for Firefox 24. We really do appreciate people porting Firefox to other platforms and we do what we can to avoid breaking or regressing tier-3 platforms. However, keeping JM around for another cycle blocks some important optimization/refactoring work (bug 868437, analysis/TI work, etc) and the competition is heavy..

Comment 11

[Cameron Kaiser [:spectre]]

Obviously I have no way of preventing it, but mentioning this for the record, most of the tier-3 ports use the ESR to stabilize now (particularly the OS/2 port and ourselves), so removing or significantly changing platform support right before an ESR branch really hurts.

Bringing Ion up on a new architecture is intimidating, whereas with Jaeger it was mostly just tedious. I have the code written, but it still doesn't build; I'm going to have to revise the move emitter and lowering code for a second time. I don't know if I'll make 24 and my other PPC JS contributor had a hardware failure and is out of commission for the time being. We'll hold the stable TenFourFox userbase on 17ESR until/if we can get it working.

I certainly appreciate the maintainability problem, but losing JIT support right before what would have been our next stable branch is a potential port killer. FWIW.

Comment 12

[Dave Garrett]

Well, from the opposite perspective, it would make sense that Mozilla would prefer to drop major deprecated things before an ESR so they don't have to maintain the code for another full year.

(by the way, the JM prefs were fixed to deal with comment 3 in another bug and it's all disabled by default, so I'll update the title)

Comment 13

[Luke Wagner [:luke]]

Given the rapid pace of engine progress and the highly competitive landscape, I agree with Jan that we can't afford to wait, especially when tier-3 ports have the immediate short-term option of turning off the JITs.  I also agree with Dave that extending the maintenance lifetime of JM on ESR is highly undesirable.

Going forward, I'd think that porting and maintaining the baseline jit should be *much* easier than JM for tier-3 ports (and hopefully not too much work given a working JM port).

Comment 14

[Andrew McCreight [:mccr8]]

JM would be disabled on ESR, so there would be no burden there for JM sec fixes.  Though I guess if tearing out JM enables a lot of cleanups of BC and IM, then backporting sec fixes to BC and IM will become much more painful...

Comment 15

[Cameron Kaiser [:spectre]]

> Going forward, I'd think that porting and maintaining the baseline jit
> should be *much* easier than JM for tier-3 ports (and hopefully not too much
> work given a working JM port).

Maintaining, probably so, since my limited understanding of Ion shows that much of the compiler's operation occurs at a higher level. But that means there has to be a lot more machinery in the architecture-specific backend. With Jaeger it was a matter of writing the assembler/ pieces, the trampoline, and teaching MethodJIT about ABI-compliant stack frames, which wasn't an easy thing to do, but the machinery was pretty clear about what needed to be done. There's a lot more going on in Ion and I don't understand a lot of it. Some of the comments in the ARM version indicate they're not exactly sure what's going on either. :P

However, this is all orthogonal to this bug and I've said my piece, so I'll respectfully return to lurking.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Cameron Kaiser from comment #15)
> There's a lot more going on in Ion and I don't understand
> a lot of it. Some of the comments in the ARM version indicate they're not
> exactly sure what's going on either. :P

Just ask in #jsapi or #ionmonkey on IRC, or feel free to mail me directly if you have any questions about Baseline or Ion.

Comment 17

[Cameron Kaiser [:spectre]]

A porting guide would help a lot more.

Comment 18

[Christian Holler (:decoder)]

Cameron, I think it would make a lot of sense for you to join the #jsapi and #ionmonkey channels on our IRC network and talk to the JS devs to describe the current problems you have with the porting. Jandem told me he'd be glad to help out with these issues :)

Comment 19

[Luke Wagner [:luke]]

(In reply to Cameron Kaiser from comment #15)
To be clear, I was talking about the newly-landed baseline jit (without Ion) as the hopefully-easy initial porting target.

Comment 20

[Cameron Kaiser [:spectre]]

Luke, I sent you an E-mail. I'll pick up the discussion there.

Comment 21

[Jan de Mooij [:jandem]]

Part 1:

http://hg.mozilla.org/integration/mozilla-inbound/rev/89a645d498e3

Comment 22

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #8)
> What was the change in the opt file size?

I looked at a (non-PGO) Windows opt build with/without the patch. mozjs.dll went from 3,310 KB to 2,908 KB.

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Part 2 - More StackFrame cleanup

Remove dead/unnecessary code.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89a645d498e3

Comment 25

[Jacek Caban]

Attachment: Remove Trampoline* assemblies from Makefile.in

The patch removed methodjit/Trampoline* files, but didn't remove them from Makefile.in. This broke 64-bit mingw build for me and, looking at the code, it probably broke MSVC 64-bit and Solaris builds.

The attached patch fixes the problem and does a little clean up (fewer ifdef ENABLE_ION are needed now).

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 750348 [details] [diff] [review]
Remove Trampoline* assemblies from Makefile.in

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

Thanks!

Comment 27

[Cameron Kaiser [:spectre]]

Jan, did you receive either my message, Luke's or Martin's? With two cycles to go before 24, I really want to know what I need to have together to get Baseline up without Ion, and I'm running out of time. I have the assembler and macroassembler written, and it does compile, but I'm still missing some pieces in the code generator.

I'd really appreciate some pointers so I can break the workload down into pieces I need to finish.

Comment 28

[Jan de Mooij [:jandem]]

(In reply to Cameron Kaiser from comment #27)
> Jan, did you receive either my message, Luke's or Martin's?

Yesterday I replied to your mail.. Did you not receive that? I will resend it.

Comment 29

[Cameron Kaiser [:spectre]]

No, I didn't. You can use either this address or the other one. Thanks, I appreciate it.

Comment 30

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Remove browser prefs, about:memory entries

Comment 31

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Just to note, jandem and I spoke in person about making the soon-to-be-old shell flags for JM to be no-ops in the short term rather than removing them, which can break portions of fuzzing harnesses as decoder discovered after TraceMonkey was removed.

Comment 32

[Luke Wagner [:luke]]

Comment on attachment 750147 [details] [diff] [review]
Part 2 - More StackFrame cleanup

Great job, continue the rippage.

Comment 33

[Kannan Vijayan [:djvj]]

Comment on attachment 750710 [details] [diff] [review]
Part 3 - Remove browser prefs, about:memory entries

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

::: js/src/jsapi-tests/testDebugger.cpp
@@ -244,5 @@
>  
> -        uint32_t opts = JS_GetOptions(cx);
> -        opts |= JSOPTION_METHODJIT | JSOPTION_METHODJIT_ALWAYS;
> -        JS_SetOptions(cx, opts);
> -

These MethodJIT options get thrown away, but shouldn't baseline be enabled here?

::: js/src/jsapi-tests/testProfileStrings.cpp
@@ -190,5 @@
>  
>  BEGIN_TEST(testProfileStrings_isCalledWhenError)
>  {
>      CHECK(initialize(cx));
> -    JS_SetOptions(cx, JS_GetOptions(cx) | JSOPTION_METHODJIT | JSOPTION_METHODJIT_ALWAYS);

Why are ion and baseline not enabled in the |isCalledWhenError| test variant, but is called on the other variants?

Comment 34

[Jacek Caban]

I landed my patch and part 2:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee42c4773641
https://hg.mozilla.org/integration/mozilla-inbound/rev/44dc31bdb7e1

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ee42c4773641
https://hg.mozilla.org/mozilla-central/rev/44dc31bdb7e1

Comment 36

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1add7f1eeb40

(In reply to Kannan Vijayan [:djvj] from comment #33)
> These MethodJIT options get thrown away, but shouldn't baseline be enabled
> here?

The test calls a function once so it's not hot enough for baseline. Furthermore the jsdbg2 jit-tests are a lot more complete so I don't think it matters.

> Why are ion and baseline not enabled in the |isCalledWhenError| test
> variant, but is called on the other variants?

Same here, but added the SetOptions call back for consistency.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1add7f1eeb40

Comment 38

[Jan de Mooij [:jandem]]

Closing this bug. There may still be some dead code left, but we can fix that in follow-up bugs.