930437 - IonMonkey keeps recompiling the same script with parallel compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Testcase

I ran into this while trying to create a standalone testcase for bug 928318. I don't think it's causing bug 928318's slowness though but I will mark it as blocking that bug for now.

The attached testcase is about 500 ms faster without parallel compilation. With parallel compilation we seem to recompile the same script hundreds of times.

This works fine with Firefox 24. Brian can you take a look? It's probably a regression from the off-thread IonBuilder patches.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Testcase

Let's see if an https URL for jQuery works...

Comment 2

[Brian Hackett [Laid off!]]

I don't see the same behavior you're seeing.  Running a 10.8 x64 opt build I see 24 compilations, with only a few duplicates, and 4 of the compilations are discarded in FinishCompilation (indicating that types found during IonBuilder were invalidated while the script was finished off thread).

Comment 3

[Jan de Mooij [:jandem]]

OK it's caused by this:

      case MIRType_String:
        // Strings are complicated - we don't handle them yet.
        IonSpew(IonSpew_Abort, "String to Int32 not supported yet.");
        return false;

      case MIRType_Object:
        // Objects might be effectful.
        IonSpew(IonSpew_Abort, "Object to Int32 not supported yet.");
        return false;

I don't think we should be aborting here.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

Don't abort during lowering but use a type policy, similar to MToDouble.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #3)
>       case MIRType_String:
>         // Strings are complicated - we don't handle them yet.
>         IonSpew(IonSpew_Abort, "String to Int32 not supported yet.");
>         return false;

If we abort here, then we should have an abort function similar to the one we currently have in IonBuilder, which save the state of the reason.

Comment 6

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e72e6e30170e

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> If we abort here, then we should have an abort function similar to the one
> we currently have in IonBuilder, which save the state of the reason.

We don't want to abort there at all; the patch just removes that code. I think the only place where we want to abort compilation is in IonBuilder and not after running all optimization passes.

Comment 7

[Jan de Mooij [:jandem]]

Correct URL this time:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85b01def9e04

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/85b01def9e04