Closed Bug 807464 Opened 12 years ago Closed 12 years ago

IonMonkey: Bump the checked size or find a better heuristic.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: nbp, Assigned: sstangl)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:p1])

Attachments

(4 files)

This prevent the compilation of 2 hot functions in PdfJS. The current limit is set at 1500. [Abort] Script too large (1782 bytes) [Abort] Prevent compilation of pdfjs.js:16934 [Abort] Disabling Ion compilation of script pdfjs.js:16934 [Abort] Script too large (1678 bytes) [Abort] Prevent compilation of pdfjs.js:27687 [Abort] Disabling Ion compilation of script pdfjs.js:27687 At the same time it would be better to move the constants to IonOptions in Ion.h.
Yes, investigating.
Assignee: general → sstangl
Bumping the limit to 2000 is slowing down the script and the spew of the 2 functions indicates that (In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #0) > [Abort] Script too large (1782 bytes) > [Abort] Prevent compilation of pdfjs.js:16934 > [Abort] Disabling Ion compilation of script pdfjs.js:16934 This function cannot compile because Bug 807443. > [Abort] Script too large (1678 bytes) > [Abort] Prevent compilation of pdfjs.js:27687 > [Abort] Disabling Ion compilation of script pdfjs.js:27687 And this function hit a type barrier after a while before being recompiled at the end of the program.
Attached is a graph of script length versus total compilation time. Data is sampled from SS, V8, Kraken, and the Emscripten bbread, box2d, and bullet benchmarks from Alon's misc-js-benchmarks repository. Note that a number of Emscripten functions trigger bad behavior in regalloc and phi-elimination -- all the outliers are from Emscripten. I have not yet diagnosed them further. The green line is an attempt at a best fit: > f(x) = a * (x**b), where > a = 0.0987429 > b = 1.38869 Based on this fit, we have the following samples of expected compilation times: > length 250: 0.2ms > length 500: 0.6ms > length 1000: 1.4ms > length 1500: 2.5ms > length 2000: 3.8ms > length 4000: 9.9ms > length 6000: 17.4ms Before we draw conclusions from this data, let's look first at the effects from removing the limit on permissible script size, assuming the same definition of function hotness: - SS is completely unaffected; we already compile everything that's hot. - V8 gains compilation of regexp.js:120 and regexp.js:240, both with size < 2000; performance is unaffected. - Kraken gains compilation of audio-beat-detection-data.js:2147, with size 4047; it takes an average of 10.4 ms to compile, we compile it 4x, and performance regresses on top of that, from 304ms -> 380ms. 42% of that compilation time is in regalloc. - Emscripten is therefore responsible for nearly every function with length > 2000 -- but, the vast majority of Emscripten functions are < 2000 in length, with a cutoff quite close to 2000. The thick red point cloud in the graph that ends at 2000 is also present if we restrict data to solely emscripten benchmarks. Increasing the script limit from 1500 to 2000 improves BananaBread performance from an average frame length of 13.5ms to an average frame length of 12.4ms. Startup time is also improved by 600ms. So, based on the observations that: 1) From the graph, most hot functions are <= 2000 in length 2) Functions > 2000 in length tend to have unpredictable compile times 3) Increasing the limit to 2000 doesn't affect SS, V8, or Kraken 4) Increasing the limit to 2000 helps BananaBread I would recommend that we just increase the limit to 2000.
Attached file Python script to analyze timer output (deleted) —
The bottom of the file contains a number of possible analyses that I ran at various points, commented out.
Attachment #679411 - Flags: review?(dvander)
Comment on attachment 679411 [details] [diff] [review] Increase script size limit to 2000. Review of attachment 679411 [details] [diff] [review]: ----------------------------------------------------------------- Can you move these constants to jsOptions ?
Attachment #679411 - Flags: review?(dvander) → review+
Blocks: 813425
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: