Bug 26953 - [V8] Do not do unnecessary handles casts and inline couple of methods
Summary: [V8] Do not do unnecessary handles casts and inline couple of methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: anton muhin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-03 10:44 PDT by anton muhin
Modified: 2009-07-14 09:17 PDT (History)
3 users (show)

See Also:


Attachments
The patch (14.49 KB, patch)
2009-07-03 12:10 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Addressing Mads' comments (15.07 KB, patch)
2009-07-07 10:35 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Removing forgotten line from ChangeLog (15.02 KB, patch)
2009-07-07 10:37 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Do not generate long boolen expression unless ASSERTs enabled (15.05 KB, patch)
2009-07-09 12:16 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Addressing Mads' comments and merging with new head (14.14 KB, patch)
2009-07-10 11:43 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Switching to NDEBUG (14.36 KB, patch)
2009-07-13 06:58 PDT, anton muhin
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2009-07-03 10:44:16 PDT
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.
Comment 1 anton muhin 2009-07-03 12:10:33 PDT
Created attachment 32244 [details]
The patch
Comment 2 Mads Ager 2009-07-07 05:36:11 PDT
>          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
Comment 3 anton muhin 2009-07-07 10:35:33 PDT
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).
Comment 4 anton muhin 2009-07-07 10:37:03 PDT
Created attachment 32385 [details]
Removing forgotten line from ChangeLog
Comment 5 anton muhin 2009-07-09 06:25:20 PDT
ping
Comment 6 Mads Ager 2009-07-09 07:37:18 PDT
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.
Comment 7 anton muhin 2009-07-09 12:16:03 PDT
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.
Comment 8 Mads Ager 2009-07-09 13:21:14 PDT
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.
Comment 9 anton muhin 2009-07-10 11:43:28 PDT
Created attachment 32567 [details]
Addressing Mads' comments and merging with new head
Comment 10 anton muhin 2009-07-10 11:46:12 PDT
(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.
Comment 11 Mads Ager 2009-07-10 11:58:38 PDT
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?
Comment 12 Dimitri Glazkov (Google) 2009-07-10 13:52:06 PDT
Comment on attachment 32567 [details]
Addressing Mads' comments and merging with new head

Definitely #ifndef NDEBUG. Looks fine otherwise.
Comment 13 Dimitri Glazkov (Google) 2009-07-10 13:52:36 PDT
Comment on attachment 32567 [details]
Addressing Mads' comments and merging with new head

I'll clear review flag until you have a newer patch.
Comment 14 anton muhin 2009-07-13 06:58:39 PDT
Created attachment 32659 [details]
Switching to NDEBUG

Thanks a lot for another round, guys!
Comment 15 Dimitri Glazkov (Google) 2009-07-13 09:21:33 PDT
Comment on attachment 32659 [details]
Switching to NDEBUG

r=me.
Comment 16 Dimitri Glazkov (Google) 2009-07-14 09:17:15 PDT
Landed as http://trac.webkit.org/changeset/45859.