Consider removing the TI flag and enabling it unconditionally

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jandem, Assigned: bhackett)

Tracking

unspecified
mozilla31
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
The prefs/flags we have for the JITs are extremely useful for testing/fuzzing/triage, and we really want to keep them. However, the TI pref isn't all that useful anymore (a lot has been moved into Ion anyway) and I don't think turning off TI noticeably improves memory usage these days.  Also note that we're working on enabling TI and the JITs everywhere (bug 929374, bug 939562).

This would simplify some other parts of the VM as well (for instance, an object's "initialized length" depends on whether TI is enabled). And when we do this we can also remove the --no-ti run from jit_test.py --tbpl, saving resources.

There's one problem I can think of: TI OOMs currently disable TI. I don't know if that's still necessary though.

Brian, what do you think?
(Assignee)

Comment 1

5 years ago
TI ooms used to disable TI because the constraint graph got into an inconsistent state we couldn't resolve.  The condtraint graph doesn't exist anymore so we should be able to get rid of the type nuking.  Though it would be good if the TI methods generally stayed infallible and handled failure by marking things unknown, canceling any active compilation, etc.
(Assignee)

Updated

5 years ago
Depends on: 980630
(Assignee)

Comment 2

5 years ago
Created attachment 8392239 [details] [diff] [review]
patch
Assignee: nobody → bhackett1024
Attachment #8392239 - Flags: review?(jdemooij)
(Reporter)

Comment 3

5 years ago
Comment on attachment 8392239 [details] [diff] [review]
patch

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

Awesome!

Please update this comment in ObjectImpl.h as well:

 ... When type inference is disabled,
 * the initialized length always equals the capacity. When inference is
 * enabled, the initialized length is some value less than or equal to both the
 * object's length and the object's capacity.
 *
 * With inference enabled, ...

::: js/src/jsinfer.cpp
@@ -1875,5 @@
> -TypeZone::init(JSContext *cx)
> -{
> -    if (!cx ||
> -        !cx->runtime()->options().typeInference() ||
> -        !cx->runtime()->jitSupportsFloatingPoint)

I think we rely on this to disable Ion compilation if we've no FPU (--no-fpu shell flag should trigger assertion failures on x86). We could add |cx->runtime()->jitSupportsFloatingPoint| to IsIonEnabled.
Attachment #8392239 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 4

5 years ago
gkw/decoder: this patch will remove the --no-ti and --ti shell flags.
https://hg.mozilla.org/mozilla-central/rev/2044699c3b05
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(In reply to Jan de Mooij [:jandem] from comment #4)
> gkw/decoder: this patch will remove the --no-ti and --ti shell flags.

Jan, thanks for the headsup. I have removed support for --no-ti from the fuzzing harness as of fuzzing rev eb80198dfc4a.
You need to log in before you can comment on or make changes to this bug.