Bug 42998

Summary: Change Noncopyable and FastAllocBase into macros instead of base classes
Product: WebKit Reporter: Darin Adler <darin>
Component: Web Template FrameworkAssignee: Zoltan Horvath <zoltan>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, aroben, darin, luiz, mrowe, ossy, simon.fraser, skyul, tonikitoo, yong.li.webkit, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 46589    
Bug Blocks: 47887, 49897    
Attachments:
Description Flags
proposed patch for FastAllocBase step 1
none
proposed patch for FastAllocBase step 1
none
updated proposed patch
darin: review-
updated proposed patch darin: review+

Description Darin Adler 2010-07-26 13:57:25 PDT
Anders recently pointed out that the problem of inheriting from base classes with no members, and having objects get larger because of them, were going to keep re-occuring as long as we have Noncopyable and FastAllocBase. He suggested instead we do these as macros. I think that’s probably the way we should go.
Comment 1 Zoltan Horvath 2010-10-08 05:14:11 PDT
Created attachment 70226 [details]
proposed patch for FastAllocBase step 1
Comment 2 Kenneth Rohde Christiansen 2010-10-08 05:29:05 PDT
Comment on attachment 70226 [details]
proposed patch for FastAllocBase step 1

View in context: https://bugs.webkit.org/attachment.cgi?id=70226&action=review

> JavaScriptCore/ChangeLog:6
> +
> +        Change FastAllocBase implementation into a macro
> +        https://bugs.webkit.org/show_bug.cgi?id=42998

Would be nice with some reasoning behind this change.
Comment 3 Zoltan Horvath 2010-10-08 05:52:29 PDT
Created attachment 70233 [details]
proposed patch for FastAllocBase step 1
Comment 4 Csaba Osztrogonác 2010-10-08 08:50:02 PDT
Comment on attachment 70233 [details]
proposed patch for FastAllocBase step 1

Are you going to get rid all inheritance, 
and change it to a construct like this?

class MyClass
{
    WTF_MAKE_FASTALLOCATED
    ....
};

And when it is finished, you simple remove class FastAllocBase;

Am I correct? This idea LGTM.
Comment 5 Zoltan Horvath 2010-10-08 08:56:29 PDT
(In reply to comment #4)
> (From update of attachment 70233 [details])
> Are you going to get rid all inheritance, 
> and change it to a construct like this?

Yes, exactly.

> class MyClass
> {
>     WTF_MAKE_FASTALLOCATED
>     ....
> };
> 
> And when it is finished, you simple remove class FastAllocBase;
> 
> Am I correct? This idea LGTM.

Yes, you are correct.
Comment 6 Darin Adler 2010-10-08 09:45:36 PDT
We should use underscores between words. Since “fast allocated” is not a single word, we will want an underscore between “fast” and “allocated”.
Comment 7 Zoltan Horvath 2010-10-08 09:49:38 PDT
Created attachment 70264 [details]
updated proposed patch

Done.
Comment 8 Darin Adler 2010-10-08 10:04:02 PDT
Comment on attachment 70233 [details]
proposed patch for FastAllocBase step 1

View in context: https://bugs.webkit.org/attachment.cgi?id=70233&action=review

Please fix the namespace issue at least.

> JavaScriptCore/wtf/FastAllocBase.h:89
> +#ifndef WTF_MAKE_FASTALOCATED
> +#define WTF_MAKE_FASTALLOCATED \

I see no reason for the #ifndef. Lets leave that out.

This should be WTF_MAKE_FAST_ALLOCATED since “fastallocated” is not a word.

> JavaScriptCore/wtf/FastAllocBase.h:97
> +        fastMallocMatchValidateMalloc(p, Internal::AllocTypeClassNew); \

This should be ::WTF::fastMallocMatchValidMalloc and ::WTF::Internal::AllocTypeClassNew. The code worked before because it was inside a class in the WTF namespace, but the macro can be invoked on classes outside the WTF namespace. The best syntax to use is to be completely independent of namespace, so start with ::WTF::, which will work even if there is another symbol named WTF.

We could do the same for the invocation of fastMalloc, although it is less important to do that since that is imported into the global namespace. Still, ::WTF::fastMalloc would be slightly more immune to unusual situations that might arise.

> JavaScriptCore/wtf/FastAllocBase.h:123
> +    WTF_MAKE_FASTALLOCATED  

I am not certain, but I believe some compilers will have trouble with "private:" at the end of a class with no members defined after it. I saw a patch at one point removing that idiom elsewhere in the project. Maybe I’m wrong, though.
Comment 9 Darin Adler 2010-10-08 10:04:23 PDT
Comment on attachment 70264 [details]
updated proposed patch

Thanks for fixing the macro name. Please also fix the namespace issue I mentioned.
Comment 10 Anders Carlsson 2010-10-08 10:13:06 PDT
Currently, the Noncopyable base class also implies that you inherit from FastAllocBase.

Should we add a new WTF_MAKE_NONCOPYABLE_AND_FAST_ALLOCATED macro that will do this?
Comment 11 Zoltan Horvath 2010-10-08 10:25:30 PDT
Created attachment 70267 [details]
updated proposed patch
Comment 12 Darin Adler 2010-10-08 10:26:31 PDT
(In reply to comment #10)
> Currently, the Noncopyable base class also implies that you inherit from FastAllocBase.
> 
> Should we add a new WTF_MAKE_NONCOPYABLE_AND_FAST_ALLOCATED macro that will do this?

I like that idea.
Comment 13 Darin Adler 2010-10-08 10:27:45 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > Currently, the Noncopyable base class also implies that you inherit from FastAllocBase.
> > 
> > Should we add a new WTF_MAKE_NONCOPYABLE_AND_FAST_ALLOCATED macro that will do this?
> 
> I like that idea.

But it’s not all that much better from invoking both macros on two subsequent lines, so it’s probably unneeded.

It would be nice to come up with a way to avoid redundant uses of these. If there was some way to make WTF_MAKE_NONCOPYABLE when already noncopyable or WTF_MAKE_FAST_ALLOCATED when already fast-allocated give an error.
Comment 14 Zoltan Horvath 2010-10-08 11:58:32 PDT
After this patch will be landed, how should I do the changings? 

- Make one patch which get rid of all FastAllocBase inheriting and add the using of this macro.
or
- Do it in patch/directory basis?

Btw, I'll make a static analysis to see how many new classes are in the system which need to be fast-allocated.
Comment 15 Zoltan Horvath 2010-10-08 11:59:32 PDT
(In reply to comment #13)
> 
> But it’s not all that much better from invoking both macros on two subsequent lines, so it’s probably unneeded.
> 
> It would be nice to come up with a way to avoid redundant uses of these. If there was some way to make WTF_MAKE_NONCOPYABLE when already noncopyable or WTF_MAKE_FAST_ALLOCATED when already fast-allocated give an error.

On next week, I'm going to investigate this.
Comment 16 Darin Adler 2010-10-08 14:45:41 PDT
(In reply to comment #14)
> - Make one patch which get rid of all FastAllocBase inheriting and add the using of this macro.
> or
> - Do it in patch/directory basis?

I think it’s fine to do it all in one giant patch.
Comment 17 Zoltan Horvath 2010-10-18 00:28:37 PDT
Landed in r69943.
http://trac.webkit.org/changeset/69943