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

[bugzilla-daemon at webkit.org]

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





------- Comment #35 from Horvath.Zoltan.6 at stud.u-szeged.hu  2008-11-24 07:32 PDT -------
>> -    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.

Okay, I'll do this.

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

new call: JavaScriptCore/profiler/CallIdentifier.h: line 84

We use a static analyzer tool called Columbus
(http://www.inf.u-szeged.hu/sed/softwarequality) to find out which classes are
instantiated with "new" so we know exactly which classes have to be inherited
from FastAllocBase. (With the help of Columbus we can filter out unneccessary
inheritances and we can handle diamond inheritances properly.)

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

It will be updated in the next patch. I did this patch for r38344.

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

I'll remove redundant headers, thanks for the tip.

>> -    Rep* r = new Rep;
>> +    Rep* r = fastNew<Rep>();
>
>The Rep struct should be changed to inherit from FastAllocBase.
>

If I inherit Rep from FastAllocBase it'll be no longer a POD and it can't be
initalized with a C-style initializer list. (causes compile error)
Should I add new/delete member functions to Rep or use fastNew/fastDelete?

>> -    , arrayTable(new HashTable(JSC::arrayTable))
>> +    , arrayTable(fastNew<HashTable>(JSC::arrayTable))
>
>The HashTable struct should be changed to inherit from FastAllocBase.

I had the same problem with Rep. I'll check it again in the new patch.

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

Okay, I'll modify.

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

new call: MainThreadInvoker: JavaScriptCore/wtf/qt/MainThreadQt.cpp: line 59
Qt's Q_GLOBAL_STATIC macro causes: MainThreadInvoker *x = new
MainThreadInvoker;

>> -class ThreadCondition : Noncopyable {
>> +class ThreadCondition : Noncopyable, public WTF::FastAllocBase {
>
>I can't find any use of this class with "new".

new call: WebCore/storage/DatabaseTask.cpp: line 74

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

Okay. I'll inherit ParserRefCounted. I didn't do this because there are some
classes which inherited from ParserRefCounted but not constructed by new.

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

Ok.

>> Index: WebKit/qt/tests/qwebframe/qwebframe.pro
>> Index: WebKit/qt/tests/qwebpage/qwebpage.pro
>>...
>> +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?

We'll leave tests alone so it won't be needed.

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

Ok. First we'll produce a patch just for JavaScriptCore.

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

Okay, but how far up? There are a lot of classes which are inherited from (for
example) Noncopyable but only a part of them instantiated with "new". And what
can we do when the inheritance is private?

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

We've only tried this under linux-qt port. For the next version we need to
modify the build system just for QtLauncher.


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