[Webkit-unassigned] [Bug 20422] Patch to allow custom memory allocation control
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 20 21:13:07 PST 2008
https://bugs.webkit.org/show_bug.cgi?id=20422
------- Comment #30 from ppedriana at gmail.com 2008-11-20 21:13 PDT -------
We (Zoltan Horvath and I) have produced a new patch (AllAllocation_v3.patch)
which addresses the previous review comments. This patch includes the primary
allocator and type traits-related code as well as a working patch of WebKit
(revision 38334) to use the allocator code and which serves to demonstrate
feasibility. The core of the patch involves the following files:
FastAllocBase.h
Primary header
TypeTraits.h
New file which includes improved traits moved from HashTraits.h
FastMalloc.h
Implements FAST_MALLOC_MATCH_VALIDATION
FastMalloc.cpp
Implements FAST_MALLOC_MATCH_VALIDATION
Platform.h
Provides definition of ENABLE(FAST_MALLOC_MATCH_VALIDATION)
HashTraits.h
moved is_integer to to TypeTraits.h
Additionally there is FastAllocBaseTest.cpp which is a standalone test of the
functionality.
A possibility is to apply this patch in two phases: 1) just the changes above
with 2) other changes to follow. We felt it was useful to produce a
demonstration of the patch which is carried through to all affected WebKit
files to demonstrate ease and practicality of implementation. Zoltan can
discuss the implementation of it better than me, since he did most of the work
there and was the producer of the current .patch file.
Zoltan could also speak more about observed performance effects, though I'd be
surprised if performance changed in a measurable way. When
FAST_MALLOC_MATCH_VALIDATION is enabled I expect that memory usage could
increase by perhaps as much as 1-2% for patterns that include a lot of small
allocations.
Most of the following discussion refers to FastAllocatorBase.h. Hopefully it
isn't too verbose.
>> I wrote:
>> Regarding the name AllocBase and fastMalloc, are you suggesting a base
>> class called FastMallocBase? That seems fine and consistent with the
>> use of fastMalloc.
> Darin wrote:
> Sounds good. I don't love the name fastMalloc, but I do love consistency
> and clarity about the relationship between these.
>
> Maybe FastAllocBase would be better -- the "m" makes it even more jargony
> without adding much -- but FastMallocBase seems fine. Maybe there's an
> even better name that makes it clear that deriving from this makes
> new/delete use FastMalloc by default for objects of this class. Because,
> as you know, that's all it does: Save you having to use fastNew/fastDelete
> explicitly. We don't use the word "Base" in classes like "Noncopyable",
> so I think it's worth thinking at least a tiny bit further about what a
> more straightforward name would be; maybe it wouldn't include Base at all.
I've got it as FastAllocBase now. I'm trying to think of a name that doesn't
include "Base" but conveys some meaning. "Noncopyable" works well for its
purpose, and an analog for an allocation base class might be something like
"FastAllocated". I did notice that GenericHashTraitsBase is the base for
HashTraits in HashTraits.h.
>> Since the file name New.h needs to be changed, I can follow suit with
>> this and rename it FastNew.h for the new revision. Or FastMallocNew.h?
> I think you should name the file after the class. So FastAllocBase.h
> (or whatever we end up naming it) even though it will contain helpful
> function templates too, not just the class. The general pattern in the
> WebKit project overall is to name files after classes, and it seems we
> can follow that here.
I've got it as FastAllocBase.h, though it could be changed to something
like FastAllocated.h.
>> Instead of newObject/deleteObject, I wonder if there is a way to use the
>> fastMalloc name here. Such a name might be
>> fastNew/fastDelete/fastNewArray/fastDeleteArray. I can't think of a
>> simple way to avoid having special names for Array versions, as both the
>> object and array delete functions take the same argument: T*. For the
>> new revision I will switch it to fastNew, etc. and refer to it as such
>> from here on.
> Seems fine. While I don't love the "fast" word, I think that using it
> consistently as a theme will help people understand how to use it.
It is now implemented as fastNew, etc.
>> Being able to detect misuse (e.g. new/delete mismatches) at compile time
>> is tough (given how C++ works), though detecting it at runtime isn't
>> very difficult.
> Yes, that's my impression too. But lets not give up on compile time
> just yet. But I am still hoping to find a creative solution to detecting
> the misuse at compile time, although I haven't thought of anything.
> In the past we have done some clever things with macros that worked
> just fine. For example, we could do something that involves defining new
> and delete macros to prevent accidental direct use of built-in new, and
> somehow exempt code that needs to do this. I don't want to give up on this
> yet, so please do think about it while you do the rest of the project.
> Keep in mind, too, that something to detect misuse at compile time is
> useful even if it isn't possible on all platforms.
Lets definitely do the work to detect it at runtime.
There's now code to detect mismatches of allocation. It's enabled via
#if ENABLE(FAST_MALLOC_MATCH_VALIDATION) and has undergone some testing.
The implementation is fairly neat in that it allows any heap implementation
to support it; it isn't limited to the FastMalloc heap.
>> For some users it's enough to simply have global operator new and
>> delete assert(false), and that's what some embedded and game developers
>> do application-wide.
> And this will probably work on Mac OS X, since the tool chain supports a
> global operator new that is not actually global, but is instead local to
> the WebKit, WebCore, and JavaScriptCore frameworks. So we should do it
> on that platform, where a lot of the WebKit development is done.
I think this is something that would be done outside this patch in the
application's or library's new/delete implementation. My testing has included
testing this and it works as expected.
>> A practical solution could involve a magic number approach whereby the
>> four bytes prior to an allocation identifies how the allocation was done.
>> Such an approach could tell not just if there was a mismatch between
>> fastNew and fastDelete, but also could potentially be used to tell if
>> there was a mistaken use of fastDelete instead of fastFree or vice versa.
>> I can implement this for the new revision without much trouble. It can
>> be controlled by a define which defaults to disabled in NDEBUG and which
>> is tested by #if ENABLE(FAST_NEW_VALIDATION) or a suitable alternative.
> Yes, worth doing. There are many misuses it won't detect, though.
> For example, if a programmer use fastNew to allocate and then uses the
> standard compiler delete to deallocate there's no guarantee it would be
> caught; those are some of the most likely types of mistakes. But it would
> be great to catch things like using fastNew and then pairing it with
> fastFree, so I think we want to do this in FastMalloc too, not just the
> new functions. We should think further about how to catch more types of
> mistake.
It turns out that fastNew followed be delete or free is being detected at
the glibc level and msvc CRT level in our tests. We've tested the other
combinations and they do the right thing.
>> Regarding underscores in integral_constant, true_type, and false_type,
>> these match the C++ TR1, as documented at:
>> http://msdn.microsoft.com/en-us/library/bb982910.aspx, for example.
> Sounds fine. There should be a comment explaining this.
Done
>> In looking at HashTraits.h I can see the aforementioned IsInteger type
>> traits. It's just checking for int and not const int, volatile int,
>> const volatile int. So it will misdetect for the latter three types.
>> Since compilers are now providing explicit support for C++ standard
>> type traits, I wonder if it would be useful to have HashTraits.h also
>> defer to the compiler/standard library when possible. Would it be useful
>> to have a TypeTraits.h header file which is shared by HashTraits.h and
>> FastNew.h. I can implement that and add the const/volatile is_integral
>> variations (recall that TR1 calls it is_integral).
> Yes, a shared implementation would be better than just leaving this
> existing one in place while we add a new one.
This is implemented in TypeTraits.h and it includes const/volatile traits
for both HashTraits' is_integral and FastMallocBase's has_trivial_constructor.
>> Regarding the uint64_t thing, it's there to provide an array size prefix
>> to arrays of non-POD types, while preserving C++'s "suitably aligned"
>> allocation requirement. I can switch that to use a type which is a union
>> of void*/long long/double/int64_t, and that would be reasonably portable.
> Why not use "unsigned long long" instead of uint64_t? Is uint64_t more
> portable in some concrete way? Also, is there any platform where double
> is not sufficient? I think the specific "64" in uint64_t places the wrong
> emphasis. It's not the concrete size that matters, but rather "aligned
> enough for all built in types", and uint64_t does not say that to me any
> more than double would.
I've used a templated union of unsigned long long and T. This should address
size and aliasing issues.
>> Should the delete function (previously deleteObject, to be changed to
>> fastDelete) check for a NULL pointer? It seems to me that it needs to do
>> so by default. I agree that using "Since we intend to use this wherever
>> we'd use C++ delete, allow a null pointer as it does" is a better comment
>> to have there. I can pretty easily add a version that avoids the NULL
>> pointer check. It's only a matter of what to call it. Is there a
>> precedent in WebKit for this kind of naming? fastDeleteSafe?
>> fastDeleteValid? fastDeletePtr?
> I couldn't find any good precedent. There's gcProtect and
> gcProtectNullTolerant, but: 1) those names are too long, 2) that's
> backwards because the standard new/delete are null tolerant. What we need
> names for are versions of delete and free that are *not* null-tolerant.
> I would suggest these:
> fastNonNullFree
> fastNonNullDelete
> Clarity is more important than brevity, I think. The above are ugly, but
> not too terrible.
>
> An aside: We don't want the word "safe" in function names, because that
> implies that all other functions are possibly "unsafe"; I don't want to
> create a sense of vague dread about all the other functions ;-)
I've added fastNonNullDelete to match fastDelete. I wonder if having this
is necessary or if it's better to simply require that the allocator
implementation always act like fastFree and check for a NULL pointer.
>> It will be useful to document that fastFree or the user's equivalent can
>> handle NULL so that FastNew.h doesn't need to.
> I think it should already be clear that since fastFree is an
> implementation of free, and free requires handling of null pointers,
> that fastFree is obligated to handle null pointers. But maybe you're just
> saying we need a comment to that effect in FastMalloc.h; you could include
> that in your patch. Or are you saying something else?
I've added a comment clarifying that the underlying allocator must check
for NULL for the use of FastDelete.
>> + T* const p = static_cast<T*>(fastMalloc(sizeof(T) * count));
> We normally don't mark local variables const, even if we don't plan to
> modify them. Is there a reason you're doing that here and not elsewhere?
This is now fixed.
>> + return reinterpret_cast<T*>(p);
> Why is there a cast here (in NewArrayImpl<T, false, true>? I believe p
> already has the right type.
This is now fixed.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
More information about the webkit-unassigned
mailing list