Closed Bug 1047346 Opened 10 years ago Closed 10 years ago

Delay linking for scripts that are already on the stack

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(1 file, 4 obsolete files)

Linking means invalidating the same scripts from the stack. This is sometimes suboptimal, since linking can happen anytime. This bug is about delaying the linking in this case. Delaying until we will start that script. In that case we will have an improved IonScript and will run it. So there is an immediate gain in linking at that point (though as result we will invalidate all other scripts on the stack. But this already ameliorate the situation a bit).
Attached patch bugXXX-link-at-start (obsolete) — Splinter Review
Took longer than expected to finish this. But finally.

So we now have a "linkstub" which can be set on the JSScript jit entrypoint. This stub will link the waiting ionbuilder and start the just created jit code immediately.
Assignee: nobody → hv1989
Attachment #8466158 - Flags: review?(jdemooij)
Blocks: 911738
Attached patch bugXXX-link-at-start (obsolete) — Splinter Review
About the two questions you had:

1) Is JitFrame_FakeIonJS really necessary?

No. This was just easier since I used callVM. I removed the use of FakeIon and changed callVM to callWithABI

2) Why using a LinkStub and not having a new entry to the IonScript which has this code.

There is no real advantage between the two currently.
I think I just was thinking about lateron we might want to always lazy link. Using a LinkStub this is easier, since we don't need any IonScript present already.
Attachment #8466158 - Attachment is obsolete: true
Attachment #8466158 - Flags: review?(jdemooij)
Attachment #8484306 - Flags: review?(jdemooij)
Attached patch bugXXX-link-at-start (obsolete) — Splinter Review
Removed some debugging cruft
Attachment #8484306 - Attachment is obsolete: true
Attachment #8484306 - Flags: review?(jdemooij)
Attachment #8484979 - Flags: review?(jdemooij)
Comment on attachment 8484979 [details] [diff] [review]
bugXXX-link-at-start

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

Sorry for the delay!

::: js/src/jit/CodeGenerator.cpp
@@ +21,1 @@
>  #include "builtin/Eval.h"

Nit: fix include order

@@ +5499,5 @@
>      return code;
>  }
>  
> +uint8_t *
> +FinishCompile(JSContext *cx)

I think it'd be nicer to have this function in Ion.cpp

@@ +5519,5 @@
> +    // Remove from list.
> +    builder->remove();
> +
> +    if (CodeGenerator *codegen = builder->backgroundCodegen()) {
> +        js::TraceLogger *logger = TraceLoggerForMainThread(cx->asJSContext()->runtime());

Nit: just cx->runtime()

@@ +5534,5 @@
> +        bool success;
> +        {
> +            // Release the helper thread lock and root the compiler for GC.
> +            //AutoUnlockHelperThreadState unlock;
> +            success = codegen->link(cx, builder->constraints());

I think you can remove the comments and the block, and just do:

if (!codegen->link(...)) {

@@ +5541,5 @@
> +        if (!success) {
> +            // Silently ignore OOM during code generation. The caller is
> +            // InvokeInterruptCallback, which always runs at a
> +            // nondeterministic time. It's not OK to throw a catchable
> +            // exception from there.

Comment is wrong.

::: js/src/jit/Ion.cpp
@@ +1813,5 @@
> +                        continue;
> +
> +                    JSScript *script = it.script();
> +                    if (builder->script() == script)
> +                        onStack = true;

Nit: add a break; here.

@@ +1814,5 @@
> +
> +                    JSScript *script = it.script();
> +                    if (builder->script() == script)
> +                        onStack = true;
> +                }

And here:

if (onStack)
    break;

::: js/src/jit/IonCode.h
@@ +299,5 @@
>      // If non-null, the list of AsmJSModules
>      // that contain an optimized call directly into this IonScript.
>      Vector<DependentAsmJSModuleExit> *dependentAsmJSModules;
>  
> +    IonBuilder *waitingBuilder_;

This needs a comment. I'd also prefer pendingBuilder_?

Can we assert in the destructor that it's null?

::: js/src/jit/JitCompartment.h
@@ +193,5 @@
>      JitCode *mallocStub_;
>      JitCode *freeStub_;
>  
> +    // Thunk to link the current called ionscript.
> +    JitCode *linkStub_;

// Thunk called to finish compilation of an IonScript.

::: js/src/vm/HelperThreads.cpp
@@ +453,5 @@
> +   helperLock(nullptr),
> +#ifdef DEbUG
> +   lockOwner(nullptr),
> +#endif
> +    consumerWakeup(nullptr),

Nit: indentation looks different here.

::: js/src/vm/HelperThreads.h
@@ +57,5 @@
>  
>      // Ion compilation worklist and finished jobs.
>      IonBuilderVector ionWorklist_, ionFinishedList_;
>  
> +    IonBuilderList ionWaitingForLinkingList_;

Needs a comment.
Attachment #8484979 - Flags: review?(jdemooij)
Attached patch bugXXX-link-at-start (obsolete) — Splinter Review
Address review comments + I renamed most names.
Attachment #8484979 - Attachment is obsolete: true
Attachment #8486307 - Flags: review?(jdemooij)
Sorry I have no idea what went wrong here. But this should be the new one.
Attachment #8486307 - Attachment is obsolete: true
Attachment #8486307 - Flags: review?(jdemooij)
Attachment #8487157 - Flags: review?(jdemooij)
Comment on attachment 8487157 [details] [diff] [review]
bugXXX-link-at-start

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

Great, thanks for addressing the nits.

::: js/src/jit/CodeGenerator.cpp
@@ +5499,5 @@
>  
>      return code;
>  }
>  
> +

Nit: can remove this blank line.

::: js/src/jit/Ion.cpp
@@ +570,4 @@
>  {
>      ExecutionMode executionMode = builder->info().executionMode();
>  
> +    // Clean the references to the waitingIonBuilder, if we just finished it.

Nit: update the comment s/waiting/pending/
Attachment #8487157 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/ab267884c5ae
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8487157 [details] [diff] [review]
bugXXX-link-at-start

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

This is failing on ARM simulator. I did probably something wrong in CodeGen. Can you look if it is correct?
Attachment #8487157 - Flags: feedback?(mrosenberg)
Comment on attachment 8487157 [details] [diff] [review]
bugXXX-link-at-start

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

Not entirely sure what the correct feedback setting would be for "found the bug", but I'm assuming feedback - is appropriate?

::: js/src/jit/CodeGenerator.cpp
@@ +5521,5 @@
> +    masm.loadJSContext(temp0);
> +    masm.passABIArg(temp0);
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, LazyLinkTopActivation));
> +    masm.leaveExitFrame();
> +    masm.retn(Imm32(sizeof(IonExitFrameLayout)));

Uh-oh.  retn isn't actually return /per-se/ on arm.  masm.retn(20) generates ldr pc, [sp],20, translated to C, it is:
   tmp = *sp; sp += 20; pc = tmp; 
The problem with this is it is one of two valid return mechanisms, which depends on the actual properties of the function in question. retn only works if the return address was pushed on the stack, which is done by the *callee*, not the caller.  (IM makes up its own abi, and pushes it before the call, it is kind of crazy, and totally deprecated in ARMv8).  It appears as if you have masm.call(&call) explicitly to put an address on the stack.  If this is the case, I'd recommend doing it with masm.push(pc), or something like that (on arm), then just incrementing sp (you're already manually incrementing it in leaveExitFrame), and finally masm.jump(ReturnReg). If there was some other reason that you have |masm.call(&call), then my guess would be that you don't actually need it on arm?

