Bug 102866 - Chromium should use TCMalloc on Mac to go fast
Summary: Chromium should use TCMalloc on Mac to go fast
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: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-20 20:36 PST by Adam Barth
Modified: 2012-12-17 21:55 PST (History)
12 users (show)

See Also:


Attachments
Patch (1.42 KB, patch)
2012-11-20 20:37 PST, Adam Barth
no flags Details | Formatted Diff | Diff
dom-modify results (first three without patch; last three with patch) (23.46 KB, text/html)
2012-11-21 00:48 PST, Adam Barth
no flags Details
Patch (4.04 KB, patch)
2012-11-21 01:33 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (3.78 KB, patch)
2012-11-24 21:30 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-11-20 20:36:27 PST
Chromium should use TCMalloc on Mac to go fast
Comment 1 Eric Seidel (no email) 2012-11-20 20:37:09 PST
http://code.google.com/p/chromium/issues/detail?id=158645
Comment 2 Adam Barth 2012-11-20 20:37:24 PST
Created attachment 175330 [details]
Patch
Comment 3 Adam Barth 2012-11-21 00:48:28 PST
Created attachment 175364 [details]
dom-modify results (first three without patch; last three with patch)

This appears to be a 5-10% win on dom-modify, which is a fairly allocation heavy benchmark.  I haven't run any other benchmarks.  It might be worth landing this patch experimentally and seeing its effect on the perf bots.
Comment 4 Adam Barth 2012-11-21 01:33:49 PST
Created attachment 175374 [details]
Patch
Comment 5 Adam Barth 2012-11-21 01:44:20 PST
One thing we need to check here is whether we still crash on failed allocations for objects that don't override operator new.  If not, we might need to create a new configuration that uses the system malloc for the global operators but not for the per-class operators.
Comment 6 Eric Seidel (no email) 2012-11-21 01:51:30 PST
Comment on attachment 175374 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175374&action=review

> Source/WTF/ChangeLog:21
> +        Fortunately, WebKit has a mechanism for selectively enablin TCMalloc

enabling

> Source/WTF/wtf/Platform.h:581
> + * WebKit isn't memory tight.

"memory tight" is not the most descriptive phrase.  :)

> Source/WTF/wtf/Platform.h:593
> + * FIXME: I believe there is actually an extra branch in malloc in this configuration
> + * because our "system" malloc already crashes on failed allocations. There's no need
> + * for the branch in the USE(SYSTEM_MALLOC) implementation of fastMalloc that crashes
> + * on failed allocations.

You should just file a bug and link to it.
Comment 7 Eric Seidel (no email) 2012-11-21 01:51:42 PST
Thank you for following up with this.
Comment 8 Eric Seidel (no email) 2012-11-21 01:52:15 PST
You might want to announce this on chromium-dev, as it may confuse some.
Comment 9 Eric Seidel (no email) 2012-11-21 01:57:16 PST
(In reply to comment #5)
> One thing we need to check here is whether we still crash on failed allocations for objects that don't override operator new.  If not, we might need to create a new configuration that uses the system malloc for the global operators but not for the per-class operators.

Yup.  Easy to test with a gtest:
http://stackoverflow.com/questions/6569713/testing-for-crash-with-google-test
Comment 10 Adam Barth 2012-11-21 02:06:39 PST
Comment on attachment 175374 [details]
Patch

cq- pending death test.
Comment 11 Ojan Vafai 2012-11-21 09:15:54 PST
I support landing this and seeing what happens on the perf bots.
Comment 12 Adam Barth 2012-11-21 23:41:32 PST
This bug is related to bug 103027, which is about optimizing malloc on Windows.
Comment 13 Adam Barth 2012-11-22 22:32:02 PST
Based on some discussion in bug 103027, it looks like http://src.chromium.org/viewvc/chrome/trunk/src/base/process_util_mac.mm is how Chromium causes the process to crash on out-of-memory, so the branch in fastMalloc is redundant on Mac as well as Windows.
Comment 14 Adam Barth 2012-11-22 22:33:09 PST
We need to test to make sure chromium-mac crashes on out-of-memory with this patch, but if it does, it sounds like this patch is correct.
Comment 15 Adam Barth 2012-11-24 21:30:36 PST
Created attachment 175873 [details]
Patch for landing
Comment 16 Adam Barth 2012-11-24 23:25:50 PST
Comment on attachment 175873 [details]
Patch for landing

Clearing flags on attachment: 175873

Committed r135666: <http://trac.webkit.org/changeset/135666>
Comment 17 Adam Barth 2012-11-24 23:25:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Eric Seidel (no email) 2012-11-25 08:55:50 PST
I'm glad this worked out so well. Thanks for taking this on. I suspect we'll want to investigate how to use chromium's copy of tcmalloc directly (eventually).