s/Ion/Jit/ where appropriate

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla36
Points:
---

Firefox Tracking Flags

(firefox34 wontfix, firefox35 fixed, firefox36 fixed)

Details

Attachments

(12 attachments, 1 obsolete attachment)

15.20 KB, patch
jandem
: review+
Details | Diff | Splinter Review
42.72 KB, patch
jandem
: review+
Details | Diff | Splinter Review
87.62 KB, patch
jandem
: review+
Details | Diff | Splinter Review
11.96 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.27 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.22 KB, patch
jandem
: review+
Details | Diff | Splinter Review
28.43 KB, patch
jandem
: review+
Details | Diff | Splinter Review
144.04 KB, patch
jandem
: review+
Details | Diff | Splinter Review
25.97 KB, patch
jandem
: review+
Details | Diff | Splinter Review
12.74 KB, patch
jandem
: review+
Details | Diff | Splinter Review
31.18 KB, patch
jandem
: review+
Details | Diff | Splinter Review
347.84 KB, patch
njn
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
I think it's valuable to rename things when their meanings shift, rather than leaving them with their original-but-now-misleading names. Bug 909499 is a good example, but it didn't go far enough: there are other things still with "Ion" in their name that apply to IonMonkey and Baseline, and so should be renamed "Jit".

As per bug 909499, we should consider backporting these changes to Aurora, Beta, and maybe ESR (though ESR backporting apparently didn't happen in bug 909499).
Assignee

Comment 1

5 years ago
I will post a number of separate patches to make reviewing easier, but I plan to merge them before landing to make backporting easier.
Assignee

Comment 2

5 years ago
Attachment #8526376 - Flags: review?(jdemooij)
Assignee

Comment 3

5 years ago
BTW, here's my current list of things to change

Yes:

- IonExecStatus [done]
- IonContext
- IonAllocPolicy / OldIonAllocPolicy
- GetTopIonJSScript
- DestroyIonScripts
- TraceIonScripts
- IonFrames.{h,cpp}, IonFrames-inl.h
- IonCaches.h

No:

- hasIonReturnOverride / ionReturnOverride
- clearIonCompiledOrInlined
Assignee

Comment 4

5 years ago
One could argue that this file should be renamed jit/AllocPolicy.h and the type
renamed as jit::AllocPolicy. But I think that would make the code harder to
read.
Attachment #8526390 - Flags: review?(jdemooij)
Assignee

Updated

5 years ago
Attachment #8526390 - Attachment is obsolete: true
Attachment #8526390 - Flags: review?(jdemooij)
Assignee

Comment 6

5 years ago
I did case-insensitive searches for "ioncontext" and "ion context", so I think
I got them all.
Attachment #8526506 - Flags: review?(jdemooij)
Assignee

Comment 7

5 years ago
> - GetTopIonJSScript
> - DestroyIonScripts
> - TraceIonScripts

Actually, should IonScript be changed to JitScript too?
(In reply to Nicholas Nethercote [:njn] from comment #7)
> Actually, should IonScript be changed to JitScript too?

Nope, IonScript is only used for Ion code. Baseline has BaselineScript.
(In reply to Nicholas Nethercote [:njn] from comment #3)
> - IonCaches.h

IonCaches is also Ion-only.
Comment on attachment 8526376 [details] [diff] [review]
Rename IonExecStatus as JitExecStatus

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

::: js/src/jit/Ion.h
@@ +106,2 @@
>  
>      // The method call succeeed and returned a value.

Nit: fix the typo (succeeded), while you're here. Also the previous comments mention Ion/IonMonkey, that could be "the Jit" or "Baseline/Ion" or something.
Attachment #8526376 - Flags: review?(jdemooij) → review+
Assignee

Comment 11

5 years ago
What about all the Ion*FrameLayout types in IonFrames.h -- can they be changed to Jit*FrameLayout?
Assignee

Comment 12

5 years ago
One could argue that GetTopJSScript() or GetTopFrameJSScript() are better
names. I'm open to suggestions.
Attachment #8526717 - Flags: review?(jdemooij)
(In reply to Nicholas Nethercote [:njn] from comment #11)
> What about all the Ion*FrameLayout types in IonFrames.h -- can they be
> changed to Jit*FrameLayout?

No, some of them are still related to Ion stack frames.

http://dxr.mozilla.org/mozilla-central/search?q=%2Bderived%3Ajs%3A%3Ajit%3A%3AIonCommonFrameLayout :
> 362 	class IonJSFrameLayout : public IonCommonFrameLayout

JitFrameLayout

> 416 	class IonEntryFrameLayout : public IonJSFrameLayout

EntryFrameLayout

> 424 	class IonRectifierFrameLayout : public IonJSFrameLayout
> 433 	class IonUnwoundRectifierFrameLayout : public IonRectifierFrameLayout

keep them.

> 480 	class IonExitFrameLayout : public IonCommonFrameLayout

ExitFrameLayout

> 816 	class IonBaselineStubFrameLayout : public IonCommonFrameLayout 

BaselineStubFrameLayout
Attachment #8526400 - Flags: review?(jdemooij) → review+
Comment on attachment 8526506 [details] [diff] [review]
Rename IonContext as JitContext

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

::: js/src/vm/Debugger.cpp
@@ +1824,5 @@
>  {
>      AutoSuppressProfilerSampling suppressProfilerSampling(cx);
>  
>      {
> +        jit::JitContext jctx(cx, nullptr);

Thanks for renaming those variables as well, very nice.
Attachment #8526506 - Flags: review?(jdemooij) → review+
Attachment #8526717 - Flags: review?(jdemooij) → review+
Attachment #8526718 - Flags: review?(jdemooij) → review+
Comment on attachment 8526719 [details] [diff] [review]
Rename TraceIonScripts as TraceJitScripts

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

Beautiful.
Attachment #8526719 - Flags: review?(jdemooij) → review+
Comment on attachment 8526758 [details] [diff] [review]
Rename IonFrames{.h,-inl.h,.cpp} as JitFrames{.h,-inl.h,.cpp}

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

::: js/src/jit/JitFrames-inl.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_IonFrames_inl_h
>  #define jit_IonFrames_inl_h

Nit: also rename those.
Attachment #8526758 - Flags: review?(jdemooij) → review+
Assignee

Updated

5 years ago
Duplicate of this bug: 1006598
Assignee

Comment 21

5 years ago
More specifically:

- IonCommonFrameLayout              --> CommonFrameLayout
- IonJSFrameLayout                  --> JitFrameLayout
- IonEntryFrameLayout               --> EntryFrameLayout
- IonRectifierFrameLayout           --> RectifierFrameLayout
- IonUnwoundRectifierFrameLayout    --> (unchanged)
- IonExitFooterFrame                --> ExitFooterFrame
- IonExitFrameLayout                --> ExitFrameLayout
- IonNativeExitFrameLayout          --> NativeExitFrameLayout
- IonOOLNativeExitFrameLayout       --> OOLNativeExitFrameLayout
- IonOOLPropertyOpExitFrameLayout   --> OOLPropertyOpExitFrameLayout
- IonOOLProxyExitFrameLayout        --> OOLProxyExitFrameLayout
- IonDOMExitFrameLayout             --> DOMExitFrameLayout
- IonDOMMethodExitFrameLayout       --> DOMMethodExitFrameLayout
- IonDOMMethodExitFrameLayoutTraits --> DOMMethodExitFrameLayoutTraits
- IonBaselineStubFrameLayout        --> BaselineStubFrameLayout
- maybeIonFrameRecovery             --> maybeJitFrameRecovery
- CopyIonJSFrameArgs                --> CopyJitFrameArgs
- removeIonFrameRecovery            --> removeJitFrameRecovery
- markIonRecovery                   --> markJitRecovery
Attachment #8527374 - Flags: review?(jdemooij)
Assignee

Comment 23

5 years ago
Attachment #8527396 - Flags: review?(jdemooij)
Assignee

Comment 25

5 years ago
And that's the end of the patches.

One thing to think about: the current dev cycle finishes in a few days, on Nov 28. Is it worth trying to get this in (and backported) before then, or waiting until the start of the next cycle?
(In reply to Nicholas Nethercote [:njn] from comment #21)
> Created attachment 8527374 [details] [diff] [review]
> Rename Lots of IonFooFrameLayout and related things, mostly to FooFrameLayout
> 
> More specifically:
> 
> - IonCommonFrameLayout              --> CommonFrameLayout
> - IonJSFrameLayout                  --> JitFrameLayout
> - IonEntryFrameLayout               --> EntryFrameLayout
> - IonRectifierFrameLayout           --> RectifierFrameLayout

I am not sure we have rectifier frame for baseline calls, do we?

> - IonOOLNativeExitFrameLayout       --> OOLNativeExitFrameLayout
> - IonOOLPropertyOpExitFrameLayout   --> OOLPropertyOpExitFrameLayout
> - IonOOLProxyExitFrameLayout        --> OOLProxyExitFrameLayout
> - IonDOMExitFrameLayout             --> DOMExitFrameLayout
> - IonDOMMethodExitFrameLayout       --> DOMMethodExitFrameLayout
> - IonDOMMethodExitFrameLayoutTraits --> DOMMethodExitFrameLayoutTraits

All the OOL and DOM are specific to Ion, baseline is not making such frames.

> - maybeIonFrameRecovery             --> maybeJitFrameRecovery

Frame recovery is only an Ion thing, this is not for any Jit.  The Frame recovery is here to bailout-ahead of time for things which either have to be read (before mutation / when requested)

> - removeIonFrameRecovery            --> removeJitFrameRecovery
> - markIonRecovery                   --> markJitRecovery

Same here.
(In reply to Nicolas B. Pierron [:nbp] from comment #26)
> I am not sure we have rectifier frame for baseline calls, do we?

Baseline and Ion use the same calling convention and both need rectifier frames.

(In reply to Nicolas B. Pierron [:nbp] from comment #26)
> All the OOL and DOM are specific to Ion, baseline is not making such frames.

At least the DOM ones are not inherently Ion-specific, Baseline could use those frames too at some point. But yeah maybe we should keep the Ion prefix there...
Comment on attachment 8527376 [details] [diff] [review]
Rename IonMacroAssembler.h as MacroAssembler.h

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

Awesome.
Attachment #8527376 - Flags: review?(jdemooij) → review+
Attachment #8527396 - Flags: review?(jdemooij) → review+
Comment on attachment 8527397 [details] [diff] [review]
Rename callIon as callJit, and likewise with some similar cases

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

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +1296,1 @@
>      void callIonFromAsmJS(Register callee);

FWIW, I'm renaming this one in bug 1103056 and making the code that uses it work for Baseline code with a little work. Glad I found out about it thanks to this bug ;)
Attachment #8527397 - Flags: review?(jdemooij) → review+
Comment on attachment 8527374 [details] [diff] [review]
Rename Lots of IonFooFrameLayout and related things, mostly to FooFrameLayout

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

Looks great. r=me with the issues in comment 26 and comment 27 fixed.
Attachment #8527374 - Flags: review?(jdemooij) → review+
(In reply to Nicholas Nethercote [:njn] from comment #25)
> One thing to think about: the current dev cycle finishes in a few days, on
> Nov 28. Is it worth trying to get this in (and backported) before then, or
> waiting until the start of the next cycle?

Either way is fine with me; I don't expect serious issues. 36 is not an ESR, that makes it a bit less urgent, but having this on 35 and 36 would be nice.
Assignee

Comment 32

5 years ago
> Looks great. r=me with the issues in comment 26 and comment 27 fixed.

Just to clarify: you want me to undo the following changes?

> - IonOOLNativeExitFrameLayout       --> OOLNativeExitFrameLayout
> - IonOOLPropertyOpExitFrameLayout   --> OOLPropertyOpExitFrameLayout
> - IonOOLProxyExitFrameLayout        --> OOLProxyExitFrameLayout
> - IonDOMExitFrameLayout             --> DOMExitFrameLayout
> - IonDOMMethodExitFrameLayout       --> DOMMethodExitFrameLayout
> - IonDOMMethodExitFrameLayoutTraits --> DOMMethodExitFrameLayoutTraits
> - maybeIonFrameRecovery             --> maybeJitFrameRecovery
> - removeIonFrameRecovery            --> removeJitFrameRecovery
> - markIonRecovery                   --> markJitRecovery

Thanks.
Flags: needinfo?(jdemooij)
(In reply to Nicholas Nethercote [:njn] from comment #32)
> Just to clarify: you want me to undo the following changes?

Correct.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/b5136e8cd58e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee

Comment 37

5 years ago
This is the final patch that landed, which is the combination of all the
reviewed patches.
Assignee

Comment 38

5 years ago
Comment on attachment 8529457 [details] [diff] [review]
s/Ion/Jit/ where appropriate

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: This patch just did a ton of trivial renaming of identifiers within the JS engine. It is about to ride the trains onto Aurora; once that happens it would be good to backport it to Beta so that any subsequent fixes within the JITs that might need to be backported to Beta are less likely to have conflicts. Bug 909499 is a similar case that establishes precedent.

[Describe test coverage new/current, TBPL]: currently on m-c without problem.

[Risks and why]: Minimal. It's just renaming. And it'll be very early in the Beta cycle.

[String/UUID change made/needed]: None.
Attachment #8529457 - Flags: review+
Attachment #8529457 - Flags: approval-mozilla-beta?
Note that this is a request for Beta 35. This is not the type of fix that we would normally backport for ESR. We should weight the cost benefit for ESR as it will presumably make backporting sec fixes easier and ESR31 will live for ~7 more months.
Comment on attachment 8529457 [details] [diff] [review]
s/Ion/Jit/ where appropriate

Take this for early beta - hopefully we'll shake out any potential issues immediately.
Attachment #8529457 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Goes without saying that the patch has conflicts galore with Beta. Nick, can you take care of the rebase? Feel free to just push it when ready to avoid bitrot.
Flags: needinfo?(n.nethercote)
Assignee

Comment 42

5 years ago
> Nick, can you take care of the rebase? Feel free to just push it when ready to avoid
> bitrot.

Sure thing, though I won't get to it until next week.
Flags: needinfo?(n.nethercote)
You need to log in before you can comment on or make changes to this bug.