Bug 26882 - Speed up creation of V8 wrappers for DOM nodes
Summary: Speed up creation of V8 wrappers for DOM nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: anton muhin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-01 07:40 PDT by anton muhin
Modified: 2009-07-07 09:21 PDT (History)
4 users (show)

See Also:


Attachments
patch to speed up creation of V8 wrappers (10.06 KB, patch)
2009-07-01 07:54 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Reworked patch (with fixed indentation) (9.95 KB, patch)
2009-07-02 05:11 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Next iteration (9.98 KB, patch)
2009-07-02 05:23 PDT, anton muhin
fishd: review-
Details | Formatted Diff | Diff
Addressing Dmitry's comments (9.43 KB, patch)
2009-07-02 09:46 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Uploaded updated version (9.44 KB, patch)
2009-07-07 05:38 PDT, anton muhin
fishd: 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-01 07:40:46 PDT
Speed up creation of V8 wrappers for DOM nodes.
Comment 1 anton muhin 2009-07-01 07:54:44 PDT
Created attachment 32122 [details]
patch to speed up creation of V8 wrappers
Comment 2 Dimitri Glazkov (Google) 2009-07-01 10:23:31 PDT
Anton, can you re-upload as patch (check the checkbox)? As-is, this is not formally reviewable -- bugzilla sees it as an attachment, not a patch.

Also, I downloaded the patch and it looks like you still have plenty of style issues. Please look at the spacing (4 spaces, not 2), positioning of brackets, naming convention, etc.
Comment 3 anton muhin 2009-07-01 12:07:40 PDT
(In reply to comment #2)
> Anton, can you re-upload as patch (check the checkbox)? As-is, this is not
> formally reviewable -- bugzilla sees it as an attachment, not a patch.
> 
> Also, I downloaded the patch and it looks like you still have plenty of style
> issues. Please look at the spacing (4 spaces, not 2), positioning of brackets,
> naming convention, etc.
> 

Dmitry, dreadful sorry, but I cannot find out a checkbox which would allow me to upload a patch proper, what's the text close to it?  I searched for 'patch', but to no avail.  May I need some bits for that?

Regarding style.  I rechecked the diff and didn't spot misplaced {} and wrong names.  I fixed many lines with invalid indentation.  If you can point out me some place which violates code style, it'd make it easier to spot more of them to me.
Comment 4 Dimitri Glazkov (Google) 2009-07-01 15:21:40 PDT
No problem -- glad to help. When you click to create new attachment, There is this option:

if the attachment is a patch, check the box below.
[ ] patch

Let's look at the style when you re-upload :)

BTW, you're sitting right next to a WebKit committer -- I highly recommend bugging him when in doubt ;)
Comment 5 anton muhin 2009-07-02 05:11:11 PDT
Created attachment 32177 [details]
Reworked patch (with fixed indentation)
Comment 6 anton muhin 2009-07-02 05:12:53 PDT
(In reply to comment #4)
> No problem -- glad to help. When you click to create new attachment, There is
> this option:
> 
> if the attachment is a patch, check the box below.
> [ ] patch
> 
> Let's look at the style when you re-upload :)
> 
> BTW, you're sitting right next to a WebKit committer -- I highly recommend
> bugging him when in doubt ;)
> 

:) Thanks a lot, Dmitry.  He just left by that time.

yours,
anton.
Comment 7 anton muhin 2009-07-02 05:23:10 PDT
Created attachment 32180 [details]
Next iteration

Thanks to Mads who spotted missing indentation of array.
Comment 8 Darin Fisher (:fishd, Google) 2009-07-02 09:07:10 PDT
Comment on attachment 32180 [details]
Next iteration

> Index: ChangeLog
...
> +2009-07-01  anton muhin  <antonm@chromium.org>

nit: your name should be Mixed Case


> +        Reviewed by NOBODY (OOPS!).
> +
> +				Speed up creation of V8 wrappers for DOM nodes.
> +
> +				https://bugs.webkit.org/show_bug.cgi?id=26882

nit: please do not put tabs in the ChangeLog


> +
> +        This patch doesn't require new tests as it a set of refactorings
> +				to speed up wrapper creation.
> +
> +        * ../../../bindings/v8/V8Proxy.cpp:
> +        * ../../../bindings/v8/V8Proxy.h:
> +

nit: these paths should be relative to the WebCore directory.


