JM: Compile large scripts in chunks

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks: 1 bug)

unspecified
mozilla12
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 578331 [details] [diff] [review]
WIP (c4832f2d9986)

There's a longstanding problem with JM+TI (and any engine that combines both whole method compilation and invalidation/recompilation) where too much time is spent compiling large scripts.  The amount of time to compile a script is linear in its size (optimistic, but seems to hold in practice) and the number of times a script needs to be recompiled is also expected to be linear in its size, all else being equal.  This leads to quadratic behavior which really hurts when dealing with large scripts.

This can be helped (not sure if fixed) by not doing whole method compilation, but rather compiling scripts in chunks.  Pick an arbitrary partitioning of the script into chunks, only compile one chunk at a time and keep track of cross-chunk edges to fix up after doing a compilation or discarding a chunk due to a recompilation.  The attached patch (WIP) does this, still needs more work to not be broken.
(Assignee)

Comment 1

6 years ago
Created attachment 578420 [details] [diff] [review]
WIP (c4832f2d9986)

Passes jit-tests -mna with a chunk size of 10 ops (for stress testing).
Assignee: general → bhackett1024
Attachment #578331 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 689284
(Assignee)

Comment 2

6 years ago
Created attachment 578447 [details] [diff] [review]
WIP

Wrong patch attached earlier.
Attachment #578420 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Blocks: 687127
(Assignee)

Updated

6 years ago
Blocks: 670885
(Assignee)

Comment 3

5 years ago
Created attachment 582350 [details] [diff] [review]
patch (c4832f2d9986)

Working patch, needs a rebase.
Attachment #578447 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
Created attachment 582383 [details] [diff] [review]
patch (61a46d539c69)

Patch rebased to tip.  The CHUNK_LIMIT environment variable is in place for testing, when this lands it will be hardcoded to something else (like 1000).
Attachment #582350 - Attachment is obsolete: true
Attachment #582383 - Flags: review?(dvander)
(Assignee)

Comment 5

5 years ago
Decoder, Gary: can you guys fuzz this patch some?  It is a significant method JIT change.  Beforehand, you should set the CHUNK_LIMIT environment variable to some smallish value, between 5 and 100 say (this controls how scripts are partitioned during compilation).  The testing should mainly be done on x86, as this functionality is totally disabled on x64 (for now at least, and possibly for the lifetime of JM).  I can also make the chunk limit controllable from the shell with a chunkLimit(N) function if that would make things easier, and will probably take this route anyways when landing the patch.
(Assignee)

Comment 6

5 years ago
Created attachment 582860 [details] [diff] [review]
patch (b121a045b451)

Rebased, removes CHUNK_LIMIT environment variable and allows the chunk limit to be changed in the shell with mjitChunkLimit(N).
Attachment #582383 - Attachment is obsolete: true
Attachment #582383 - Flags: review?(dvander)
Attachment #582860 - Flags: review?(dvander)
> least, and possibly for the lifetime of JM).  I can also make the chunk
> limit controllable from the shell with a chunkLimit(N) function if that
> would make things easier, and will probably take this route anyways when
> landing the patch.

Adding the chunkLimit(N) function will likely be really useful. :)
I'm on it, will file bugs blocking this one when I have results.
Depends on: 712090
Depends on: 712171
Depends on: 712265
Depends on: 712266
Depends on: 712267
(Assignee)

Comment 9

5 years ago
Created attachment 583509 [details] [diff] [review]
patch (b121a045b451)

Updated with fixes for dependent bugs.
Attachment #582860 - Attachment is obsolete: true
Attachment #582860 - Flags: review?(dvander)
Attachment #583509 - Flags: review?(dvander)
> Created attachment 583509 [details] [diff] [review]
> patch (b121a045b451)
> 
> Updated with fixes for dependent bugs.

fwiw, this patch has bitrotted, it would be nice to have a rebase. :)

Updated

5 years ago
Blocks: 716255
Comment on attachment 583509 [details] [diff] [review]
patch (b121a045b451)

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

There's not a whole lot I can do to review this patch but I do have one suggestion with regards to IonMonkey compatibility:

::: js/src/jsinfer.h
@@ +1125,5 @@
> +struct RecompileInfo
> +{
> +    JSScript *script;
> +    bool constructing:1;
> +    uint32_t chunkIndex:31;

It would be much nicer if this structure could be:
    JSScript *script;
    jsbytecode *pc;

And then have processPendingRecompiles do the work of finding the chunk index in the ctor and non-ctor JITScripts. Currently, IonMonkey doesn't do chunked compilation (and won't in the next few months); doesn't have constructor bifurcation; it might end up having a different chunking approach too. So having a more generic interface would be ideal.
Attachment #583509 - Flags: review?(dvander) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c192e5bd41
(Assignee)

Comment 13

5 years ago
Oh, for integrating this into IM, just use constructing == false and chunkIndex == 0.  The script/constructing/chunkIndex are just identifiers for a compilation unit, and JM/IM do not track dependencies at such a fine granularity as the pc.
Blocks: 719189
https://hg.mozilla.org/mozilla-central/rev/d0c192e5bd41
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
No longer blocks: 719189
Duplicate of this bug: 719189

Updated

5 years ago
Depends on: 719918
(Assignee)

Updated

5 years ago
Depends on: 720359
Depends on: 721284
Depends on: 720234
Depends on: 722598
Depends on: 726799
Depends on: 727223
Depends on: 728342
(Assignee)

Updated

5 years ago
Depends on: 728372
Depends on: 730806
Depends on: 719674
Depends on: 728509
Depends on: 732776

Updated

5 years ago
Depends on: 770089
Depends on: 817475
You need to log in before you can comment on or make changes to this bug.