[webkit-reviews] review denied: [Bug 20422] Patch to allow custom memory allocation control : [Attachment 25333] Patch against revision 38334

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Paul Pedriana <ppedriana at gmail.com>'s
request for review:
Bug 20422: Patch to allow custom memory allocation control
https://bugs.webkit.org/show_bug.cgi?id=20422

Attachment 25333: Patch against revision 38334
https://bugs.webkit.org/attachment.cgi?id=25333&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    class HeavyProfile : public Profile {
> +    class HeavyProfile : public Profile, public WTF::FastAllocBase {

Instead of having all the classes that derive from Profile derive from
FastAllocBase, I suggest having Profile derive from FastAllocBase.

> -    struct CallIdentifier {
> +    struct CallIdentifier : public WTF::FastAllocBase {

This class is never used with new/delete, so I'm not sure it's helpful to
derive from FastAllocBase. What's the rule of thumb you're using to decide when
to derive from it? I'm thinking we should start with classes that are used with
"new".

> -    class StructureID : public RefCounted<StructureID> {
> +    class StructureID : public RefCounted<StructureID>, public
WTF::FastAllocBase {

This class has been renamed to Structure, so this patch is a bit stale.

> Index: JavaScriptCore/runtime/Identifier.h
> ===================================================================
> --- JavaScriptCore/runtime/Identifier.h	(revision 38334)
> +++ JavaScriptCore/runtime/Identifier.h	(working copy)
> @@ -23,12 +23,13 @@
>  
>  #include "JSGlobalData.h"
>  #include "UString.h"
> +#include <wtf/FastAllocBase.h>

Since UString.h already includes this header, the new include is redundant.
There's a script, find-extra-includes, that you can use to find redundant
includes of this file. You could do "find-extra-includes | grep FastAllocBase"
and eliminate any redundant includes before you put the next patch up for
review.

> -    Rep* r = new Rep;
> +    Rep* r = fastNew<Rep>();

The Rep struct should be changed to inherit from FastAllocBase.

> -    , arrayTable(new HashTable(JSC::arrayTable))
> +    , arrayTable(fastNew<HashTable>(JSC::arrayTable))

The HashTable struct should be changed to inherit from FastAllocBase.

> -    class JSValue : Noncopyable {
> +    class JSValue : Noncopyable, public WTF::FastAllocBase {

No JSValue object or object of any class derived from it can be allocated with
a plain "new". These objects are garbage-collected. So there's no need to add
the base class here.

> -class MainThreadInvoker : public QObject {
> +class MainThreadInvoker : public QObject, public FastAllocBase {

Is this class used with "new"? I couldn't find any place where it was.

> -class ThreadCondition : Noncopyable {
> +class ThreadCondition : Noncopyable, public WTF::FastAllocBase {

I can't find any use of this class with "new".

> +    template <typename T> struct has_trivial_constructor : public
integral_constant<bool, __has_trivial_constructor(T)>{ };

Missing spaces between the ">" and the "{" on many lines in this file.

> +namespace WTF {
> +
> +    COMPILE_ASSERT(is_integral<bool>::value, WTF_IsInteger_bool_true);
> +    COMPILE_ASSERT(is_integral<const bool>::value, WTF_IsInteger_bool_true);

> +    COMPILE_ASSERT(is_integral<volatile bool>::value,
WTF_IsInteger_bool_true);
> +    COMPILE_ASSERT(is_integral<const volatile bool>::value,
WTF_IsInteger_bool_true);
> +    COMPILE_ASSERT(is_integral<char>::value, WTF_IsInteger_char_true);
> +    COMPILE_ASSERT(is_integral<signed char>::value,
WTF_IsInteger_signed_char_true);
> +    COMPILE_ASSERT(is_integral<unsigned char>::value,
WTF_IsInteger_unsigned_char_true);
> +    COMPILE_ASSERT(is_integral<short>::value, WTF_IsInteger_short_true);
> +    COMPILE_ASSERT(is_integral<unsigned short>::value,
WTF_IsInteger_unsigned_short_true);
> +    COMPILE_ASSERT(is_integral<int>::value, WTF_IsInteger_int_true);
> +    COMPILE_ASSERT(is_integral<unsigned int>::value,
WTF_IsInteger_unsigned_int_true);
> +    COMPILE_ASSERT(is_integral<long>::value, WTF_IsInteger_long_true);
> +    COMPILE_ASSERT(is_integral<unsigned long>::value,
WTF_IsInteger_unsigned_long_true);
> +    COMPILE_ASSERT(is_integral<long long>::value,
WTF_IsInteger_long_long_true);
> +    COMPILE_ASSERT(is_integral<unsigned long long>::value,
WTF_IsInteger_unsigned_long_long_true);
> +
> +#if !defined(_MSC_VER) || defined(_NATIVE_WCHAR_T_DEFINED)
> +    COMPILE_ASSERT(is_integral<wchar_t>::value, WTF_IsInteger_wchar_t_true);

> +#endif
> +
> +    COMPILE_ASSERT(!is_integral<char*>::value,
WTF_IsInteger_char_pointer_false);
> +    COMPILE_ASSERT(!is_integral<const char*>::value,
WTF_IsInteger_const_char_pointer_false);
> +    COMPILE_ASSERT(!is_integral<volatile char*>::value,
WTF_IsInteger_volatile_char_pointer__false);
> +    COMPILE_ASSERT(!is_integral<double>::value, WTF_IsInteger_double_false);

> +    COMPILE_ASSERT(!is_integral<float>::value, WTF_IsInteger_float_false);
> +
> +} // namespace WTF

Could we put these assertions in a .cpp file instead of in the header? I don't
think we need to compile these compile time assertions in every single file
that includes TypeTraits.h.

> +    // This defines a type which holds an unsigned integer and is the same 
> +    // size as the minimally aligned memory allocation. It can be used as 
> +    // a size prefix for array allocations.
> +    typedef unsigned long long AlignedAllocSize;

I think that the word "Size" in this name makes it sound like it's a block
size. That's not what this is at all. Maybe MaximallyAlignedInteger would be a
better name for this type? Or AllocAlignmentInteger?

Also, I don't understand what the comment "can be used as a size prefix" means,
since you're storying the AllocType byte there, not a size.

> +    inline AllocType getFastMallocMatchValidationType(const void* p)
> +    {
> +	   const char* pType = static_cast<const char*>(p) -
sizeof(AlignedAllocSize);
> +	   return static_cast<AllocType>(*pType);
> +    }

WebKit style guidelines onit the word "get" in a function name like this one.

Since AllocType is used for validation, I think you should use longer values
instead of just a byte, and more unusual numbers, to decrease the chance of a
false negative.

Why not put AllocType and these functions into the Internal namespace?

> +#include <wtf/TypeTraits.h>
> +
> +
> +namespace WTF {

We normally don't put two blank lines in a place like this. I noticed otehr
examples of that, and I think you should omit those also.

> +#if ENABLE(FAST_MALLOC_MATCH_VALIDATION)
> +	       if (p)
> +		   setFastMallocMatchValidationType(p, AllocTypeClassNew);
> +#endif

I'd like to see this ditty in an inline function so that FastAllocBase can
share it with fastNew without repeating the code twice. This would also have
the benefit of moving the #if out of those functions.

> +#if ENABLE(FAST_MALLOC_MATCH_VALIDATION)
> +	       if (p) {
> +		   if (getFastMallocMatchValidationType(p) !=
AllocTypeClassNew)
> +		       fastMallocMatchFailed(p);
> +		   setFastMallocMatchValidationType(p, AllocTypeMalloc);  //
Set it to this so that fastFree thinks it's OK.
> +	       }
> +#endif

Same comment here.

> +    /* Disabled until it's known that we want to actually do this.
> +    template <typename T, typename Arg1>
> +    inline T* fastNew(Arg1 arg1)
> +    {
> +	   void* p = fastMalloc(sizeof(T));
> +
> +	   if (p) {
> +#if ENABLE(FAST_MALLOC_MATCH_VALIDATION)
> +	       if (p)
> +		   setFastMallocMatchValidationType(p, AllocTypeFastNew);
> +#endif
> +	       return ::new(p) T(arg1);
> +	   }
> +
> +	   return 0;
> +    }
> +    */

It looks like you have this commented out, but later in the same file you have
one that's not commented out.

> +		   void* p = fastMalloc(sizeof(AlignedAllocSize) + (sizeof(T) *
count));
> +		   ArraySize<T> a = { static_cast<AlignedAllocSize*>(p) };
> +
> +		   if (p) {
> +#if ENABLE(FAST_MALLOC_MATCH_VALIDATION)
> +		       setFastMallocMatchValidationType(p,
AllocTypeFastNewArray);
> +#endif
> +		       *a.size++ = count;

I don't understand how you can store both the array size and the validation
type in the same place; "p" and "a.size" both point to the same address. Maybe
I'm missing something obvious here.

> +	   if(p) { // We need to test p here because we dereference it.

Needs a space after the "if".

> +		   if(p) { // We need to test p here because we dereference it.


Needs a space after the "if".

> +		       while(pEnd-- != p)

Needs a space after the "while".

> +    template <typename T>
> +    inline void fastNonNullDelete(T* p)
> +    {
> +	   if (p) {

Given "NonNull" I don't understand why there's a null check here. Maybe
"NonNull" means something different than I think it does.

> +		       while(pEnd-- != p)

Needs a space after the "while".

> +    n += sizeof(AlignedAllocSize);  // A pathologically high n value could
result in this overflowing.

I think we need code to handle that and return a 0, not just a comment.
 
> +    totalBytes += sizeof(AlignedAllocSize); // A pathologically high
totalBytes value could result in this overflowing.

I think we need code to handle that and return a 0, not just a comment.

>  void cfree(void* ptr) {
> -#ifndef WTF_CHANGES
> -    MallocHook::InvokeDeleteHook(ptr);
> -#endif
> -  do_free(ptr);
> +  free(ptr);
>  }

Do you really need to change this?

>  
>  #ifndef WTF_CHANGES
> @@ -3324,25 +3430,35 @@
>      return realloc<false>(old_ptr, new_size);
>  }
>  
> +
>  template <bool abortOnFailure>
>  ALWAYS_INLINE
>  #endif
>  void* realloc(void* old_ptr, size_t new_size) {
>    if (old_ptr == NULL) {
> +#if ENABLE(FAST_MALLOC_MATCH_VALIDATION)
> +    void* result = malloc(new_size);
> +#else
>      void* result = do_malloc(new_size);
>  #ifndef WTF_CHANGES
>      MallocHook::InvokeNewHook(result, new_size);
>  #endif
> +#endif
>      return result;
>    }
>    if (new_size == 0) {
> -#ifndef WTF_CHANGES
> -    MallocHook::InvokeDeleteHook(old_ptr);
> -#endif
>      free(old_ptr);
>      return NULL;
>    }

I don't understand what the value is of removing InvokeDeleteHook here.

> +#if !defined(ENABLE_FAST_MALLOC_MATCH_VALIDATION)
> +#define ENABLE_FAST_MALLOC_MATCH_VALIDATION 1
> +#endif

This seems wrong. I think this is something we'd want off in production builds.


> -    class NullNode : public ExpressionNode {
> +    class NullNode : public ExpressionNode, public WTF::FastAllocBase {

We can avoid adding so much separate derivation of every node class from
FastAllocBase by making ParserRefCounted inherit from it.

> Index: WebKit/qt/tests/qwebframe/tst_qwebframe.cpp
> ===================================================================
> --- WebKit/qt/tests/qwebframe/tst_qwebframe.cpp	(revision 38334)
> +++ WebKit/qt/tests/qwebframe/tst_qwebframe.cpp	(working copy)
> @@ -27,6 +27,8 @@
>  #include <qwebhistory.h>
>  #include <QRegExp>
>  #include <QNetworkRequest>
> +
> +#include <wtf/FastAllocBase.h>
>  //TESTED_CLASS=
>  //TESTED_FILES=
>  
> @@ -69,7 +71,7 @@
>  Q_DECLARE_METATYPE(QVariantList)
>  Q_DECLARE_METATYPE(QVariantMap)
>  
> -class MyQObject : public QObject
> +class MyQObject : public QObject, public WTF::FastAllocBase
>  {
>      Q_OBJECT
>  
> @@ -2037,7 +2039,7 @@
>      QCOMPARE(m_view->page()->mainFrame()->toHtml(), html);
>  }
>  
> -class TestNetworkManager : public QNetworkAccessManager
> +class TestNetworkManager : public QNetworkAccessManager, public
WTF::FastAllocBase
>  {
>  public:
>      TestNetworkManager(QObject* parent) : QNetworkAccessManager(parent) {}

Given this is just a test, I think it's OK to just leave these classes alone.

> Index: WebKit/qt/tests/qwebframe/qwebframe.pro
> ===================================================================
> --- WebKit/qt/tests/qwebframe/qwebframe.pro	(revision 38334)
> +++ WebKit/qt/tests/qwebframe/qwebframe.pro	(working copy)
> @@ -4,3 +4,6 @@
>  SOURCES  += tst_qwebframe.cpp
>  QT += testlib network
>  QMAKE_RPATHDIR = $$OUTPUT_DIR/lib $$QMAKE_RPATHDIR
> +
> +LIBS += -L../../../../JavaScriptCore
> +LIBS += -lJavaScriptCore
> Index: WebKit/qt/tests/qwebpage/qwebpage.pro
> ===================================================================
> --- WebKit/qt/tests/qwebpage/qwebpage.pro	(revision 38334)
> +++ WebKit/qt/tests/qwebpage/qwebpage.pro	(working copy)
> @@ -4,3 +4,6 @@
>  SOURCES  += tst_qwebpage.cpp
>  QT += testlib network
>  QMAKE_RPATHDIR = $$OUTPUT_DIR/lib $$QMAKE_RPATHDIR
> +
> +LIBS += -L../../../../JavaScriptCore
> +LIBS += -lJavaScriptCore

Are you sure these are correct? Why wasn't this needed before now? It this a
good/reasonable new requirement?

I can't review all the rest of this. I think it's a mistake to make adoption of
this across the entire project one giant patch. It would be much smarter to
convert a class in JavaScriptCore and another in WebCore and get the kinks out
before adding the code to hundreds of files all at the same time.

In general, I think we should put FastAllocBase into some base classes such as
RefCounted and thereby avoid having to explicitly put it in so many classes
individually, and in various other cases put it into a class as far up the
inheritance tree as possible.

I'm concerned that the patch has modifications to the Qt build system but not
others. Is that because you've only tried this with the Qt build system, or is
there something Qt-specific? I think we should get the build system changes out
of the way first, so a small patch that just adds the dependency and gets the
build right would be good to do first before the giant patch.

review- for now.

Lets come up with a smaller patch that introduces the new header and adopts it
in enough places so we can test it, rather than an "all at once" approach. Then
we can follow up with adopting it elsewhere and then some kind of "check that
it's used everywhere" final step.