Bug 142443 – bmalloc: tryFastMalloc shouldn't crash

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Geoffrey Garen]

bmalloc: tryFastMalloc shouldn't crash

[Geoffrey Garen]

Created attachment 248170 [details]
Patch

[Geoffrey Garen]

Created attachment 248173 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

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?

[Geoffrey Garen]

> > 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.

[Geoffrey Garen]

Created attachment 248210 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Darin Adler]

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.

[Geoffrey Garen]

Created attachment 248224 [details]
Patch

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

> 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.

[Darin Adler]

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

[Darin Adler]

(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.

[Geoffrey Garen]

> 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.

[Geoffrey Garen]

Created attachment 248245 [details]
Patch

[Geoffrey Garen]

Committed r181272: <http://trac.webkit.org/changeset/181272>

[Geoffrey Garen]

Reopening to attach new patch.

[Geoffrey Garen]

Created attachment 248289 [details]
Patch

[Geoffrey Garen]

This time for sure!

[Geoffrey Garen]

Created attachment 248290 [details]
Patch

[Geoffrey Garen]

Committed r181307: <http://trac.webkit.org/changeset/181307>

[Alexey Proskuryakov]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 142525

[Geoffrey Garen]

Created attachment 248336 [details]
Patch

[Geoffrey Garen]

Committed r181329: <http://trac.webkit.org/changeset/181329>