WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
2012-11-20 20:37:09 PST
http://code.google.com/p/chromium/issues/detail?id=158645
Adam Barth
Comment 2
2012-11-20 20:37:24 PST
Created
attachment 175330
[details]
Patch
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
Created
attachment 175374
[details]
Patch
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.
Adam Barth
Comment 18
2012-11-25 08:19:02 PST
As expected, there was a 10% improvement in dom-modify:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoremodify/report.html?history=100&rev=169394
intl1 also got faster by 7.5%:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/intl1/report.html?history=100&rev=169394
intl2 also got faster by 4.5%:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/intl2/report.html?history=100&rev=169394
dom-perf improved by 9%:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dom_perf/report.html?history=100&rev=169394
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).
Ojan Vafai
Comment 20
2012-11-27 10:19:35 PST
3% improvement:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_domcoretraverse/report.html?rev=169690&graph=dom_traverse_previousSibling&trace=score&history=150
8% improvement:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibeventjquery/report.html?rev=169690&graph=jslib_event_jquery_jQuery___trigger&trace=score&history=150
25% improvement:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=169690&graph=jslib_style_jquery_jQuery____is__visible_&trace=score&history=150
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=169693&graph=jslib_style_jquery_jQuery____show__&trace=score&history=150
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=169693&graph=jslib_style_jquery_jQuery___width___x10&trace=score&history=150
7% improvement:
http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/moz/report.html?rev=169690&graph=times&trace=t_extwr&history=150
James Robinson
Comment 21
2012-11-27 12:55:53 PST
7.4% improvement:
http://build.chromium.org/f/chromium/perf/gpu-mac-release-intel/gpu_throughput/report.html?rev=169724&graph=many_images&trace=gpu_thread&history=150
Ryosuke Niwa
Comment 22
2012-11-30 00:53:18 PST
12% improvement on Animation/balls:
http://webkit-perf.appspot.com/graph.html#tests=[[3976565,2001,3001]]&sel=1353810236487.3518,1353841390292.866,32.294862429997565,37.20112003895788&displayrange=90&datatype=running
Ryosuke Niwa
Comment 23
2012-12-17 20:58:50 PST
6.7% on html5-full-render.html as well:
http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,32196],[3068,2001,7288486],[3068,2001,3001]]&sel=1353762378720.7466,1353879008367.7524,3796.4345053622596,4262.170346323358&displayrange=30&datatype=running
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug