Closed
Bug 1266676
Opened 9 years ago
Closed 9 years ago
Compile small functions faster
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(4 files)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Something I noticed earlier already is that our performance increase going from cold to warm to hot code is not consequential. We have a gap between cold and hot code. We are or slow or fast, we don't improve performance incremental. This is mostly because we have a single threshold where we start compiling ion code. That threshold is because compiling takes time and we want to make sure it is beneficial to compile it. (I.e. that for short running code compiling code we don't take overhead of compiling when we won't run it long enough.
The threshold is chosen based on the average compilation cost of a normal function. As a result this gives a disadvantage to small functions. Small functions can get compiled earlier, since they will be finished quite quickly. And this will help warm, but not yet hot code.
I noticed this earlier on a benchmark till provided, but cannot find the link anymore. But this is also quite visible in the graphs arai created. Our performance difference over X iterations. In the beginning we accumulated some overhead, by running slower and as soon as we get in ionmonkey we have the same performance as before (on the regexp code). But in the meantime we have some gathered some extra execution time by first running in the baseline.
(See bug 887016 graphic "Performance comparison of match/search/replace/split with different input")
To improve the process of going from slow to fast code, we can compile smaller functions faster. The overhead is quite small and we can run those faster in ion. As a result this can improve our warm, but not hot execution code.
(This helps selfhosting quite a lot, since we introduce quite a lot of smaller functions, but also normal JS code will feel this improvement. Though not quite visible on longer running benchmarks)
Assignee | ||
Comment 1•9 years ago
|
||
Compiles small functions when warmup threshold is 100. Could go further, but that increased the number of compilation considerably.
Improves current trunk from 331ms to 316.5ms on sunspider.
Assignee: nobody → hv1989
Attachment #8744236 -
Flags: review?(jdemooij)
Assignee | ||
Comment 2•9 years ago
|
||
I also measured this with all patches of arai already committed. Those bring already an improvement from 331ms to 312.4ms. This extra patch improves it further from 312.4ms to 294.6ms (6%).
The improvements are mostly the regressed benchmarks from the regexp and array landing.
Comment 3•9 years ago
|
||
Comment on attachment 8744236 [details] [diff] [review]
Patch
Review of attachment 8744236 [details] [diff] [review]:
-----------------------------------------------------------------
Interesting idea! I wonder if 100 is the right value - looking forward to AWFY results.
Attachment #8744236 -
Flags: review?(jdemooij) → review+
Comment 4•9 years ago
|
||
Here's the performance comparison graph for same testcases as bug 887016's graph (RegExp).
Surely, it shows different characteristic in the curve between cold and hot.
In most cases:
1) "after" curve reaches the constant gradient on Ion execution quicker than "before"
2) "after" curve shows better performance on more than 1000 iterations, as "before" curve shows bump around 1000, but "after" curve doesn't or does smaller
3) "after" curve shows worse performance on less than 1000 iterations, as "after" curve shows bump at around 100 iteration
maybe, some regression shown in SunSpider matches to the (3) case? as they're all short-time testcases.
Comment 6•9 years ago
|
||
Backed out because it looks like it fails regress-476427.js.
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae9a6bb0821
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=26422968&repo=mozilla-inbound
Tons of:
09:56:16 INFO - JavaScript warning: file:///C:/slave/test/build/tests/jsreftest/tests/js1_8/extensions/regress-476427.js, line 1: JavaScript 1.6's for-each-in loops are deprecated; consider using ES6 for-of instead
09:56:17 INFO - Hit MOZ_CRASH() at c:/builds/moz2_slave/m-in-w32-d-0000000000000000000/build/src/xpcom/base/nsDebugImpl.cpp:604
Some:
09:56:17 INFO - ### ERROR: SymInitialize: Not enough storage is available to process this command.
09:56:17 INFO - #01: ??? (???:???)
09:56:20 WARNING - TEST-UNEXPECTED-FAIL | file:///C:/slave/test/build/tests/jsreftest/tests/jsreftest.html?test=js1_8/extensions/regress-476427.js | application terminated with exit code 1
Please let me know if it's caused by something different, e.g. a bad slave.
Flags: needinfo?(hv1989)
Assignee | ||
Comment 7•9 years ago
|
||
Need to look into this. Later results show this is probably not a bad slave. I assume compiling more is hitting something bad in ionmonkey.
Flags: needinfo?(hv1989)
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
The cause of the failure is test "regress-476427.js". It starts compiling earlier, which results in taking all the memory. So we crash due to OOM. Decreasing the total iterations on that test helps us to not OOM
Attachment #8748515 -
Flags: review?(jdemooij)
Updated•9 years ago
|
Attachment #8748515 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc70bad825e8702f2ee171ae89392887753c21c
Bug 1266676 - IonMonkey: Compile smaller functions faster, r=jandem
Comment 12•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•