1285890 - Remove locking on lazy linking

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Hannes Verschore [:h4writer]]

I added the locking when trying to find the root cause of crashes during lazy linking. I eventually found the culprit, but never removed the locking. Lets remove it.

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Patch

Another reason the locking isn't needed anymore, is because the ionLazyLinkList doesn't need locking.

This patch removes this locking.

FinishOffThreadBuilder was hard to follow, since it has code that should only happend on the main thread and code that was shared. That is why I spit it into two functions: FinishBuilderOnThread and FinishBuilderOffThread. The OffThread version can only be used in very specific cases, which it guards for.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8769616 [details] [diff] [review]
Patch

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

Nice asserts.

::: js/src/jit/Ion.cpp
@@ +476,5 @@
> +void
> +jit::FinishBuilderOnThread(JSRuntime* runtime, IonBuilder* builder)
> +{
> +    MOZ_ASSERT(TlsPerThreadData.get()->runtimeFromMainThread(),
> +            "Should only be mutated by the main thread.");

Nit: fix indentation

@@ +485,5 @@
> +    {
> +        builder->script()->baselineScript()->removePendingIonBuilder(builder->script());
> +    }
> +
> +    // If the builder is still in one of the helper thread list, then remove it.

Nit: lists

Comment 3

[Jon Coppeard (:jonco)]

Hannes, this patch doesn't seem to apply any more.  Is it still needed?

Comment 4

[Hannes Verschore [:h4writer]]

This is still in my "I want to land this" list. Though jit-tests was upset and I do know why and it isn't an easy fix. I will come back to this hopefully.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 6

[Jan de Mooij [:jandem]]

A lot has changed, but part of this still applies: there's still an unnecessary AutoLockHelperThreadState in LinkIonScript. The second one is necessary today because of the off-thread IonFree task.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1285890 - Remove unnecessary AutoLockHelperThreadState in LinkIonScript. r?jonco!

Both the lazy link list and the pendingIonCompileTask are only used on the main thread.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8b6edbc1963
Remove unnecessary AutoLockHelperThreadState in LinkIonScript. r=jonco

Comment 9

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/c8b6edbc1963