| Summary: | fastMalloc(0) should not be supported | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||
| Component: | New Bugs | Assignee: | Eric Seidel (no email) <eric> | ||||||||
| Status: | NEW --- | ||||||||||
| Severity: | Normal | CC: | abarth, andersca, ap, arun.patole, darin, mjs, yong.li.webkit, zoltan | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Other | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Eric Seidel (no email)
2011-02-23 15:48:28 PST
Created attachment 83569 [details]
wip
The idea for this patch came from finding and fixing bug 55091. Comment on attachment 83569 [details] wip View in context: https://bugs.webkit.org/attachment.cgi?id=83569&action=review > Source/WebCore/dom/Document.cpp:1272 > + return ""; Not String() or emptyString() ? > Source/WebCore/loader/DocumentWriter.cpp:222 > - // make sure nothing's left in there > + // FIXME: This is intended to flush the decoder. I'm not sure it's still needed though. > + // Certainly we could expose a cleaner way to do this. > addData(0, 0, true); I've wanted to kill this line of code for a while. I looked into it recently. It's non-trivial. (In reply to comment #3) > (From update of attachment 83569 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83569&action=review > > > Source/WebCore/dom/Document.cpp:1272 > > + return ""; > > Not String() or emptyString() ? I just matched what the rests of that function did. But I'm happy to change them all to emptyString() or whatever is the current best practice. It depends whether clients want an empty string or a null string. In most code, we return String() which is the null string, in cases like that. Comment on attachment 83569 [details] wip View in context: https://bugs.webkit.org/attachment.cgi?id=83569&action=review > Source/WebCore/loader/TextResourceDecoder.cpp:689 > + result = m_codec->decode(m_buffer.data(), m_buffer.size(), true, m_contentType == XML && !m_useLenientXMLDecoding, m_sawError); if (!m_buffer.isEmpty()) ? Comment on attachment 83569 [details] wip View in context: https://bugs.webkit.org/attachment.cgi?id=83569&action=review >> Source/WebCore/loader/TextResourceDecoder.cpp:689 >> - String result = m_codec->decode(m_buffer.data(), m_buffer.size(), true, m_contentType == XML && !m_useLenientXMLDecoding, m_sawError); >> + String result; >> + if (m_buffer.size()) >> + result = m_codec->decode(m_buffer.data(), m_buffer.size(), true, m_contentType == XML && !m_useLenientXMLDecoding, m_sawError); > > if (!m_buffer.isEmpty()) ? This change is wrong. Calling the text codec with flush equal to true can do work even if the data passed in has no length. + ASSERT(n); // fastMalloc(0) returns implementation-specific results and just wastes time. Could you please elaborate on how the results are implementation specific? It may or may not be a good idea to forbid fastMalloc(0) - the downsides I can think of is that more branching will be needed in code that could assume a non-null pointer before, and that fastMalloc will be less like malloc, confusingly to people working on the code. Updating the patch now. I think that if you want to make fastMalloc semantically different from malloc in this case, you also need to rename it from fastMalloc to WTF::allocate or something else that doesn't have "malloc" in it. (In reply to comment #10) > I think that if you want to make fastMalloc semantically different from malloc in this case, you also need to rename it from fastMalloc to WTF::allocate or something else that doesn't have "malloc" in it. The C Standard says: 7.20.3 Memory management functions If the size of the space requested is zero, the behavior is implementation defined: either a null pointer is returned, or the behavior is as if the size were some nonzero value, except that the returned pointer shall not be used to access an object. I think it's valid for our implementation to ASSERT. We can obviously leave the if (!size) return 0; if in. Right now on some platforms our fastMalloc returns 0, and others it returns fastMalloc(1). In all cases, it does unnecessary work. So even if we don't change our behavior, I think we should ASSERT to make callers check size == 0 and possibly avoid lots of work. In at least bug 55091 avoiding malloc(0) was a big perf win. I'll post a patch shortly and start a discussion on webkit-dev. If we make this change, it should be widely agreed upon (hence the soon-to-be-started thread). Thanks Eric. (In reply to comment #11) > I think it's valid for our implementation to ASSERT. We can obviously leave the if (!size) return 0; if in. I meant "if desired" here. We could mark said if as UNLIKELY() as well. In any case, we shouldn't hit it unless we're using fastMalloc for code we don't control (which I'm not sure if we are or not). I also forgot a link to the spec: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf See however the C++ standard, section 3.7.3.1.2 about allocation functions: "If the size of the space requested is zero, the value returned shall not be a null pointer value." Certainly, "new" does not necessarily need to use fastMalloc directly, but there isn't a lot of WebKit code that uses fastMalloc. It’s clear and not debatable that it’s legal to call malloc(0) and then call free() on the result of malloc(0). The fact that the value returned is undefined and may be a null pointer is not really material, in my opinion. If we make fastMalloc(0) illegal, then anyone porting code that previously used malloc to use fastMalloc will have to consider whether the value could be 0. The issue is not “code we don’t control” but rather any code we incorporate and change to call fastMalloc. However, fastMalloc already is different from malloc in one respect. The fastMalloc function crashes if memory is not available. The real malloc is like tryFastMalloc in that it returns 0 if there is no memory available. The argument for making fastMalloc(0) illegal is that we think that calling it is often a programming mistake, and we think that a runtime assertion is a good way to catch that mistake. The arguments for making fastMalloc(0) legal are that we may want to port code using malloc without changing the semantic of a zero size and that we think there are code paths that have a size that is sometimes zero and sometimes non-zero where it is cleaner to be able to malloc and free without having to add a special case for zero. Created attachment 96056 [details]
another wip
Created attachment 96139 [details]
Patch
Two issues prevent us from turning this on for the Mac port. 1. Safari crashes on exit in some safari-specific code I can't see. Running inspector/audits/audits-panel-functional.html may make that more likely. 2. Any use of the Java plugin will cause WebKitPluginHost to crash on this ASSERT (again, closed-source code). Comment on attachment 96139 [details] Patch Changes that had to be made to accommodate this mode do not look like they are improving code quality. Besides, what about my comment 44? Excuse me, but is there any test results showing how much performance gain can be given by this patch? Comment on attachment 96139 [details]
Patch
Clearing the review flag for now. There seems to be some question about whether to make this change more than a discussion about this particular patch.
|