RESOLVED FIXED 102866
Chromium should use TCMalloc on Mac to go fast
https://bugs.webkit.org/show_bug.cgi?id=102866
Summary Chromium should use TCMalloc on Mac to go fast
Adam Barth
Reported 2012-11-20 20:36:27 PST
Chromium should use TCMalloc on Mac to go fast
Attachments
Patch (1.42 KB, patch)
2012-11-20 20:37 PST, Adam Barth
no flags
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
Patch (4.04 KB, patch)
2012-11-21 01:33 PST, Adam Barth
no flags
Patch for landing (3.78 KB, patch)
2012-11-24 21:30 PST, Adam Barth
no flags
Eric Seidel (no email)
Comment 1 2012-11-20 20:37:09 PST
Adam Barth
Comment 2 2012-11-20 20:37:24 PST
Adam Barth
Comment 3 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.
Adam Barth
Comment 4 2012-11-21 01:33:49 PST
Adam Barth
Comment 5 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.
Eric Seidel (no email)
Comment 6 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.
Eric Seidel (no email)
Comment 7 2012-11-21 01:51:42 PST
Thank you for following up with this.
Eric Seidel (no email)
Comment 8 2012-11-21 01:52:15 PST
You might want to announce this on chromium-dev, as it may confuse some.
Eric Seidel (no email)
Comment 9 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
Adam Barth
Comment 10 2012-11-21 02:06:39 PST
Comment on attachment 175374 [details] Patch cq- pending death test.
Ojan Vafai
Comment 11 2012-11-21 09:15:54 PST
I support landing this and seeing what happens on the perf bots.
Adam Barth
Comment 12 2012-11-21 23:41:32 PST
This bug is related to bug 103027, which is about optimizing malloc on Windows.
Adam Barth
Comment 13 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.
Adam Barth
Comment 14 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.
Adam Barth
Comment 15 2012-11-24 21:30:36 PST
Created attachment 175873 [details] Patch for landing
Adam Barth
Comment 16 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>
Adam Barth
Comment 17 2012-11-24 23:25:54 PST
All reviewed patches have been landed. Closing bug.
Eric Seidel (no email)
Comment 19 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).
Note You need to log in before you can comment on or make changes to this bug.