[webkit-reviews] review requested: [Bug 20422] Patch to allow custom memory allocation control : [Attachment 27097] Updated FastMalloc patch.

[bugzilla-daemon at webkit.org]

Paul Pedriana <ppedriana at gmail.com> has asked  for review:
Bug 20422: Patch to allow custom memory allocation control
https://bugs.webkit.org/show_bug.cgi?id=20422

Attachment 27097: Updated FastMalloc patch.
https://bugs.webkit.org/attachment.cgi?id=27097&action=review

------- Additional Comments from Paul Pedriana <ppedriana at gmail.com>
We have an updated patch, which has the source and build file 
changes in one patch. Some comments, FWIW:

    > + 	   fastFree(p);  // We don't need to check for a null pointer;
    the compiler does this.

    The comment is curious, because fastFree also checks for a null 
    pointer. I'm not really sure what we're saying with the comment.

It has been removed. It didn't really need to be said.


    > +        void* p = fastMalloc(sizeof(T));
    > +
    > +        if (p) {
    > + 	   fastMallocMatchValidateMalloc(p,
Internal::AllocTypeFastNew);
    > + 	   return ::new(p) T;
    > +        }
    > +
    > +        return 0;

    In general in this project, we prefer the "early return" style, 
    so we'd check for null and return 0, then put the success case 
    code in a straight line rather than nested in an if statement.

I changed the FastAllocBase code to use the early return style. 
I might point out that the early return style results in slightly 
slower code with most compilers, including GCC and VC++. The reason 
is that branch prediction expects the 'true' case to be executed
and optimizes for that. Code with if statements which are false pay 
a small branch prediction penalty. Some processors (e.g. PowerPC) 
have instruction variants which tell the processor to reverse this 
expectation, but you need to use things like GCC's __builtin_expect 
or assembly language to access them. I realize you're probably aware
of this already, but I'm mentioning it because that's why in the past
I've often written code that way.


    > +    if(static_cast<size_t>(~0) - sizeof(AllocAlignmentInteger) <= n)  
      // If overflow would occur...

    We put a space after the if and before the "(" character.

    It's OK to use static_cast<size_t>(~0), but it doesn't seem better 
    than numeric_limits<size_t>::max() to me.

OK. Done on both accounts.



    > +    n += sizeof(AllocAlignmentInteger);
    > +    char* result = static_cast<char*>(malloc(n));

    I think it's slightly better to write this as:

	char* result = static_cast<char*>(malloc(sizeof(AllocAlignmentInteger)
+
    n));

    rather than modifying the argument. On the minus side, it makes 
    the line of code a little more complex, but on the plus side it 
    avoids modifying an argument which can sometimes be confusing. 

I changed the code to use the (sizeof(AllocAlignmentInteger) + n) form.

    I'm also not sure that making the result be a char* is 
    all that valuable, since we have to cast it with reinterpret_cast. 
    If we made it a void* we could use static_cast instead.

    > +        *reinterpret_cast<AllocAlignmentInteger*>(result) = 
	 Internal::AllocTypeMalloc;
    > +        result += sizeof(AllocAlignmentInteger);

    With void* this would be:

	*static_cast<AllocAlignmentInteger*>(result) = 
	    Internal::AllocTypeMalloc;
	result = static_cast<AllocAlignmentInteger*>(result) + 1;

    Or:

	AllocAlignmentInteger* header = static_cast<AllocAlignmentInteger*>
	    (result);
	*header = Internal::AllocTypeMalloc;
	result = header + 1;

OK. Done.


    > +        AllocAlignmentInteger* pValue =
	  Internal::fastMallocMatchValidationValue(p);
    > +        if (*pValue != Internal::AllocTypeMalloc)
    > + 	   Internal::fastMallocMatchFailed(p);
    > +        free(pValue);

    We almost never use the "Hungarian" style where a varible is 
    named "pType", which you're using here. I'd personally prefer 
    a letter or a word, maybe "header".

header seems fine. Done.


    > +    new_ptr = static_cast<char*>(new_ptr) + 
	sizeof(AllocAlignmentInteger);

    I don't see a good reason to involve char* here. I'd use this:

	new_ptr = static_cast<AllocAlignmentInteger*>(new_ptr) + 1;

OK. Done.


    > + const AllocAlignmentInteger* pType = reinterpret_cast<const 
	 AllocAlignmentInteger*>(static_cast<const char*>(p) - 
	 sizeof(AllocAlignmentInteger));

    Again, unnecessary use of char* and reinterpret_cast here.

	const AllocAlignmentInteger* pType = static_cast<const 
	    AllocAlignmentInteger*>(p) - 1;

Yep. The latter is significantly better. 


    > +    // These could alternatively be inline functions which do nothing.
    > +    #define fastMallocMatchValidateMalloc(p, type)
    > +    #define fastMallocMatchValidateFree(p, type)

    And they should be. We'd prefer empty inline functions 
    to macros for this sort of thing.

OK. Done.


    >  /* ENABLE macro defaults */
    > +#if !defined(ENABLE_FAST_MALLOC_MATCH_VALIDATION)
    > +#define ENABLE_FAST_MALLOC_MATCH_VALIDATION 0
    > +#endif

    Our platform header is really getting out of hand. 
    A long list of things with no documentation about what 
    they are! Not that your single addition makes it
    significantly worse.

I'm a fan of more in-code documentation. I didn't put anything 
there because there wasn't existing similar documentation. 
I put a two line comment explaining what this feature does.


    I think there were just enough comments that I'll say 
    review-, although I think this is in great shape and 
    nearly ready to go. I don't think there's any one comment 
    above that's important, but I'm hoping you'll want to take 
    at least one of my suggestions above.

I did them all.