Bug 140900 - Use FastMalloc (bmalloc) instead of BlockAllocator for GC pages
Summary: Use FastMalloc (bmalloc) instead of BlockAllocator for GC pages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on: 140951 140953 141119
Blocks:
  Show dependency treegraph
 
Reported: 2015-01-26 13:44 PST by Geoffrey Garen
Modified: 2015-03-07 16:23 PST (History)
11 users (show)

See Also:


Attachments
Patch (89.40 KB, patch)
2015-01-26 14:00 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
run-jsc-benchmarks results (52.15 KB, text/plain)
2015-01-26 14:01 PST, Geoffrey Garen
no flags Details
Patch (90.07 KB, patch)
2015-01-26 15:40 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (9.65 KB, patch)
2015-03-05 19:42 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (6.53 KB, patch)
2015-03-06 16:07 PST, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (59.08 KB, patch)
2015-03-07 13:19 PST, Geoffrey Garen
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2015-01-26 13:44:41 PST
Use FastMalloc (bmalloc) instead of BlockAllocator for GC pages
Comment 1 Geoffrey Garen 2015-01-26 14:00:27 PST
Created attachment 245372 [details]
Patch
Comment 2 Geoffrey Garen 2015-01-26 14:01:41 PST
Created attachment 245374 [details]
run-jsc-benchmarks results

Uploading performance results.
Comment 3 Mark Hahnenberg 2015-01-26 14:11:59 PST
Comment on attachment 245372 [details]
Patch

Lots of red, me gusta :-) Looks like builds are angry. r=me w/ fixes.

What's bmalloc's story for returning memory to the OS? I'm assuming it's something similar to what BlockAllocator did, just curious.
Comment 4 Geoffrey Garen 2015-01-26 14:37:51 PST
> What's bmalloc's story for returning memory to the OS? I'm assuming it's
> something similar to what BlockAllocator did, just curious.

