1282944 - Ion lazy linking algorithm pathologically degenerates on overload.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Eric Faust [:efaust]]

When we have too many functions compiled off thread at once, we throw out the oldest compilation before linking. If there are a bunch of functions that want to be compiled all at once, and this ring of functions is repeatedly called, we thrash in a circle, wasting huge quantities of time.

Comment 1

[Eric Faust [:efaust]]

Michael, we should get the work you've already done reviewed and landed. Can you post the patch you have, and request review?

Comment 2

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

My current patch is just to comment out the while loop introduced in bug 1270108. Should I upload it now, or should we talk to Hannes first about how to properly fix the issue?

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Michael Smith [:mismith] from comment #2)
> My current patch is just to comment out the while loop introduced in bug
> 1270108. Should I upload it now, or should we talk to Hannes first about how
> to properly fix the issue?

Good find!

Since IonBuilders take a lot of space, I wouldn't recommend to remove the cap altogether.

1) We could increase the cap limit. What would work here? I took 100, since we only hit ~10 in our benchmarks and I assumed that we would never hit 100. Now I suspect we don't want to increase it too much.

2) Instead of throwing away the IonBuilders, we could also link all the IonBuilders if we hit the cap. That way we have LazyLinking in normal scenarios and in extreme cases we fall back to normal linking?

Comment 4

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

I think #2 is definitely worth trying, as just raising the cap doesn't seem very future-proof.

Should I self-assign this bug?

Comment 5

[Eric Faust [:efaust]]

Yeah, I think that makes sense. It's your by rights, having found the issue ;)

Comment 6

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: Bug 1282944: Link excess lazy builders immediately instead of throwing them away

This works well for the React benchmark, having the same performance boost (for me) as commenting out the while loop entirely. Any way to test this on a broader set of benchmarks?

Also, I'm unsure whether to make AttachFinishedCompilations fallible if one of the lazy links fails (as I've done in the patch) or to ignore OOMs as in jit::LazyLink.

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8767289 [details] [diff] [review]
Bug 1282944: Link excess lazy builders immediately instead of throwing them away

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

Like mentioned below I think we can just call LazyLink instead.
Can you upload an updated patch using that?