anyhow, if you want to keep the control flow the same, there is masm.callIonHalfPush.  Currently, it only takes a register as an argument, but it can be duplicated + made to take a Label* as well.
Attachment #8487157 - Flags: feedback?(mrosenberg) → feedback-
Depends on: 1067064
This was backed out due to ARM failures, but I posted this in the wrong bug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/85a0a388a339
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1067984
(In reply to Hannes Verschore [:h4writer], pto till 22 September from comment #12)
> This was backed out due to ARM failures, but I posted this in the wrong bug.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/85a0a388a339

Ok I was totally wrong. This bug hasn't been backed out.
(In reply to Marty Rosenberg [:mjrosenb] from comment #11)
> Comment on attachment 8487157 [details] [diff] [review]
> bugXXX-link-at-start
> 
> Review of attachment 8487157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not entirely sure what the correct feedback setting would be for "found the
> bug", but I'm assuming feedback - is appropriate?
> 
> ::: js/src/jit/CodeGenerator.cpp
> @@ +5521,5 @@
> > +    masm.loadJSContext(temp0);
> > +    masm.passABIArg(temp0);
> > +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, LazyLinkTopActivation));
> > +    masm.leaveExitFrame();
> > +    masm.retn(Imm32(sizeof(IonExitFrameLayout)));
> 
> Uh-oh.  retn isn't actually return /per-se/ on arm.  masm.retn(20) generates
> ldr pc, [sp],20, translated to C, it is:
>    tmp = *sp; sp += 20; pc = tmp; 
> The problem with this is it is one of two valid return mechanisms, which
> depends on the actual properties of the function in question. retn only
> works if the return address was pushed on the stack, which is done by the
> *callee*, not the caller.  (IM makes up its own abi, and pushes it before
> the call, it is kind of crazy, and totally deprecated in ARMv8).  It appears
> as if you have masm.call(&call) explicitly to put an address on the stack. 
> If this is the case, I'd recommend doing it with masm.push(pc), or something
> like that (on arm), then just incrementing sp (you're already manually
> incrementing it in leaveExitFrame), and finally masm.jump(ReturnReg). If
> there was some other reason that you have |masm.call(&call), then my guess
> would be that you don't actually need it on arm?
> 
> anyhow, if you want to keep the control flow the same, there is
> masm.callIonHalfPush.  Currently, it only takes a register as an argument,
> but it can be duplicated + made to take a Label* as well.

I tried to use "masm.callIonHalfPush" like you suggested. But I have no idea on how to transform it to use a Label instead of register. Can you create that function for me? I think I should be able to do the rest.
Flags: needinfo?(mrosenberg)
Depends on: 1079806
Flags: needinfo?(mrosenberg)
Set status of this bug correctly. This has landed in FF35 and wasn't backed out. So I'm marking resolved.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.