bmalloc uses a timer on a background thread with back-off if other threads start asking the heap to grow -- very similar to what BlockAllocator did.
Comment 5 Geoffrey Garen 2015-01-26 15:40:56 PST
Created attachment 245382 [details]
Patch
Comment 6 Geoffrey Garen 2015-01-26 15:41:39 PST
Trying to fix the build...
Comment 7 Geoffrey Garen 2015-01-27 10:29:27 PST
Committed r179192: <http://trac.webkit.org/changeset/179192>
Comment 8 Csaba Osztrogonác 2015-01-27 12:07:45 PST
(In reply to comment #7)
> Committed r179192: <http://trac.webkit.org/changeset/179192>

It made almost all tests assert on the 32 bit bot:
https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/7055
Comment 9 Csaba Osztrogonác 2015-01-27 12:09:54 PST
and many failures on the ARM bot:
http://build.webkit.sed.hu/builders/EFL%20ARMv7-ARM%20Linux%20Release%20%28Build%29%201404/builds/106
Comment 10 Csaba Osztrogonác 2015-01-27 12:11:55 PST
and all tests fail on the Yosemite performance bot:
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%28Perf%29/builds/847
Comment 11 Alexey Proskuryakov 2015-01-27 12:30:51 PST
Strangely, it seems flaky. I forced clean builds on Yosemite, although there probably is a real problem.
Comment 12 WebKit Commit Bot 2015-01-27 12:49:56 PST
Re-opened since this is blocked by bug 140951
Comment 14 Csaba Osztrogonác 2015-01-27 13:04:58 PST
more info:

There are 2 Yosemite bots:
- https://build.webkit.org/buildslaves/bot191 (pass)
- https://build.webkit.org/buildslaves/bot190 (fail)

- https://build.webkit.org/buildslaves/bot192 (fail)
- https://build.webkit.org/buildslaves/bot193 (pass)

... but failures occure only one of them. Maybe there are some 
black magic intalled on them which is necessary to make tests pass.
Comment 15 Csaba Osztrogonác 2015-01-27 13:18:15 PST
any plan to fix this serious regression?
Comment 16 Alexey Proskuryakov 2015-01-27 14:16:22 PST
Matt rolled this out in r179211, however Mac Yosemite bots went green even before that. We did a clean build, and also there was a seemingly unrelated patch landed in the meanwhile (http://trac.webkit.org/changeset/179202).

Not sure about ARM and 32-bit tests.
Comment 17 Geoffrey Garen 2015-01-29 11:20:03 PST
Committed r179348: <http://trac.webkit.org/changeset/179348>
Comment 18 Geoffrey Garen 2015-01-29 13:41:28 PST
Committed r179361: <http://trac.webkit.org/changeset/179361>
Comment 19 Geoffrey Garen 2015-01-30 11:57:30 PST
Committed r179407: <http://trac.webkit.org/changeset/179407>
Comment 20 Geoffrey Garen 2015-01-30 16:51:00 PST
Committed r179426: <http://trac.webkit.org/changeset/179426>
Comment 21 Andreas Kling 2015-01-31 00:28:38 PST
(In reply to comment #20)
> Committed r179426: <http://trac.webkit.org/changeset/179426>

Looks like ~7% memory use regression on the ol' buster.
Comment 22 WebKit Commit Bot 2015-01-31 10:24:01 PST
Re-opened since this is blocked by bug 141119
Comment 23 Andreas Kling 2015-01-31 16:54:18 PST
(In reply to comment #18)
> Committed r179361: <http://trac.webkit.org/changeset/179361>

Looks like ~40% regression on Bindings/create-element from this change:

https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22Bindings%2Fcreate-element%3ARuns%22%5D%5D
Comment 24 Geoffrey Garen 2015-02-02 14:27:11 PST
Committed r179500: <http://trac.webkit.org/changeset/179500>
Comment 25 Geoffrey Garen 2015-03-05 19:41:57 PST
Reopening to attach new patch.
Comment 26 Geoffrey Garen 2015-03-05 19:42:02 PST
Created attachment 248031 [details]
Patch
Comment 27 Geoffrey Garen 2015-03-05 19:44:42 PST
> > Committed r179361: <http://trac.webkit.org/changeset/179361>
> 
> Looks like ~40% regression on Bindings/create-element from this change:
> 
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-
> mavericks%22%2C%22Bindings%2Fcreate-element%3ARuns%22%5D%5D

This regression mysteriously disappeared after a bmalloc refactoring patch.
Comment 28 Geoffrey Garen 2015-03-06 08:59:15 PST
Committed r181157: <http://trac.webkit.org/changeset/181157>
Comment 29 Csaba Osztrogonác 2015-03-06 09:34:28 PST
(In reply to comment #28)
> Committed r181157: <http://trac.webkit.org/changeset/181157>

It broke zillion tests on the 32 bit bot:
https://build.webkit.org/builders/Apple%20Mavericks%2032-bit%20JSC%20%28BuildAndTest%29/builds/8148
Comment 30 Csaba Osztrogonác 2015-03-06 09:35:42 PST
and on the Windows bot too:
https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/50194
Comment 31 Csaba Osztrogonác 2015-03-06 10:17:17 PST
Geoffrey, are you working on fixing the regression on 32 bit bots?
Comment 32 Geoffrey Garen 2015-03-06 11:48:51 PST
sunspider-1.0/access-nsieve.js.default: ASSERTION FAILED: is8ByteAligned(reinterpret_cast<void*>(m_remaining))
sunspider-1.0/access-nsieve.js.default: /Volumes/Data/slave/mavericks-32bitJSC-debug/build/Source/JavaScriptCore/heap/CopiedBlock.h(156) : JSC::CopiedBlock::CopiedBlock(size_t)
sunspider-1.0/access-nsieve.js.default: 1   0xaf561d WTFCrash
sunspider-1.0/access-nsieve.js.default: 2   0x2752f2 JSC::CopiedBlock::CopiedBlock(unsigned long)
sunspider-1.0/access-nsieve.js.default: 3   0x2751d4 JSC::CopiedBlock::CopiedBlock(unsigned long)
sunspider-1.0/access-nsieve.js.default: 4   0x2751a1 JSC::CopiedBlock::createNoZeroFill(unsigned long)
sunspider-1.0/access-nsieve.js.default: 5   0x2723e7 JSC::CopiedBlock::create(unsigned long)
sunspider-1.0/access-nsieve.js.default: 6   0x270bc1 JSC::CopiedSpace::tryAllocateOversize(unsigned long, void**)
Comment 33 Geoffrey Garen 2015-03-06 12:00:52 PST
I think sizeof(CopiedBlock) must not be 8-byte-aligned on 32-bit platforms. I'm regression-testing a fix.
Comment 34 Geoffrey Garen 2015-03-06 12:21:11 PST
Committed r181177: <http://trac.webkit.org/changeset/181177>
Comment 35 Csaba Osztrogonác 2015-03-06 13:04:54 PST
(In reply to comment #34)
> Committed r181177: <http://trac.webkit.org/changeset/181177>

The Windows bot is still broken: https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/50202
Comment 36 Geoffrey Garen 2015-03-06 14:03:24 PST
Committed revision 181180: <http://trac.webkit.org/changeset/181180>.
Comment 37 Geoffrey Garen 2015-03-06 16:07:00 PST
Reopening to attach new patch.
Comment 38 Geoffrey Garen 2015-03-06 16:07:05 PST
Created attachment 248108 [details]
Patch
Comment 39 Geoffrey Garen 2015-03-07 11:04:33 PST
Committed r181210: <http://trac.webkit.org/changeset/181210>
Comment 40 Geoffrey Garen 2015-03-07 13:19:12 PST
Reopening to attach new patch.
Comment 41 Geoffrey Garen 2015-03-07 13:19:37 PST
Created attachment 248166 [details]
Patch
Comment 42 Geoffrey Garen 2015-03-07 13:21:38 PST
> Looks like ~7% memory use regression on the ol' buster.

This regression did not reappear in re-landing this patch broken out into pieces, probably because of these improvements to bmalloc:

https://bugs.webkit.org/show_bug.cgi?id=142055
https://bugs.webkit.org/show_bug.cgi?id=142058
https://bugs.webkit.org/show_bug.cgi?id=142194
Comment 43 Andreas Kling 2015-03-07 13:33:11 PST
Comment on attachment 248166 [details]
Patch

rs=me to remove, but one thing needs fixing:

/Volumes/Data/EWS/WebKit/Source/WebCore/platform/cocoa/MemoryPressureHandlerCocoa.mm:156:42: error: no member named 'blockAllocator' in 'JSC::Heap'
        JSDOMWindowBase::commonVM().heap.blockAllocator().releaseFreeRegions();
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
1 error generated.
Comment 44 Geoffrey Garen 2015-03-07 16:23:27 PST
Committed r181215: <http://trac.webkit.org/changeset/181215>