Delay linking for scripts that are already on the stack

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

3 years ago
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).
(Assignee)

Comment 1

3 years ago
Created attachment 8466158 [details] [diff] [review]
bugXXX-link-at-start

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)
(Assignee)

Updated

3 years ago
Blocks: 911738
(Assignee)

Comment 2

3 years ago
Created attachment 8484306 [details] [diff] [review]
bugXXX-link-at-start

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)
(Assignee)

Comment 3

3 years ago
Created attachment 8484979 [details] [diff] [review]
bugXXX-link-at-start

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)
(Assignee)

Comment 5

3 years ago
Created attachment 8486307 [details] [diff] [review]
bugXXX-link-at-start

Address review comments + I renamed most names.
Attachment #8484979 - Attachment is obsolete: true
Attachment #8486307 - Flags: review?(jdemooij)
(Assignee)

Comment 6

3 years ago
Created attachment 8487157 [details] [diff] [review]
bugXXX-link-at-start

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+
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab267884c5ae
https://hg.mozilla.org/mozilla-central/rev/ab267884c5ae
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(Assignee)

Comment 10

3 years ago
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
(Assignee)

Comment 12

3 years ago
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 → ---
(Assignee)

Updated

3 years ago
Depends on: 1067984
(Assignee)

Comment 13

3 years ago
(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.
(Assignee)

Comment 14

3 years ago
(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)
(Assignee)

Updated

3 years ago
Depends on: 1079806
Flags: needinfo?(mrosenberg)
(Assignee)

Comment 15

3 years ago
Set status of this bug correctly. This has landed in FF35 and wasn't backed out. So I'm marking resolved.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.