bmalloc: tryFastMalloc shouldn't crash
Created attachment 248170 [details] Patch
Created attachment 248173 [details] Patch
Comment on attachment 248173 [details] Patch Attachment 248173 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6160743829340160 New failing tests: svg/custom/absolute-root-position-masking.xhtml js/regress/exit-length-on-plain-object.html
Created attachment 248176 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248173 [details] Patch Attachment 248173 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6145023175294976 New failing tests: fast/history/replacestate-nocrash.html fast/multicol/newmulticol/multicol-inside-multicol.html platform/mac-wk2/plugins/muted-state.html
Created attachment 248177 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 248173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248173&action=review > Source/WTF/wtf/FastMalloc.cpp:192 > + return bmalloc::api::malloc(size); Would it make sense for these to be in the header so they could be inlined?
> > Source/WTF/wtf/FastMalloc.cpp:192 > > + return bmalloc::api::malloc(size); > > Would it make sense for these to be in the header so they could be inlined? Currently, WTF::fastMalloc is an out-of-line function, but bmalloc::api::malloc is fully inlined into it. That's pretty good. We probably don't want to inline the whole malloc function -- it's a bit too big. But we probably do want to inline the size class calculation eventually, since it's constant.
Created attachment 248210 [details] Patch
Comment on attachment 248210 [details] Patch Attachment 248210 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5808178553946112 New failing tests: fast/multicol/newmulticol/multicol-inside-multicol.html
Created attachment 248220 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248210 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248210&action=review Tests seem to be failing on EWS. Please don’t land until you find out why. > Source/WTF/ChangeLog:14 > + * wtf/FastMalloc.cpp: > + (WTF::fastMalloc): > + (WTF::fastCalloc): > + (WTF::fastRealloc): > + (WTF::fastAlignedMalloc): Don't check for NULL; bmalloc will just crash > + for us if allocation fails, and we'd rather not pay the cost of an extra > + check. Very interesting. I had no idea! (As you can tell since I wrote all that wrong code.) > Source/WebKit2/Shared/ShareableBitmap.cpp:149 > + char* data = 0; nullptr > Source/WebKit2/Shared/ShareableBitmap.cpp:155 > + fastFree(m_data); Seems unfortunate that we can’t free the old data before allocating the new data; one advantage of the old realloc code path is that it combined the new allocation with the old deallocation. We don’t want to reuse the data, but it would be nice to be able to reuse the memory. Perhaps my naiveté about malloc is showing, though.
I think it’s strange that bmalloc has an entry point named malloc that is “crash on fail” rather than “return null on fail”. That differs from the traditional meaning of the malloc C library function although it matches the behavior of our fastMalloc function.
Created attachment 248224 [details] Patch
> Tests seem to be failing on EWS. Please don’t land until you find out why. OK. > > Source/WebKit2/Shared/ShareableBitmap.cpp:155 > > + fastFree(m_data); > > Seems unfortunate that we can’t free the old data before allocating the new > data; one advantage of the old realloc code path is that it combined the new > allocation with the old deallocation. We don’t want to reuse the data, but > it would be nice to be able to reuse the memory. Perhaps my naiveté about > malloc is showing, though. Since realloc must copy the contents of the old buffer, it's not allowed to free the old buffer before allocating the new one.
> I think it’s strange that bmalloc has an entry point named malloc that is > “crash on fail” rather than “return null on fail”. That differs from the > traditional meaning of the malloc C library function although it matches the > behavior of our fastMalloc function. Yeah. I never really thought through the crash vs return null behavior, and now that I think about it, it's probably not right to call the functions malloc and realloc -- it's more like mallocOrCrash and reallocOrCrash, or something.
Comment on attachment 248224 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248224&action=review > Source/WebKit2/Shared/ShareableBitmap.cpp:149 > + char* data = 0; nullptr
(In reply to comment #15) > > > Source/WebKit2/Shared/ShareableBitmap.cpp:155 > > > + fastFree(m_data); > > > > Seems unfortunate that we can’t free the old data before allocating the new > > data; one advantage of the old realloc code path is that it combined the new > > allocation with the old deallocation. We don’t want to reuse the data, but > > it would be nice to be able to reuse the memory. Perhaps my naiveté about > > malloc is showing, though. > > Since realloc must copy the contents of the old buffer, it's not allowed to > free the old buffer before allocating the new one. Still seems like a missed opportunity here. Not something we were doing before, but something we could do. Again, not sure if it’s realistic to expect one free to help the other allocate to succeed.
> Still seems like a missed opportunity here. Not something we were doing > before, but something we could do. Ah, I see. Yes, I think we could do it.
Created attachment 248245 [details] Patch
Committed r181272: <http://trac.webkit.org/changeset/181272>
Reopening to attach new patch.
Created attachment 248289 [details] Patch
This time for sure!
Created attachment 248290 [details] Patch
Committed r181307: <http://trac.webkit.org/changeset/181307>
This seems to have somehow broken ASan tests, with a nondescript error. I'm going to try rolling out. r181306 was fine, r181308 crashes on many tests.
Re-opened since this is blocked by bug 142525
Created attachment 248336 [details] Patch
Committed r181329: <http://trac.webkit.org/changeset/181329>