[Webkit-unassigned] [Bug 20422] Patch to allow custom memory allocation control

Tue Oct 21 11:32:17 PDT 2008

https://bugs.webkit.org/show_bug.cgi?id=20422

------- Comment #29 from darin at apple.com  2008-10-21 11:32 PDT -------
Comments just on your comments. I didn't reread my original comments to make
sure you addressed all of them.

(In reply to comment #28)
> Regarding the name AllocBase and fastMalloc, are you suggesting a base class
> called FastMallocBase? That seems fine and consistent with the use of
> fastMalloc.

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 fromthis makes new/delete use
FastMalloc by default for objects of this class. Because, as you know, that's
all it does: Saves 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.

> 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 should it be 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.

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

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

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

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

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

> Regarding the const version of has_trivial_constructor, yes volatile versions
> are needed to be complete. I left that out of the first proposal for expediency
> but left a comment in there about volatile.

Sorry, I overlooked the comment about volatile.

> Note that
> the most recent versions of GCC and VC++ (and IBM, CodeWarrior, and Dinkumware)
> for desktop platforms have built-in support for these type traits as per the
> new C++ standard.

I think the best way to handle this is to make a header that supplies the new
standard stuff as needed for compilers that lack it. Analogous to the
MathExtras.h header. Then we can just use this almost as if the standard was
present on all the platforms. If we can get away with actually defining it
inside the std namespace on the non-compliant platforms, that's good. Or we can
import it into the WTF namespace on the platforms that have it, and define it
in the WTF namespace for platforms that don't.

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

> 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 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 ;-)

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

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