1102538 - s/Ion/Jit/ where appropriate

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicholas Nethercote [inactive]]

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).

Comment 1

[Nicholas Nethercote [inactive]]

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Rename IonExecStatus as JitExecStatus

Comment 3

[Nicholas Nethercote [inactive]]

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

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Rename IonAllocPolicy as JitAllocPolicy

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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Rename IonAllocPolicy as JitAllocPolicy

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Rename IonContext as JitContext

I did case-insensitive searches for "ioncontext" and "ion context", so I think
I got them all.

Comment 7

[Nicholas Nethercote [inactive]]

> - GetTopIonJSScript
> - DestroyIonScripts
> - TraceIonScripts

Actually, should IonScript be changed to JitScript too?

Comment 8

[Jan de Mooij [:jandem]]

(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.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> - IonCaches.h

IonCaches is also Ion-only.

Comment 10

[Jan de Mooij [:jandem]]

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.

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Rename GetTopIonJSScript as GetTopJitJSScript

One could argue that GetTopJSScript() or GetTopFrameJSScript() are better
names. I'm open to suggestions.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: Rename DestroyIonScripts as DestroyJitScripts

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: Rename TraceIonScripts as TraceJitScripts

Comment 15

[Nicolas B. Pierron [:nbp]]

(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

Comment 16

[Jan de Mooij [:jandem]]

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.

Comment 17

[Jan de Mooij [:jandem]]

Comment on attachment 8526719 [details] [diff] [review]
Rename TraceIonScripts as TraceJitScripts

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

Beautiful.

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: Rename IonFrames{.h,-inl.h,.cpp} as JitFrames{.h,-inl.h,.cpp}

Comment 19

[Jan de Mooij [:jandem]]

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.

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: Rename Lots of IonFooFrameLayout and related things, mostly to FooFrameLayout

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

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: Rename IonMacroAssembler.h as MacroAssembler.h

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: Rename IonLinker.h as Linker.h

Comment 24

[Nicholas Nethercote [inactive]]

Attachment: Rename callIon as callJit, and likewise with some similar cases

Comment 25

[Nicholas Nethercote [inactive]]

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?

Comment 26

[Nicolas B. Pierron [:nbp]]

(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.

Comment 27

[Jan de Mooij [:jandem]]

(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 28

[Jan de Mooij [:jandem]]

Comment on attachment 8527376 [details] [diff] [review]
Rename IonMacroAssembler.h as MacroAssembler.h

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

Awesome.

Comment 29

[Jan de Mooij [:jandem]]

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 ;)

Comment 30

[Jan de Mooij [:jandem]]

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.

Comment 31

[Jan de Mooij [:jandem]]

(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.

Comment 32

[Nicholas Nethercote [inactive]]

> 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.

Comment 33

[Jan de Mooij [:jandem]]

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

Correct.

Comment 34

[Nicholas Nethercote [inactive]]

Try looks good:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cf9f00ba2481

Comment 35

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5136e8cd58e

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5136e8cd58e

Comment 37

[Nicholas Nethercote [inactive]]

Attachment: s/Ion/Jit/ where appropriate

This is the final patch that landed, which is the combination of all the
reviewed patches.

Comment 38

[Nicholas Nethercote [inactive]]

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.

Comment 39

[Lawrence Mandel [:lmandel] (use needinfo)]

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 40

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 42

[Nicholas Nethercote [inactive]]

> 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.

Comment 43

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/releases/mozilla-beta/rev/84689cd678fc