1178834 - Always lazy link ion code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

We currently only lazy link for code that gets recompiled and is still on the stack. Remove this restriction and always lazy link.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: always-lazy-linking

Comment 2

[Hannes Verschore [:h4writer]]

review ping?

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8627768 [details] [diff] [review]
always-lazy-linking

Clearing review flag for now per our IRC conversation.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: always-lazy-linking WIP

Adjusted patch to save pendingIonBuilder in baseline. Passes jittests but gonna double check everything tonight for correctness.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: always-lazy-linking

Now the timeouts using the "onIonCompilation" are also fixed.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8641534 [details] [diff] [review]
always-lazy-linking

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

Nice!

The patch file is a bit weird (some files are duplicated) and I can't add this comment to BaselineJIT.cpp, but in BaselineScript::Destroy, can we assert we don't have a pending builder? I think we should have finished all off-thread compilations at that point.

::: js/src/jit/BaselineIC.cpp
@@ +137,5 @@
>          return true;
>      }
>  
>      if (isLoopEntry) {
> +        // Check if the jitcode still need to get linked and do this

Nit: needs

@@ +145,5 @@
> +            if (!script->hasIonScript())
> +                return true;
> +            if (script->ionScript()->osrPc() != pc)
> +                return true;
> +        }

Can you move this code to CanEnterAtBranch? I think that's nice because both CanEnter and CanEnterAtBranch will have similar LazyLink code then, and I think you can remove the osrPc check because CanEnterAtBranch already checks that too.

::: js/src/jit/Ion.cpp
@@ +466,5 @@
>  void
>  jit::FinishOffThreadBuilder(JSContext* cx, IonBuilder* builder)
>  {
>      // Clean the references to the pending IonBuilder, if we just finished it.
> +    if (builder->script()->hasBaselineScript() &&

Aren't we guaranteed to have a BaselineScript here? Would be nice if we could remove this check to make this clear.

@@ -1785,5 @@
>      JitCompartment* ion = cx->compartment()->jitCompartment();
>      if (!ion)
>          return;
>  
> -    LifoAlloc* debuggerAlloc = cx->new_<LifoAlloc>(TempAllocator::PreferredLifoChunkSize);

Nice code removal :)

::: js/src/jsscript.h
@@ +965,5 @@
>      js::HeapPtrFunction function_;
>      js::HeapPtrObject   enclosingStaticScope_;
>  
> +    /*
> +     * Information attached by Ion for sequential mode execution.

Nit: remove the "for sequential execution" part, it's no longer relevant since the PJS removal.

@@ +974,3 @@
>      js::jit::IonScript* ion;
> +
> +    /* Information attached by Baseline for sequential mode execution. */

Same here.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bbd0d929128

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bbd0d929128