Closed Bug 930437 Opened 6 years ago Closed 6 years ago

IonMonkey keeps recompiling the same script with parallel compilation

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached file Testcase (obsolete) —
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.
Flags: needinfo?(bhackett1024)
Attached file Testcase
Let's see if an https URL for jQuery works...
Attachment #821579 - Attachment is obsolete: true
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).
Flags: needinfo?(bhackett1024)
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.
Blocks: 735406
Attached patch PatchSplinter Review
Don't abort during lowering but use a type policy, similar to MToDouble.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #822253 - Flags: review?(bhackett1024)
Attachment #822253 - Flags: review?(bhackett1024) → review+
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/85b01def9e04
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.