::: js/src/jit/Ion.cpp
@@ +2043,4 @@
>              while (cx->runtime()->ionLazyLinkListSize() > 100) {
>                  jit::IonBuilder* builder = cx->runtime()->ionLazyLinkList().getLast();
> +                if (!LinkBackgroundCodeGen(cx, builder))
> +                    return false;

Before calling this function some things need to happen. Which happen in LazyLink but are not here. I think it would be better to just call "LazyLink"  instead.

Feel free to rename it to "Link" or "LinkIonScript" to make that function have a more general name again.

Also the LazyLink function "cannot fail" as a result we don't need to propagate this state either.

Comment 8

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

> Before calling this function some things need to happen. Which happen in LazyLink but are not here. I think it would be better to just call "LazyLink"  instead.

Which things need to happen?

- We don't need to extract the builder from the script, because we already have the builder to start with.
- FinishOffThreadBuilder already removes the pending builder from the script and the ionLazyLinkList. And AttachFinishedCompilations has its own AutoLockHelperThreadState and AutoEnterAnalysis.
- Calling LazyLink from AttachFinishedCompilations actually deadlocks because of this, and (AFAIK) would require factoring the locks out of LazyLink.

> Also the LazyLink function "cannot fail" as a result we don't need to propagate this state either.

Perhaps it would be better to factor https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.cpp?q=LazyLink&redirect_type=direct#557-565 out into a LinkIonScript function, called from both LazyLink and AttachFinishedCompilations.

Comment 9

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: bug-1282944.patch

I've attached an updated patch to illustrate my suggestion.

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 8768261 [details] [diff] [review]
bug-1282944.patch

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

(In reply to Michael Smith [:mismith] from comment #8)
> > Before calling this function some things need to happen. Which happen in LazyLink but are not here. I think it would be better to just call "LazyLink"  instead.
> Which things need to happen?
> - We don't need to extract the builder from the script, because we already
> have the builder to start with.
> - FinishOffThreadBuilder already removes the pending builder from the script
> and the ionLazyLinkList. And AttachFinishedCompilations has its own
> AutoLockHelperThreadState and AutoEnterAnalysis.

I was actually thinking about AutoEnterAnalysis. Which isn't needed in "AttachFinishedCompilations", but I forgot to remove. 
And I overlooked when writing the previous comment. I stand corrected here.
Like said the AutoEnterAnalysis is actually obsolete. While you are here, can you remove the AutoEnterAnalysis out of AttachFinishedCompilations?

Another issue is that the IonBuilder cannot be present in the ionLazyLink list during linking. That will give problems.
LazyLink already handles this and removes it.

> - Calling LazyLink from AttachFinishedCompilations actually deadlocks
> because of this, and (AFAIK) would require factoring the locks out of
> LazyLink.

We shouldn't keep the lock during the full linking. This blocks all other threads (any kind) from reading the global state. You can add "AutoUnlockHelperThreadState" in the while loop to fix that.

> > Also the LazyLink function "cannot fail" as a result we don't need to propagate this state either.
> 
> Perhaps it would be better to factor
> https://dxr.mozilla.org/mozilla-central/source/js/src/jit/Ion.
> cpp?q=LazyLink&redirect_type=direct#557-565 out into a LinkIonScript
> function, called from both LazyLink and AttachFinishedCompilations.

I think that addresses all issues you found with using LazyLink, right?

Comment 11

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: Bug 1282944: Link excess lazy builders immediately instead of throwing them away

> I think that addresses all issues you found with using LazyLink, right?

Yep - thanks! I've revised the patch accordingly.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8768545 [details] [diff] [review]
Bug 1282944: Link excess lazy builders immediately instead of throwing them away

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

Thanks, good work!

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 8768545 [details] [diff] [review]
Bug 1282944: Link excess lazy builders immediately instead of throwing them away

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

::: js/src/jit/Ion.cpp
@@ +2042,5 @@
>              while (cx->runtime()->ionLazyLinkListSize() > 100) {
>                  jit::IonBuilder* builder = cx->runtime()->ionLazyLinkList().getLast();
> +                RootedScript script(cx, builder->script());
> +
> +                AutoUnlockHelperThreadState unlock(lock);

This doesn't take an argument.

Comment 14

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Which version of the code are you applying this patch to? Both mozilla-central and mozilla-inbound show it taking an argument, as of https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ad05d9cad4#l5.71

Comment 15

[Hannes Verschore [:h4writer]]

(In reply to Michael Smith [:mismith] from comment #14)
> Which version of the code are you applying this patch to? Both
> mozilla-central and mozilla-inbound show it taking an argument, as of
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e5ad05d9cad4#l5.71

Like already mentioned on irc I was on an old branch.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27054f5883df
Link excess lazy builders immediately, instead of throwing them away. (r=h4writer)

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for jsreftest failures: https://treeherder.mozilla.org/logviewer.html#?job_id=31603133&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff20b7b7651

Comment 18

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Ugh, since the ionLazyLinkList contains builders with scripts in multiple compartments, we end up trying to link a script in a different compartment from the current one in our JSContext. https://dxr.mozilla.org/mozilla-central/source/js/src/jsobjinlines.h#138 doesn't like that.

Comment 19

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: Bug 1282944: Link excess lazy builders immediately instead of throwing them away

I've updated the patch. It now passes the browser jsreftest suite on my machine (unfortunately the problem was missed by the js-shell tests).

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Michael Smith [:mismith] from comment #19)
> Created attachment 8769983 [details] [diff] [review]
> Bug 1282944: Link excess lazy builders immediately instead of throwing them
> away
> 
> I've updated the patch. It now passes the browser jsreftest suite on my
> machine (unfortunately the problem was missed by the js-shell tests).

Any chance you can make a minimal test case (using the shell only JS function newGlobal() maybe) that reproduces the issue in the shell?

Comment 21

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: Bug 1282944: Add jit-test for cross-compartment IonBuilder linking

> Any chance you can make a minimal test case (using the shell only JS function newGlobal() maybe) that reproduces the issue in the shell?

I've added one to the jit-tests.

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8770266 [details] [diff] [review]
Bug 1282944: Add jit-test for cross-compartment IonBuilder linking

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

Thanks for adding a test case! I am actually not such a good reviewer for this, redirecting to Hannes.

Comment 23

[Hannes Verschore [:h4writer]]

Comment on attachment 8770266 [details] [diff] [review]
Bug 1282944: Add jit-test for cross-compartment IonBuilder linking

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

Wow nice!

Comment 24

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5a523085ccb

Comment 25

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: Bug 1282944: Add jit-test for cross-compartment IonBuilder linking

Turns out arm runs with --no-threads, which breaks the test case as it disables offThreadCompileScript. I've added a check for this to the test case and will retry.

Comment 26

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43ebf4dbfb38

Comment 27

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=766841d28405

Comment 28

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

Attachment: Bug 1282944: Add jit-test for cross-compartment IonBuilder linking

Attempt #2 to skip the test when running with --no-threads.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/815c7d8dfebe
Link excess lazy builders immediately, instead of throwing them away. (r=h4writer,efaust)

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/815c7d8dfebe