103027 2012-11-21 23:39:42 -0800 [Chromium] fastMalloc has an extra branch on Windows 2013-04-11 11:48:15 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 103405 1 abarth abarth avi benjamin eric jamesr ojan schenney thakis tony webkit.review.bot oldest_to_newest 773626 0 abarth 2012-11-21 23:39:42 -0800 On Windows, we route WebKit allocations through the USE(SYSTEM_ALLOCATOR) path in FastMalloc.cpp. That code path checks whether malloc() returns 0 in order to crash when we run out of memory. However, the crash stacks we get when we really run out of memory on Windows look like the following: 0x5e3a9d97 [chrome.dll] - process_util_win.cc:109] base::`anonymous namespace'::OnNoMemory() 0x5de8165f [chrome.dll] - allocator_shim.cc:135] malloc 0x5dec8c18 [chrome.dll] - fastmalloc.cpp:268] WTF::fastMalloc(unsigned int) 0x5e0ebff3 [chrome.dll] - vector.h:903] WTF::Vector<char,0>::reserveCapacity(unsigned int) 0x5e0ebfc7 [chrome.dll] - vector.h:820] WTF::Vector<char,0>::expandCapacity(unsigned int) 0x5e271993 [chrome.dll] - sharedbuffer.cpp:224] WebCore::SharedBuffer::buffer() 0x5e85df0d [chrome.dll] - cachedrawresource.cpp:53] WebCore::CachedRawResource::data(WTF::PassRefPtr<WebCore::SharedBuffer>,bool) 0x5e26cf82 [chrome.dll] - subresourceloader.cpp:253] WebCore::SubresourceLoader::sendDataToResource(char const *,int) 0x5e26cba8 [chrome.dll] - subresourceloader.cpp:227] WebCore::SubresourceLoader::didReceiveData(char const *,int,__int64,bool) (See, for example, <https://code.google.com/p/chromium/issues/detail?id=138506>.) Notice that we actually crash inside malloc rather than in FastMalloc.cpp. That means that the branch for malloc() returning zero is not needed. We should remove it so that WebKit can go fast. 773629 1 abarth 2012-11-21 23:40:11 -0800 This bug is related to bug 102866, which is about optimizing malloc on Mac. 773652 2 eric 2012-11-21 23:59:22 -0800 This is as simple as defining ENABLE_GLOBAL_FASTMALLOC_NEW=0, no? Or will that still leave the per-object overrides on? 773653 3 eric 2012-11-22 00:00:11 -0800 I guess we could also just conditionalize the branch. With a SYSTEM_MALLOC_CRASHES_ON_OOM or similar. 773981 4 abarth 2012-11-22 08:48:21 -0800 > Or will that still leave the per-object overrides on? I believe that will still leave the per-object overrides. We might need a ENABLE_PER_OBJECT_FASTMALLOC_NEW as well. We should think about how to structure these defines in the context of bug 102866 as well because we'll like need to keep the global overrides but use the system malloc rather than TCMalloc. Here are the independent choices: 1) Override global new. 2) Override per-object new. 3) When overriding global new, use the system malloc (versus TCMalloc). 4) When overriding per-object new, use the system malloc (versus TCMalloc). Currently I believe (2) is required and (3) and (4) are coupled. For Windows (and I believe Linux), Chromium seems to want to disable all of these options. For Mac, Chromium seems to want (1), (2), and (4)---but not (3). 774035 5 thakis 2012-11-22 10:37:08 -0800 mac malloc questions -> +avi 774083 6 avi 2012-11-22 12:46:24 -0800 I'm not sure of the terminology you're using (global vs per-object new). On Mac Chrome we override the world when it comes to allocators, probably redundantly. http://src.chromium.org/viewvc/chrome/trunk/src/base/process_util_mac.mm We override: - C malloc/calloc/valloc/realloc/posix_memalign for default and purgeable zones - C++ operator new - Core Foundation CFAllocators - Cocoa +[NSObject allocWithZone:] If you have specific questions about implementation, ask away. 774216 7 abarth 2012-11-22 22:21:05 -0800 This bug is about Windows. The bug relating to Mac is bug 102866. 774217 8 abarth 2012-11-22 22:27:24 -0800 To answer your questions... > I'm not sure of the terminology you're using (global vs per-object new). These are WebKit-specific overrides in http://trac.webkit.org/browser/trunk/Source/WTF/wtf/FastMalloc.h#L259 and http://trac.webkit.org/browser/trunk/Source/WTF/wtf/FastAllocBase.h#L96 > On Mac Chrome we override the world when it comes to allocators, probably redundantly. In bug 102866, we observe a speedup by switch to using WebKit's copy of TCMalloc instead of defining the USE(SYSTEM_MALLOC) symbol. Based on profiles and looking at symbols, it appears that currently on Mac, WebKit uses the OS-provided malloc implementation, which isn't particularly fast. Is process_util_mac.mm supposed to cause WebKit to use a malloc implementation other than the system-provided implementation? (Please feel free to respond on bug 102866 as that bug is actually about Mac.) 774509 9 avi 2012-11-23 06:39:53 -0800 (In reply to comment #8) > Is process_util_mac.mm supposed to cause WebKit to use a malloc implementation other than the system-provided implementation? No, there is nothing currently in process_util_mac intended to do anything of the sort. 774997 10 175884 abarth 2012-11-25 09:05:57 -0800 Created attachment 175884 Causes many test to time out; need to investigate 775738 11 176043 abarth 2012-11-26 11:53:03 -0800 Created attachment 176043 Patch 775741 12 abarth 2012-11-26 11:53:50 -0800 I marked this patch cq- because I want to land the patch when the tree is quiet and I can see the perf effects clearly. 776124 13 176043 eric 2012-11-26 17:31:13 -0800 Comment on attachment 176043 Patch LGTM. Other ports which USE_SYSTEM_MALLOC might want this implied as well, but we can make that decision if/when others switch it on. Also, I wonder if these shouldn't be USE_ macros. ENABLE has historically been for features. 776412 14 176043 webkit.review.bot 2012-11-26 23:35:03 -0800 Comment on attachment 176043 Patch Clearing flags on attachment: 176043 Committed r135828: <http://trac.webkit.org/changeset/135828> 776413 15 webkit.review.bot 2012-11-26 23:35:08 -0800 All reviewed patches have been landed. Closing bug. 776663 16 webkit.review.bot 2012-11-27 05:41:40 -0800 Re-opened since this is blocked by bug 103405 776733 17 abarth 2012-11-27 07:30:15 -0800 I can't find any evidence that this improves performance on the perf bots. It's possible I screwed it up, but it's also possible the branch is extremely well-predicted and doesn't cost performance. 175884 2012-11-25 09:05:57 -0800 2012-11-26 11:53:01 -0800 Causes many test to time out; need to investigate bug-103027-20121125090338.patch text/plain 2134 abarth U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1NjY3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9G YXN0QWxsb2NCYXNlLmggYi9Tb3VyY2UvV1RGL3d0Zi9GYXN0QWxsb2NCYXNlLmgKaW5kZXggYTA4 MDRhZDNkN2EwM2FhNDcxNzcxNTg2ZmMxZDIyZGFiYmZlYmRhNi4uYWM2YWFmOWI3ZWMxYTA2ZTBi MDk0MDNmOWMyNGVhOGJlNDE0NDYzZCAxMDA2NDQKLS0tIGEvU291cmNlL1dURi93dGYvRmFzdEFs bG9jQmFzZS5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL0Zhc3RBbGxvY0Jhc2UuaApAQCAtOTMsNiAr OTMsOCBAQAogI2luY2x1ZGUgPHd0Zi9TdGRMaWJFeHRyYXMuaD4KICNpbmNsdWRlIDx3dGYvVHlw ZVRyYWl0cy5oPgogCisjaWYgRU5BQkxFKFBFUl9PQkpFQ1RfRkFTVE1BTExPQ19ORVcpCisKICNk ZWZpbmUgV1RGX01BS0VfRkFTVF9BTExPQ0FURUQgXAogcHVibGljOiBcCiAgICAgdm9pZCogb3Bl cmF0b3IgbmV3KHNpemVfdCwgdm9pZCogcCkgeyByZXR1cm4gcDsgfSBcCkBAIC0xMzEsNiArMTMz LDE0IEBAIHB1YmxpYzogXAogcHJpdmF0ZTogXAogdHlwZWRlZiBpbnQgX190aGlzSXNIZXJlVG9G b3JjZUFTZW1pY29sb25BZnRlclRoaXNNYWNybwogCisjZWxzZSAvLyAhRU5BQkxFKFBFUl9PQkpF Q1RfRkFTVE1BTExPQ19ORVcpCisKKyNkZWZpbmUgV1RGX01BS0VfRkFTVF9BTExPQ0FURUQgXAor cHJpdmF0ZTogXAordHlwZWRlZiBpbnQgX190aGlzSXNIZXJlVG9Gb3JjZUFTZW1pY29sb25BZnRl clRoaXNNYWNybworCisjZW5kaWYKKwogbmFtZXNwYWNlIFdURiB7CiAKICAgICAvLyBmYXN0TmV3 IC8gZmFzdERlbGV0ZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvUGxhdGZvcm0uaCBiL1Nv dXJjZS9XVEYvd3RmL1BsYXRmb3JtLmgKaW5kZXggMTJmZmJmMjA3ZWYzZjhjM2Y1ZjUxOTI3NTEw MGFmZTQ1ZmVlMjQ5Ny4uZjViYzIyZDdjMTAxNjQ2OTk1NTRjZjNiMDFlN2NjMWY1OTI5ZGYxOSAx MDA2NDQKLS0tIGEvU291cmNlL1dURi93dGYvUGxhdGZvcm0uaAorKysgYi9Tb3VyY2UvV1RGL3d0 Zi9QbGF0Zm9ybS5oCkBAIC01NzksMTUgKzU3OSwxNiBAQAogI2VuZGlmCiAKICNpZiBQTEFURk9S TShDSFJPTUlVTSkKLSNpZiBPUyhEQVJXSU4pCiAvKiBXZSBjYW4ndCBvdmVycmlkZSB0aGUgZ2xv YmFsIG9wZXJhdG9yIG5ldyBhbmQgZGVsZXRlIG9uIE9TKERBUldJTikgYmVjYXVzZQotICogc29t ZSBvYmplY3QgYXJlIGFsbG9jYXRlZCBieSBXZWJLaXQgYW5kIGRlYWxsb2NhdGVkIGJ5IHRoZSBl bWJlZGRlci4gKi8KLSNkZWZpbmUgRU5BQkxFX0dMT0JBTF9GQVNUTUFMTE9DX05FVyAwCi0jZWxz ZSAvKiAhT1MoREFSV0lOKSAqLwotLyogT24gbm9uLU9TKERBUldJTiksIHRoZSAic3lzdGVtIG1h bGxvYyIgaXMgYWN0dWFsbHkgVENNYWxsb2MgYW55d2F5LCBzbyB0aGVyZSdzCisgKiBzb21lIG9i amVjdCBhcmUgYWxsb2NhdGVkIGJ5IFdlYktpdCBhbmQgZGVhbGxvY2F0ZWQgYnkgdGhlIGVtYmVk ZGVyLiAKKyAqCisgKiBPbiBub24tT1MoREFSV0lOKSwgdGhlICJzeXN0ZW0gbWFsbG9jIiBpcyBh Y3R1YWxseSBUQ01hbGxvYyBhbnl3YXksIHNvIHRoZXJlJ3MKICAqIG5vIG5lZWQgdG8gdXNlIFdl YktpdCdzIGNvcHkgb2YgVENNYWxsb2MuICovCisjZGVmaW5lIEVOQUJMRV9HTE9CQUxfRkFTVE1B TExPQ19ORVcgMAorI2lmICFPUyhEQVJXSU4pCisjZGVmaW5lIEVOQUJMRV9QRVJfT0JKRUNUX0ZB U1RNQUxMT0NfTkVXIDAKICNkZWZpbmUgVVNFX1NZU1RFTV9NQUxMT0MgMQotI2VuZGlmIC8qIE9T KERBUldJTikgKi8KKyNlbmRpZiAvKiAhT1MoREFSV0lOKSAqLwogI2VuZGlmIC8qIFBMQVRGT1JN KENIUk9NSVVNKSAqLwogCiAjaWYgUExBVEZPUk0oSU9TKQpAQCAtODIzLDYgKzgyNCwxMCBAQAog I2RlZmluZSBFTkFCTEVfR0xPQkFMX0ZBU1RNQUxMT0NfTkVXIDEKICNlbmRpZgogCisjaWYgIWRl ZmluZWQoRU5BQkxFX1BFUl9PQkpFQ1RfRkFTVE1BTExPQ19ORVcpCisjZGVmaW5lIEVOQUJMRV9Q RVJfT0JKRUNUX0ZBU1RNQUxMT0NfTkVXIDEKKyNlbmRpZgorCiAjaWYgIWRlZmluZWQoRU5BQkxF X1BBUlNFRF9TVFlMRV9TSEVFVF9DQUNISU5HKQogI2RlZmluZSBFTkFCTEVfUEFSU0VEX1NUWUxF X1NIRUVUX0NBQ0hJTkcgMQogI2VuZGlmCg== 176043 2012-11-26 11:53:03 -0800 2012-11-26 23:35:03 -0800 Patch bug-103027-20121126115042.patch text/plain 3207 abarth U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1NzM3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IDAxYWY0ZmM0ZTQzNDE1YmY2NTQ3MWY0 ODRkZTRlMmY2NTQyMjJiYzUuLjNmZmNjODk3MGM5ZmM4NGFlZWI2NGFlMzVlYTZjOTExNzg3NTZl YzEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMjQgQEAKKzIwMTItMTEtMjYgIEFkYW0gQmFydGggIDxhYmFydGhA d2Via2l0Lm9yZz4KKworICAgICAgICBbQ2hyb21pdW1dIGZhc3RNYWxsb2MgaGFzIGFuIGV4dHJh IGJyYW5jaCBvbiBXaW5kb3dzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMDMwMjcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBUaGVyZSdzIG5vIG5lZWQgdG8gb3ZlcnJpZGUgdGhlIG5ldy9kZWxldGUgb3Bl cmF0b3JzIG9uIG5vbi1NYWMKKyAgICAgICAgcGxhdGZvcm0gYmVjYXVzZToKKworICAgICAgICAx KSBXZSB1c2UgdGhlIHN5c3RlbSBtYWxsb2MgYW55d2F5LgorICAgICAgICAyKSBXZSd2ZSBtb2Rp ZmllZCB0aGUgc3lzdGVtIG1hbGxvYyB0byBjcmFzaCBpbiBvdXQtb2YtbWVtb3J5IGNvbmRpdGlv bnMuCisKKyAgICAgICAgVGhpcyBwYXRjaCByZW1vdmVzIGEgYnJhbmNoIChhbmQgYSBjYWxsKSBm cm9tIG1hbGxvYywgd2hpY2ggd2lsbAorICAgICAgICBob3BlZnVsbHkgaW1wcm92ZSBwZXJmb3Jt YW5jZS4gSSBoYXZlbid0IG1lYXN1cmVkIHRoZSBwZXJmb3JtYW5jZQorICAgICAgICBjaGFyYWN0 ZXJpc3RpY3Mgb2YgdGhpcyBwYXRjaCwgYnV0IEkgd2lsbCBsb29rIGF0IHRoZSBncmFwaHMgY2xv c2VseQorICAgICAgICB3aGVuIGxhbmRpbmcuCisKKyAgICAgICAgKiB3dGYvRmFzdEFsbG9jQmFz ZS5oOgorICAgICAgICAqIHd0Zi9QbGF0Zm9ybS5oOgorCiAyMDEyLTExLTI2ICBQYXRyaWNrIEdh bnN0ZXJlciAgPHBhcm9nYUB3ZWJraXQub3JnPgogCiAgICAgICAgIEJ1aWxkIGZpeCBmb3IgV2lu Q0UgYWZ0ZXIgcjEzNTY0MC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL0Zhc3RBbGxvY0Jh c2UuaCBiL1NvdXJjZS9XVEYvd3RmL0Zhc3RBbGxvY0Jhc2UuaAppbmRleCBhMDgwNGFkM2Q3YTAz YWE0NzE3NzE1ODZmYzFkMjJkYWJiZmViZGE2Li5hYzZhYWY5YjdlYzFhMDZlMGIwOTQwM2Y5YzI0 ZWE4YmU0MTQ0NjNkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi9GYXN0QWxsb2NCYXNlLmgK KysrIGIvU291cmNlL1dURi93dGYvRmFzdEFsbG9jQmFzZS5oCkBAIC05Myw2ICs5Myw4IEBACiAj aW5jbHVkZSA8d3RmL1N0ZExpYkV4dHJhcy5oPgogI2luY2x1ZGUgPHd0Zi9UeXBlVHJhaXRzLmg+ CiAKKyNpZiBFTkFCTEUoUEVSX09CSkVDVF9GQVNUTUFMTE9DX05FVykKKwogI2RlZmluZSBXVEZf TUFLRV9GQVNUX0FMTE9DQVRFRCBcCiBwdWJsaWM6IFwKICAgICB2b2lkKiBvcGVyYXRvciBuZXco c2l6ZV90LCB2b2lkKiBwKSB7IHJldHVybiBwOyB9IFwKQEAgLTEzMSw2ICsxMzMsMTQgQEAgcHVi bGljOiBcCiBwcml2YXRlOiBcCiB0eXBlZGVmIGludCBfX3RoaXNJc0hlcmVUb0ZvcmNlQVNlbWlj b2xvbkFmdGVyVGhpc01hY3JvCiAKKyNlbHNlIC8vICFFTkFCTEUoUEVSX09CSkVDVF9GQVNUTUFM TE9DX05FVykKKworI2RlZmluZSBXVEZfTUFLRV9GQVNUX0FMTE9DQVRFRCBcCitwcml2YXRlOiBc Cit0eXBlZGVmIGludCBfX3RoaXNJc0hlcmVUb0ZvcmNlQVNlbWljb2xvbkFmdGVyVGhpc01hY3Jv CisKKyNlbmRpZgorCiBuYW1lc3BhY2UgV1RGIHsKIAogICAgIC8vIGZhc3ROZXcgLyBmYXN0RGVs ZXRlCmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybS5oIGIvU291cmNlL1dURi93 dGYvUGxhdGZvcm0uaAppbmRleCAxMmZmYmYyMDdlZjNmOGMzZjVmNTE5Mjc1MTAwYWZlNDVmZWUy NDk3Li5mNWJjMjJkN2MxMDE2NDY5OTU1NGNmM2IwMWU3Y2MxZjU5MjlkZjE5IDEwMDY0NAotLS0g YS9Tb3VyY2UvV1RGL3d0Zi9QbGF0Zm9ybS5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL1BsYXRmb3Jt LmgKQEAgLTU3OSwxNSArNTc5LDE2IEBACiAjZW5kaWYKIAogI2lmIFBMQVRGT1JNKENIUk9NSVVN KQotI2lmIE9TKERBUldJTikKIC8qIFdlIGNhbid0IG92ZXJyaWRlIHRoZSBnbG9iYWwgb3BlcmF0 b3IgbmV3IGFuZCBkZWxldGUgb24gT1MoREFSV0lOKSBiZWNhdXNlCi0gKiBzb21lIG9iamVjdCBh cmUgYWxsb2NhdGVkIGJ5IFdlYktpdCBhbmQgZGVhbGxvY2F0ZWQgYnkgdGhlIGVtYmVkZGVyLiAq LwotI2RlZmluZSBFTkFCTEVfR0xPQkFMX0ZBU1RNQUxMT0NfTkVXIDAKLSNlbHNlIC8qICFPUyhE QVJXSU4pICovCi0vKiBPbiBub24tT1MoREFSV0lOKSwgdGhlICJzeXN0ZW0gbWFsbG9jIiBpcyBh Y3R1YWxseSBUQ01hbGxvYyBhbnl3YXksIHNvIHRoZXJlJ3MKKyAqIHNvbWUgb2JqZWN0IGFyZSBh bGxvY2F0ZWQgYnkgV2ViS2l0IGFuZCBkZWFsbG9jYXRlZCBieSB0aGUgZW1iZWRkZXIuIAorICoK KyAqIE9uIG5vbi1PUyhEQVJXSU4pLCB0aGUgInN5c3RlbSBtYWxsb2MiIGlzIGFjdHVhbGx5IFRD TWFsbG9jIGFueXdheSwgc28gdGhlcmUncwogICogbm8gbmVlZCB0byB1c2UgV2ViS2l0J3MgY29w eSBvZiBUQ01hbGxvYy4gKi8KKyNkZWZpbmUgRU5BQkxFX0dMT0JBTF9GQVNUTUFMTE9DX05FVyAw CisjaWYgIU9TKERBUldJTikKKyNkZWZpbmUgRU5BQkxFX1BFUl9PQkpFQ1RfRkFTVE1BTExPQ19O RVcgMAogI2RlZmluZSBVU0VfU1lTVEVNX01BTExPQyAxCi0jZW5kaWYgLyogT1MoREFSV0lOKSAq LworI2VuZGlmIC8qICFPUyhEQVJXSU4pICovCiAjZW5kaWYgLyogUExBVEZPUk0oQ0hST01JVU0p ICovCiAKICNpZiBQTEFURk9STShJT1MpCkBAIC04MjMsNiArODI0LDEwIEBACiAjZGVmaW5lIEVO QUJMRV9HTE9CQUxfRkFTVE1BTExPQ19ORVcgMQogI2VuZGlmCiAKKyNpZiAhZGVmaW5lZChFTkFC TEVfUEVSX09CSkVDVF9GQVNUTUFMTE9DX05FVykKKyNkZWZpbmUgRU5BQkxFX1BFUl9PQkpFQ1Rf RkFTVE1BTExPQ19ORVcgMQorI2VuZGlmCisKICNpZiAhZGVmaW5lZChFTkFCTEVfUEFSU0VEX1NU WUxFX1NIRUVUX0NBQ0hJTkcpCiAjZGVmaW5lIEVOQUJMRV9QQVJTRURfU1RZTEVfU0hFRVRfQ0FD SElORyAxCiAjZW5kaWYK