633702 - Reduce JIT-related configure-time options

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Paul Biggar]

We currently have --enable-tracejit, --enable-methodjit, --enable-monoic and --enable-polyic. Are we able to reduce this to just --enable-jits? This would reduce code complexity a little bit, and reduce the size of our testing space.

I imagine the issue is that some platforms aren't happy with jitted code at all (say game consoles and similar embedded devices), but this still allows us to amalgamate the flags.

I could also see that the methodjit used a lot of memory, which might be undesirable, but I believe that is solved now.

Finally, I believe some ICs didn't work on ARM, but I believe that too is solved.

Have we other reasons to keep these? Perhaps I should poll the js-engine mailing list?


(Of course, we want to keep the run-time options, no qualms there.)

Comment 2

[Paul Biggar]

This isn't a dup. Bug 633543 is about run-time flags for the shell binary. This is about configure-time options and #ifdefs.

Comment 3

[Sean Stangl [:sstangl]]

Whoops, sorry! This is indeed distinct.

We should not remove the existing #ifdefs: they are extremely helpful for testing, and are necessary when implementing certain new features. For example, it is extremely helpful to be able to selectively turn off JITs when investigating the performance of 128-bit fatvals, without forcing me to modify the tracejit and/or methodjit in order to test interpreter performance. I would be significantly less happy.

I added the --enable-monoic and --enable-polyic options as the first step of the x86_64 port because the various inline caches were/are rather non-trivial to implement, and I really needed to get them out of the picture to test anything at all. Additionally, it was necessary to implement them one-at-a-time. Removing these options hinders porting to other platforms in the future; I have also used them in the course of investigating complicated bugs.

It just does not seem worth it to lose this control, especially when the default configuration options enable all jits, and so they must only be selectively disabled. "Reducing the size of our testing space" is not necessarily a positive thing.

Comment 4

[Paul Biggar]

(In reply to comment #3)
> For example,
> it is extremely helpful to be able to selectively turn off JITs when
> investigating the performance of 128-bit fatvals,

> I have also used them in the course of investigating complicated bugs.

Are the shell command line options not sufficient for this?



> I added the --enable-monoic and --enable-polyic options as the first step of
> the x86_64 port

> Removing these options hinders porting to other platforms in the
> future; I have also used them in the course of investigating complicated bugs.
 
> It just does not seem worth it to lose this control

Keeping these options requires maintainance. Already --disable-methodjit breaks very often, and takes people's time to fix. It does seem like having them would make ports simpler, but AFAICT, there aren't any more instruction sets to port to.

Comment 5

[(not currently active) Ted Mielczarek]

More options means more ways for users to shoot themselves in the foot "ooh, I wonder what *this* button does?" as well as more code paths for us to maintain. The fewer options necessary, the better. I think we'd be well-served by just having a --disable-jit option at this point.

In fact, I would have r-'ed the patch(es) that added --enable-monoic / --enable-polyic if they had come through my review queue. Those options seem really obscure, and only useful to JS hackers. We shouldn't expose stuff like that as configure options.

Comment 6

[Paul Biggar]

(In reply to comment #5)
> More options means more ways for users to shoot themselves in the foot "ooh, I
> wonder what *this* button does?" as well as more code paths for us to maintain.
> The fewer options necessary, the better. I think we'd be well-served by just
> having a --disable-jit option at this point.

A real problem is that some (secondary) platforms are supported by nanojit but not Nitro, which means tracejit works but methodjit doesnt. So we need to keep the #ifndefs.

It would be possible to disable methodjit automatically via a configure check, and simply not expose it to users. However, we actually want this, so that we can test it on primary platforms.

So my current thinking is:
 - remove the obscure IC options
 - create --enable-jits, which turns on all JITs supported on the platform.
 - create --disable-jits, which turns off all dynamic-code generation
 - create --internal-only-disable-nitro-jits so that we can test non-Nitro builds on primary platforms.

Comment 7

[Andreas Gal :gal]

We should probably not worry too much about secondary platforms. We take patches for those and in the meantime focus on our 4 platforms (all of which support every JIT we have).

Comment 8

[(not currently active) Ted Mielczarek]

For things that you really just want for testing, if you really want a way to override something, it's fine to have a JS_WHATEVER variable that gets AC_SUBST / AC_DEFINEed, that requires you to pass it to configure as an environment variable to use. Less obvious than a configure option but still usable.

Comment 9

[Sean Stangl [:sstangl]]

JIT options are by default enabled, so only negative configure flags are ever expressed. If you don't specifically want to disable a component, you never need to know about its flag. I do not understand what we gain from eliminating these configuration options, especially if the proposed alternative is to have developers manually editing configuration files.

Is this bug attempting to solve a problem that we have actually encountered? It would be helpful to specifically describe the problem first.