If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Compile small functions faster

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

a year ago
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

a year ago
Created attachment 8744236 [details] [diff] [review]
Patch

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

a year ago
Created attachment 8744239 [details] [diff] [review]
SS improvement after Arai fixes

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 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+
(Assignee)

Updated

a year ago
Blocks: 1265307
Created attachment 8744345 [details]
Performance comparison of match/search/replace/split with different input, before/after the patch (and before self-hosting)

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 5

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c23b44faa969
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

a year 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

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de06be16ae84
(Assignee)

Comment 9

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=668ad5bbfbb5
(Assignee)

Comment 10

a year ago
Created attachment 8748515 [details] [diff] [review]
Decrease iterations of regress-476427.js

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)
Attachment #8748515 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 11

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc70bad825e8702f2ee171ae89392887753c21c
Bug 1266676 - IonMonkey: Compile smaller functions faster, r=jandem

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ecc70bad825e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Updated

a year ago
Depends on: 1270108
Depends on: 1273233
You need to log in before you can comment on or make changes to this bug.