Reduce JIT-related configure-time options

UNCONFIRMED
Unassigned

Status

()

UNCONFIRMED
8 years ago
4 years ago

People

(Reporter: paul.biggar, Unassigned)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

8 years ago
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.)
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 633543
(Reporter)

Comment 2

8 years ago
This isn't a dup. Bug 633543 is about run-time flags for the shell binary. This is about configure-time options and #ifdefs.
Status: RESOLVED → UNCONFIRMED
Ever confirmed: false
Resolution: DUPLICATE → ---
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.
(Reporter)

Comment 4

8 years ago
(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.
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.
(Reporter)

Comment 6

8 years ago
(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

8 years ago
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).
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.
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.
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.