Bug 26953 – [V8] Do not do unnecessary handles casts and inline couple of methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[anton muhin]

Currently convertToNativeObject accepts v8::Handle<v8::Value>, but can be only used for v8::Handle<v8::Object> (see a cast).  This cast is cheap, but is not free (e.g. it checks if handle is empty).  So this patch lifts casts to callers and makes conversion methods require v8::Handle<v8::Object>

Alongside: inline convertToNativeObject to enable more compiler optimizations.

[anton muhin]

Created attachment 32244 [details]
The patch

[Mads Ager]

>          template <class C>
> -        static C* convertToNativeObject(V8ClassIndex::V8WrapperType type, v8::Handle<v8::Value> object)
> +        static C* convertToNativeObject(V8ClassIndex::V8WrapperType type, v8::Handle<v8::Object> object)
>          {
> -            return static_cast<C*>(convertToNativeObjectImpl(type, object));
> -        }
> +            // Native event listener is per frame, it cannot be handled by this generic function.
> +            ASSERT(type != V8ClassIndex::EVENTLISTENER);
> +            ASSERT(type != V8ClassIndex::EVENTTARGET);
> +
> +            ASSERT(maybeDOMWrapper(object));
> +
> +            ASSERT(
> +            #define MAKE_CASE(TYPE, NAME) (type != V8ClassIndex::TYPE) &&
> +                DOM_NODE_TYPES(MAKE_CASE)
> +            #if ENABLE(SVG)
> +                SVG_NODE_TYPES(MAKE_CASE)
> +            #endif
> +                true
> +                );
> +
> +            return convertDOMWrapperToNative<C>(object);
> +            #undef MAKE_CASE
> +		}

Indentation is broken here (tab?).  

You should undef MAKE_CASE as soon as it is no longer used.  

Do we really need that massive macro-generated assert?

-- Mads

[anton muhin]

Created attachment 32384 [details]
Addressing Mads' comments

Mads, thanks a lot for review!

I hope I addressed all your comments.  I don't know if we need this assert at all, but I just don't want to touch it as my knowledge of bindings is currently limited.  But if you all agree it should go away, I'd be more than happy to remove it.

Sorry, original patch didn't compile cleanly in debug mode (some checks in asserts), so I needed to through in some more changes, but of the similar kind---don't downcast to v8::Value.

And I need a patch for webkit/glue part as well (sending it to you, guys).

[anton muhin]

Created attachment 32385 [details]
Removing forgotten line from ChangeLog

[anton muhin]

ping

[Mads Ager]

The main part of this change looks fine to me, but the type ASSERT is still fishy:

It seems that you are macro expanding into a lot of single-line comments?

Also, with the calculation of the boolean value outside of the ASSERT, it will happen in release mode as well which is probably not what you want.

[anton muhin]

Created attachment 32525 [details]
Do not generate long boolen expression unless ASSERTs enabled

Mads, thanks a lot for another round!

I'd easily remove ASSERT---I do agree it fishy---I just wanted to keep it closer to the original version.  So if you repeat it again, next patch would be w/o ASSERT :)  For now I'm just putting boolean generation under proper condition.

[Mads Ager]

418 #if !ASSERT_DISABLED

should this be #ifndef NDEBUG?

419	            const bool typeIsValid =
420	#define MAKE_CASE(TYPE, NAME) // (type != V8ClassIndex::TYPE) &&

You still have the actual inequality checking commented out so that MAKE_CASE expands to nothing.

[anton muhin]

Created attachment 32567 [details]
Addressing Mads' comments and merging with new head

[anton muhin]

(In reply to comment #8)
> 418 #if !ASSERT_DISABLED
> ut 
> should this be #ifndef NDEBUG?

If I'm reading definition of ASSERT in WebKit/JavaScriptCore/wtf/Assertions.h, it rather depends on ASSERT_DISABLED when on NDEBUG directly.  But of course I could use NDEBUG here.

> 
> 419                const bool typeIsValid =
> 420    #define MAKE_CASE(TYPE, NAME) // (type != V8ClassIndex::TYPE) &&
> 
> You still have the actual inequality checking commented out so that MAKE_CASE
> expands to nothing.

My bad, sorry, hopefully fixed and merged with new head.

[Mads Ager]

This looks good to me.

Over to Dimitri for the official review.  Dimitri, would you use !ASSERT_DISABLED or  #ifndef NDEBUG to only compute the boolean value if the ASSERT generates code?

[Dimitri Glazkov (Google)]

Comment on attachment 32567 [details]
Addressing Mads' comments and merging with new head

Definitely #ifndef NDEBUG. Looks fine otherwise.

[Dimitri Glazkov (Google)]

Comment on attachment 32567 [details]
Addressing Mads' comments and merging with new head

I'll clear review flag until you have a newer patch.

[anton muhin]

Created attachment 32659 [details]
Switching to NDEBUG

Thanks a lot for another round, guys!

[Dimitri Glazkov (Google)]

Comment on attachment 32659 [details]
Switching to NDEBUG

r=me.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/45859.