Bug 147154 - Fixed VM pool allocation should have a reserve for allocations that cannot fail
Summary: Fixed VM pool allocation should have a reserve for allocations that cannot fail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-21 11:14 PDT by Filip Pizlo
Modified: 2015-07-22 13:52 PDT (History)
14 users (show)

See Also:


Attachments
the patch (17.64 KB, patch)
2015-07-21 12:57 PDT, Filip Pizlo
ggaren: review+
Details | Formatted Diff | Diff
performance results (79.49 KB, text/plain)
2015-07-21 14:39 PDT, Filip Pizlo
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2015-07-21 11:14:08 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2015-07-21 12:36:30 PDT
Currently, if I run with a JIT pool that is artificially limited to 50KB, I get these failures due to crashes when we try to allocate things for which there is no fallback:

stress/lexical-let-loop-semantics.js.ftl-no-cjit-small-pool
stress/lexical-let-semantics.js.ftl-no-cjit-small-pool
stress/load-varargs-then-inlined-call-exit-in-foo.js.ftl-no-cjit-small-pool
stress/math-clz32-basics.js.ftl-no-cjit-small-pool
stress/multi-put-by-offset-multiple-transitions.js.ftl-no-cjit-small-pool
stress/multi-put-by-offset-reallocation-butterfly-cse.js.ftl-no-cjit-small-pool
stress/multi-put-by-offset-reallocation-cases.js.ftl-no-cjit-small-pool
stress/throw-from-ftl-call-ic-slow-path-cells.js.ftl-no-cjit-small-pool
stress/throw-from-ftl-call-ic-slow-path.js.ftl-no-cjit-small-pool
stress/type-of-functions-and-objects.js.ftl-no-cjit-small-pool
stress/typed-array-get-by-val-profiling.js.ftl-no-cjit-small-pool
stress/typed-array-put-by-val-profiling.js.ftl-no-cjit-small-pool
regress/script-tests/deltablue-varargs.js.ftl-no-cjit-small-pool
regress/script-tests/emscripten-cube2hash.js.ftl-no-cjit-small-pool
regress/script-tests/inlined-put-by-id-transition.js.ftl-no-cjit-small-pool
regress/script-tests/sorting-benchmark.js.ftl-no-cjit-small-pool
sunspider-1.0/date-format-tofte.js.ftl-no-cjit-small-pool
v8-v6/v8-crypto.js.ftl-no-cjit-small-pool
v8-v6/v8-deltablue.js.ftl-no-cjit-small-pool
v8-v6/v8-earley-boyer.js.ftl-no-cjit-small-pool
v8-v6/v8-raytrace.js.ftl-no-cjit-small-pool
v8-v6/v8-regexp.js.ftl-no-cjit-small-pool
v8-v6/v8-richards.js.ftl-no-cjit-small-pool
v8-v6/v8-splay.js.ftl-no-cjit-small-pool
stress/const-loop-semantics.js.ftl-no-cjit-small-pool
stress/const-tdz.js.ftl-no-cjit-small-pool
stress/constant-folding-osr-exit.js.ftl-no-cjit-small-pool
stress/flatten-oversize-dictionary-object.js.ftl-no-cjit-small-pool
stress/for-in-tests.js.ftl-no-cjit-small-pool
Comment 2 Filip Pizlo 2015-07-21 12:57:57 PDT
Created attachment 257196 [details]
the patch
Comment 3 WebKit Commit Bot 2015-07-21 13:00:02 PDT
Attachment 257196 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/Options.cpp:353:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Geoffrey Garen 2015-07-21 13:02:43 PDT
Comment on attachment 257196 [details]
the patch

r=me
Comment 5 Filip Pizlo 2015-07-21 14:39:40 PDT
Created attachment 257203 [details]
performance results

