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.

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

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?

(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?

I think #2 is definitely worth trying, as just raising the cap doesn't seem very future-proof. Should I self-assign this bug?

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

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 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.

> 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.

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

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?

> I think that addresses all issues you found with using LazyLink, right? Yep - thanks! I've revised the patch accordingly.

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 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.

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

(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.

Pushed by efaustbmo@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/27054f5883df Link excess lazy builders immediately, instead of throwing them away. (r=h4writer)

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

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.

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).

(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?

> 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 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 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!

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.

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

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