Closed Bug 909499 Opened 6 years ago Closed 6 years ago

Rename js::ion namespace to js::jit

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 - fixed
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

(Whiteboard: [qa-])

Attachments

(4 files)

Similar to bug 902908.

Nicholas, asking you to review since you've done similar large, mechanical changes recently and asked about this (bug 902908 comment 5).

I can build a 32-bit shell with this patch. Will send it to try before landing.

I'd appreciate a quick review here, since these patches bitrot quickly..
Attachment #795620 - Flags: review?(n.nethercote)
With the previous patch we have:

jit::IsEnabled -- to check if Ion is enabled
jit::IsBaselineEnabled -- to check if Baseline is enabled

This patch renames the former to IsIonEnabled.
Attachment #795628 - Flags: review?(n.nethercote)
RyanVM notes that this will make his trunk->esr24 backports quite interesting for the entire lifetime of the esr24 branch.  We should land this a couple cycles ago so it makes esr24.

Failing that, I think we should seriously consider redoing this rename for esr24 as well.  As long as we're careful about it just being renaming, I think it'd be worth it to avoid ten months of backport conflict-fixing.  To be honest, that scares me more than the possibility of screwing something up in a single, concerted renaming backport-session.
I actually just sent a message to js-engine.internals over this (not entirely sure if that was the right forum, though). Note that the s/ion/jit path changes have already made patch backporting more of a manual process.
Comment on attachment 795620 [details] [diff] [review]
Part 1 - Rename js::ion namespace to js::jit

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

Nice that the old and new names have the same number of characters.

As for ESR... we don't want to hold up desirable changes on trunk, and we don't want to make backporting normal changes difficult.  So I agree that backporting this change sounds sensible.

::: js/src/jit/UnreachableCodeElimination.h
@@ -4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -#ifndef jit_UnreachableCodeEliminatjit_h
> -#define jit_UnreachableCodeEliminatjit_h

LOL

::: js/src/jsapi.h
@@ +125,5 @@
>          SCRIPTVECTOR =-16, /* js::AutoScriptVector */
>          NAMEVECTOR =  -17, /* js::AutoNameVector */
>          HASHABLEVALUE=-18, /* js::HashableValue */
> +        IONMASM =     -19, /* js::jit::MacroAssembler */
> +        IONALLOC =    -20, /* js::jit::AutoTempAllocatorRooter */

Should these be renamed JITMASM and JITALLOC?  (Could be done as a follow-up patch.)
Attachment #795620 - Flags: review?(n.nethercote) → review+
Attachment #795628 - Flags: review?(n.nethercote) → review+
Thanks for the very fast reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/77280a2a30b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/492fdffa7ab7

Looks good on Try: https://tbpl.mozilla.org/?tree=Try&rev=bccb54aea767

(In reply to Nicholas Nethercote [:njn] from comment #4)
> Should these be renamed JITMASM and JITALLOC?  (Could be done as a follow-up
> patch.)

Yep, I will post a follow-up patch.
Whiteboard: [leave open]
jandem, will be helping with try runs on this and https://bugzilla.mozilla.org/show_bug.cgi?id=902908 to make sure nothing is broken before we uplift this.

I am not sure why we would track this, an approval request here should do.

Also our next beta goes to build tomorrow morning PT and jandem is aware of that and will help land by then on branches.
Attachment #797180 - Flags: review?(hv1989)
Attachment #797180 - Flags: approval-mozilla-aurora?
Attachment #797181 - Flags: review?(hv1989)
Attachment #797181 - Flags: approval-mozilla-beta?
Comment on attachment 797180 [details] [diff] [review]
Combined patch for Aurora

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

r+ if it compiles, I don't foresee any other issues. This should be fine.
Attachment #797180 - Flags: review?(hv1989) → review+
Comment on attachment 797181 [details] [diff] [review]
Combined patch for Beta

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

r+ if it compiles, I don't foresee any other issues. This should be fine.
Attachment #797181 - Flags: review?(hv1989) → review+
Attachment #797180 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #797181 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-aurora/rev/148464732cfe
https://hg.mozilla.org/releases/mozilla-beta/rev/1f01633c4a02

Resolving - jandem and I agree that comment 5 is better suited for a new bug.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla26
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.