Closed Bug 1102538 Opened 10 years ago Closed 9 years ago

s/Ion/Jit/ where appropriate

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(12 files, 1 obsolete file)

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
n.nethercote
: review+
Details | Diff | Splinter Review
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).
I will post a number of separate patches to make reviewing easier, but I plan to merge them before landing to make backporting easier.
Attachment #8526376 - Flags: review?(jdemooij)
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
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)
Attachment #8526390 - Attachment is obsolete: true
Attachment #8526390 - Flags: review?(jdemooij)
I did case-insensitive searches for "ioncontext" and "ion context", so I think
I got them all.
Attachment #8526506 - Flags: review?(jdemooij)
> - 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+
What about all the Ion*FrameLayout types in IonFrames.h -- can they be changed to Jit*FrameLayout?
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+
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)
Attachment #8527396 - Flags: review?(jdemooij)
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.
> 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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
This is the final patch that landed, which is the combination of all the
reviewed patches.
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)
> 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.