> Index: bindings/v8/V8Proxy.h
...
> +        static v8::Local<v8::Object> instantiateV8Object(V8ClassIndex::V8WrapperType descType, V8ClassIndex::V8WrapperType cptrType, void* impl)
> +        {
> +          return instantiateV8Object(NULL, descType, cptrType, impl);

nit: indent by 4 spaces
Comment 9 anton muhin 2009-07-02 09:46:25 PDT
Created attachment 32184 [details]
Addressing Dmitry's comments

Dmitry!

Thanks a lot for review.  All done (hopefully)
Comment 10 anton muhin 2009-07-03 01:18:46 PDT
Darin, sorry, I already answered your comments, but thought it was a review by Dmitry, dreadful sorry.  May you have another pass?

tia and yours,
anton.

(In reply to comment #8)
> (From update of attachment 32180 [details])
> > Index: ChangeLog
> ...
> > +2009-07-01  anton muhin  <antonm@chromium.org>
> 
> nit: your name should be Mixed Case
> 
> 
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +				Speed up creation of V8 wrappers for DOM nodes.
> > +
> > +				https://bugs.webkit.org/show_bug.cgi?id=26882
> 
> nit: please do not put tabs in the ChangeLog
> 
> 
> > +
> > +        This patch doesn't require new tests as it a set of refactorings
> > +				to speed up wrapper creation.
> > +
> > +        * ../../../bindings/v8/V8Proxy.cpp:
> > +        * ../../../bindings/v8/V8Proxy.h:
> > +
> 
> nit: these paths should be relative to the WebCore directory.
> 
> 
> > Index: bindings/v8/V8Proxy.h
> ...
> > +        static v8::Local<v8::Object> instantiateV8Object(V8ClassIndex::V8WrapperType descType, V8ClassIndex::V8WrapperType cptrType, void* impl)
> > +        {
> > +          return instantiateV8Object(NULL, descType, cptrType, impl);
> 
> nit: indent by 4 spaces
Comment 11 anton muhin 2009-07-07 05:38:39 PDT
Created attachment 32373 [details]
Uploaded updated version
Comment 12 Darin Fisher (:fishd, Google) 2009-07-07 09:15:15 PDT
Comment on attachment 32373 [details]
Uploaded updated version

>Index: bindings/v8/V8Proxy.cpp

>+static const V8ClassIndex::V8WrapperType mapping[] = {
>+    V8ClassIndex::INVALID_CLASS_INDEX,    // NONE
>+    V8ClassIndex::INVALID_CLASS_INDEX,    // ELEMENT_NODE needs special treatment
>+    V8ClassIndex::ATTR,                   // ATTRIBUTE_NODE
>+    V8ClassIndex::TEXT,                   // TEXT_NODE
>+    V8ClassIndex::CDATASECTION,           // CDATA_SECTION_NODE
>+    V8ClassIndex::ENTITYREFERENCE,        // ENTITY_REFERENCE_NODE
>+    V8ClassIndex::ENTITY,                 // ENTITY_NODE
>+    V8ClassIndex::PROCESSINGINSTRUCTION,  // PROCESSING_INSTRUCTION_NODE
>+    V8ClassIndex::COMMENT,                // COMMENT_NODE
>+    V8ClassIndex::INVALID_CLASS_INDEX,    // DOCUMENT_NODE needs special treatment
>+    V8ClassIndex::DOCUMENTTYPE,           // DOCUMENT_TYPE_NODE
>+    V8ClassIndex::DOCUMENTFRAGMENT,       // DOCUMENT_FRAGMENT_NODE
>+    V8ClassIndex::NOTATION,               // NOTATION_NODE
>+    V8ClassIndex::NODE,                   // XPATH_NAMESPACE_NODE
>+};

This is a little bit fragile.  If someone changes the NodeType
enum, then we'll be in trouble.  I don't have a good suggestion
to avoid this though :(

R=me
Comment 13 Darin Fisher (:fishd, Google) 2009-07-07 09:17:20 PDT
Landed as:  http://trac.webkit.org/changeset/45596
Comment 14 anton muhin 2009-07-07 09:21:11 PDT
(In reply to comment #13)
> Landed as:  http://trac.webkit.org/changeset/45596

(In reply to comment #12)
> (From update of attachment 32373 [details])
> >Index: bindings/v8/V8Proxy.cpp
> 
> >+static const V8ClassIndex::V8WrapperType mapping[] = {
> >+    V8ClassIndex::INVALID_CLASS_INDEX,    // NONE
> >+    V8ClassIndex::INVALID_CLASS_INDEX,    // ELEMENT_NODE needs special treatment
> >+    V8ClassIndex::ATTR,                   // ATTRIBUTE_NODE
> >+    V8ClassIndex::TEXT,                   // TEXT_NODE
> >+    V8ClassIndex::CDATASECTION,           // CDATA_SECTION_NODE
> >+    V8ClassIndex::ENTITYREFERENCE,        // ENTITY_REFERENCE_NODE
> >+    V8ClassIndex::ENTITY,                 // ENTITY_NODE
> >+    V8ClassIndex::PROCESSINGINSTRUCTION,  // PROCESSING_INSTRUCTION_NODE
> >+    V8ClassIndex::COMMENT,                // COMMENT_NODE
> >+    V8ClassIndex::INVALID_CLASS_INDEX,    // DOCUMENT_NODE needs special treatment
> >+    V8ClassIndex::DOCUMENTTYPE,           // DOCUMENT_TYPE_NODE
> >+    V8ClassIndex::DOCUMENTFRAGMENT,       // DOCUMENT_FRAGMENT_NODE
> >+    V8ClassIndex::NOTATION,               // NOTATION_NODE
> >+    V8ClassIndex::NODE,                   // XPATH_NAMESPACE_NODE
> >+};
> 
> This is a little bit fragile.  If someone changes the NodeType
> enum, then we'll be in trouble.  I don't have a good suggestion
> to avoid this though :(
> 
> R=me

There is minimal safety net below (when bounds of array are checked in debug mode)---hopefully that might catch at least some troubles.  Plus I believe in LayoutTests :)

Thanks a lot for review!