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

Tue Oct 21 01:38:08 PDT 2008

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

------- Comment #28 from ppedriana at gmail.com  2008-10-21 01:38 PDT -------
The following discussion follows Darin's comment #12. It references a new
revision of the proposed allocation interface which is not yet posted. This is
something I can work on right away and which I've done some work on already.
But some of the items below might require a little feedback before a revision
is completed and posted with some confidence. 

Regarding the name AllocBase and fastMalloc, are you suggesting a base class
called FastMallocBase? That seems fine and consistent with the use of
fastMalloc. It would make it somewhat clear that its intent is to use
fastMalloc. I will AllocBase to FastMallocBase for the new revision. 

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? 

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. 

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. 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. But that won't work for most users. 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. 

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. The
functionality there is defined to be identical to the new C++ standard, with
the only difference being that it is in the WTF namespace. And the new C++
functionality is imported into WTF if present. So I think it's a good idea to
match the new C++ standard functionality.

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. I'm not aware of a way to avoid
having the four types defined, but I'm pretty sure there are only these four
variations and no others. Granted, the code will execute correctly regardless
of the omission of the volatile variations; it just would use a little extra
memory. I will nevertheless add the volatile versions to be complete. 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. 

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

I can add a space in between the braces and remove the lining up easily enough.

I will get rid of that mistaken "We don't need to check for a null pointer"
comment.

I suppose "whereby" isn't right; that can be easily changed.

Ditto for the 'const' in local variable declarations. 

Yes the cast in NewArrayImpl<T, false, true> is redundant.

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. The built-in
compiler generated array new is going to be doing something similar if not
identical to this in a lot of cases, so adding a prefix of 8 bytes shouldn't be
adding any more space than the compiler or standard library is already adding
on its own (I just verified with GCC 4.1 on PowerPC). The reason it is 8 bytes
even on a 32 bit platform is for this alignment preservation. Exactly how
compilers deal with this is compiler-specific, but they all face the problem
that non-PODs must be aligned. 

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? It will be useful to
document that fastFree or the user's equivalent can handle NULL so that
FastNew.h doesn't need to. 

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