Closed Bug 931039 Opened 6 years ago Closed 6 years ago

Rename IonRuntime/IonCompartment to JitRuntime/JitCompartment

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
Big patch but I just did a mass search-and-replace and fixed the #include ordering.
Attachment #822370 - Flags: review?(hv1989)
Comment on attachment 822370 [details] [diff] [review]
Patch

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

Nice work!

::: js/src/jit/Ion.cpp
@@ +2570,5 @@
>  void
>  AutoFlushCache::updateTop(uintptr_t p, size_t len)
>  {
>      IonContext *ictx = MaybeGetIonContext();
> +    JitRuntime *irt = (ictx != nullptr) ? ictx->runtime->jitRuntime() : nullptr;

irt = ionRunTime. So this variablen name should probably also get updated to jrt or rt?

::: js/src/jit/IonMacroAssembler.h
@@ +649,5 @@
>  
>          JSRuntime *runtime = GetIonContext()->runtime;
>          IonCode *preBarrier = (type == MIRType_Shape)
> +                              ? runtime->jitRuntime()->shapePreBarrier()
> +                              : runtime->jitRuntime()->valuePreBarrier();

Aangezien JSRuntime niet wordt gebruikt, kunnen we dit aanpassen naar:

JitRuntime *rt = GetIonContext()->runtime->jitRuntime();
IonCode *preBarrier = (type == MIRType_Shape)? rt->shapePreBarrier() : rt->valuePreBarrier();

::: js/src/jit/arm/Bailouts-arm.cpp
@@ +12,5 @@
>  
>  using namespace js;
>  using namespace js::jit;
>  
>  #if 0

legacy code?

@@ +25,5 @@
>                   sizeof(BailoutStack) +
>                   sizeof(uintptr_t));
>  
>  #endif
>  #if 0

legacy code?
Maybe good to get Marty look at this in follow-up bug and probably remove?

::: js/src/jit/x64/Trampoline-x64.cpp
@@ +29,5 @@
>   *               Value *vp)
>   *   ...using standard x64 fastcall calling convention
>   */
>  IonCode *
> +JitRuntime::generateEnterJIT(JSContext *cx, EnterJitType type)

Feels very wrong to get IonCode from jitRuntime->generateEnterJIT that is used to enter baseline and IM. Is it the idea to change IonCode to JitCode also?

As extra question. I assume not all generate* functions also apply to baseline? Should we rename some to include "IM" to make it clear those are only used and valid for IM?

::: js/src/jsworkers.cpp
@@ +125,5 @@
>          return;
>  
>      WorkerThreadState &state = *rt->workerThreadState;
>  
> +    jit::JitCompartment *ion = compartment->jitCompartment();

s/ion/.../
Something related to compartment? jitCompartment or something.
Attachment #822370 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f00ded19bfee

Thanks, great comments.

(In reply to Hannes Verschore [:h4writer] from comment #2)
> irt = ionRunTime. So this variablen name should probably also get updated to
> jrt or rt?

Done.

> Aangezien JSRuntime niet wordt gebruikt, kunnen we dit aanpassen naar:
> 
> JitRuntime *rt = GetIonContext()->runtime->jitRuntime();
> IonCode *preBarrier = (type == MIRType_Shape)? rt->shapePreBarrier() :
> rt->valuePreBarrier();

Done. Though it still didn't fit on one line (102 characters or so).

> legacy code?

> legacy code?
> Maybe good to get Marty look at this in follow-up bug and probably remove?

Filed bug 931732.

> Feels very wrong to get IonCode from jitRuntime->generateEnterJIT that is
> used to enter baseline and IM. Is it the idea to change IonCode to JitCode
> also?

Definitely, and also s/IonContext/JitContext and s/IonFrame*/JitFrame*. One step at a time :)

> As extra question. I assume not all generate* functions also apply to
> baseline? Should we rename some to include "IM" to make it clear those are
> only used and valid for IM?

Yeah we should probably use generateIon* for the Ion-only trampolines..

> s/ion/.../
> Something related to compartment? jitCompartment or something.

Done.
https://hg.mozilla.org/mozilla-central/rev/f00ded19bfee
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Target Milestone: mozilla28 → mozilla27
You need to log in before you can comment on or make changes to this bug.