Bug 42998 – Change Noncopyable and FastAllocBase into macros instead of base classes

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

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.

[Zoltan Horvath]

Created attachment 70226 [details]
proposed patch for FastAllocBase step 1

[Kenneth Rohde Christiansen]

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.

[Zoltan Horvath]

Created attachment 70233 [details]
proposed patch for FastAllocBase step 1

[Csaba Osztrogonác]

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.

[Zoltan Horvath]

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

[Darin Adler]

We should use underscores between words. Since “fast allocated” is not a single word, we will want an underscore between “fast” and “allocated”.

[Zoltan Horvath]

Created attachment 70264 [details]
updated proposed patch

Done.

[Darin Adler]

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.

[Darin Adler]

Comment on attachment 70264 [details]
updated proposed patch

Thanks for fixing the macro name. Please also fix the namespace issue I mentioned.

[Anders Carlsson]

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?

[Zoltan Horvath]

Created attachment 70267 [details]
updated proposed patch

[Darin Adler]

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

[Darin Adler]

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

[Zoltan Horvath]

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.

[Zoltan Horvath]

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

[Darin Adler]

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

[Zoltan Horvath]

Landed in r69943.
http://trac.webkit.org/changeset/69943