This patch is neutral.  Using a 16MB limit is also neutral.  But this is cutting it close - a 1.6MB limit causes huge slow-downs.
Comment 6 Filip Pizlo 2015-07-21 14:43:02 PDT
Landed in http://trac.webkit.org/changeset/187125
Comment 8 Filip Pizlo 2015-07-21 15:24:11 PDT
(In reply to comment #7)
> Looks like this broke a lot of tests:
> 
> https://build.webkit.org/builders/
> Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/7561/steps/
> webkit-jsc-cloop-test/logs/stdio
> 
> https://build.webkit.org/builders/Apple%20Mavericks%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/11551/steps/webkit-32bit-jsc-test/logs/
> stdio

This isn't real.  On the bot all tests fail.  I'm testing 32-bit locally and nothing fails.

> 
> https://build.webkit.org/builders/
> Apple%20Mavericks%20LLINT%20CLoop%20%28BuildAndTest%29/builds/16973/steps/
> webkit-jsc-cloop-test/logs/stdio
Comment 9 Filip Pizlo 2015-07-21 15:33:37 PDT
(In reply to comment #7)
> Looks like this broke a lot of tests:
> 
> https://build.webkit.org/builders/
> Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/7561/steps/
> webkit-jsc-cloop-test/logs/stdio

This is almost certainly a fluke.  This reports that all tests fail, but on my machine I am seeing nothing but passes so far.

> 
> https://build.webkit.org/builders/Apple%20Mavericks%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/11551/steps/webkit-32bit-jsc-test/logs/
> stdio
> 
> https://build.webkit.org/builders/
> Apple%20Mavericks%20LLINT%20CLoop%20%28BuildAndTest%29/builds/16973/steps/
> webkit-jsc-cloop-test/logs/stdio

Ditto.
Comment 10 Filip Pizlo 2015-07-21 15:54:03 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > Looks like this broke a lot of tests:
> > 
> > https://build.webkit.org/builders/
> > Apple%20Yosemite%20LLINT%20CLoop%20%28BuildAndTest%29/builds/7561/steps/
> > webkit-jsc-cloop-test/logs/stdio
> 
> This is almost certainly a fluke.  This reports that all tests fail, but on
> my machine I am seeing nothing but passes so far.
> 
> > 
> > https://build.webkit.org/builders/Apple%20Mavericks%2032-
> > bit%20JSC%20%28BuildAndTest%29/builds/11551/steps/webkit-32bit-jsc-test/logs/
> > stdio
> > 
> > https://build.webkit.org/builders/
> > Apple%20Mavericks%20LLINT%20CLoop%20%28BuildAndTest%29/builds/16973/steps/
> > webkit-jsc-cloop-test/logs/stdio
> 
> Ditto.

I've now run all of the stress tests in cloop, and found some longstanding issues, but certainly nothing like what the bots were reporting.

https://bugs.webkit.org/show_bug.cgi?id=147167
https://bugs.webkit.org/show_bug.cgi?id=147168
Comment 11 Filip Pizlo 2015-07-21 17:03:22 PDT
Well, looks like the bots were right.  This was a timing issue that only manifested on some debug builds.  The fix is:

http://trac.webkit.org/changeset/187139
Comment 12 Csaba Osztrogonác 2015-07-22 09:07:03 PDT
(In reply to comment #11)
> Well, looks like the bots were right.  This was a timing issue that only
> manifested on some debug builds.  The fix is:
> 
> http://trac.webkit.org/changeset/187139

The Apple Yosemite Debug bot still has 914 asserting tests:
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20JSC%20%28Tests%29
Comment 13 Filip Pizlo 2015-07-22 13:19:51 PDT
(In reply to comment #12)
> (In reply to comment #11)
> > Well, looks like the bots were right.  This was a timing issue that only
> > manifested on some debug builds.  The fix is:
> > 
> > http://trac.webkit.org/changeset/187139
> 
> The Apple Yosemite Debug bot still has 914 asserting tests:
> https://build.webkit.org/builders/
> Apple%20Yosemite%20Debug%20JSC%20%28Tests%29

Ooops, fix on the way.
Comment 14 Filip Pizlo 2015-07-22 13:52:01 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Well, looks like the bots were right.  This was a timing issue that only
> > > manifested on some debug builds.  The fix is:
> > > 
> > > http://trac.webkit.org/changeset/187139
> > 
> > The Apple Yosemite Debug bot still has 914 asserting tests:
> > https://build.webkit.org/builders/
> > Apple%20Yosemite%20Debug%20JSC%20%28Tests%29
> 
> Ooops, fix on the way.

Fixed: http://trac.webkit.org/changeset/187175