RESOLVED FIXED 98567
[V8] toV8(Node*, ...) does more work than needed (6% faster on dom-traverse) 
https://bugs.webkit.org/show_bug.cgi?id=98567
Summary [V8] toV8(Node*, ...) does more work than needed (6% faster on dom-traverse)
Adam Barth
Reported 2012-10-05 15:42:03 PDT
[V8] toV8(Node*, ...) does more work than needed
Attachments
work in progress (3.29 KB, patch)
2012-10-05 15:42 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
dom-traverse results (6% better; first 3 runs are with patch, last 3 are without) (21.62 KB, text/html)
2012-10-05 15:52 PDT, Adam Barth 		no flags
Details
Patch (4.10 KB, patch)
2012-10-05 16:08 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (4.13 KB, patch)
2012-10-05 16:09 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch for landing (4.25 KB, patch)
2012-10-05 18:34 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-10-05 15:42:21 PDT
Created attachment 167399 [details] work in progress
Adam Barth
Comment 2 2012-10-05 15:52:55 PDT
Created attachment 167404 [details] dom-traverse results (6% better; first 3 runs are with patch, last 3 are without)
Adam Barth
Comment 3 2012-10-05 16:08:38 PDT
Created attachment 167405 [details] Patch
Adam Barth
Comment 4 2012-10-05 16:09:45 PDT
Created attachment 167406 [details] Patch
Kentaro Hara
Comment 5 2012-10-05 17:25:37 PDT
Comment on attachment 167406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167406&action=review Awesome! > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:534 > + if (holder->wrapper() && *holder->wrapper() == holderWrapper && node->wrapper()) This looks a bit hacky. Shall we add a comment on this? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:535 > + return *node->wrapper(); Maybe you can now remove the logic to check node->wrapper() from V8DOMWrapper::getCachedWrapper()? (You can do it in a follow-up patch.)
Adam Barth
Comment 6 2012-10-05 18:13:21 PDT
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:534 > > + if (holder->wrapper() && *holder->wrapper() == holderWrapper && node->wrapper()) > > This looks a bit hacky. Shall we add a comment on this? Add a comment is a good idea. It's a subtle trick. > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:535 > > + return *node->wrapper(); > > Maybe you can now remove the logic to check node->wrapper() from V8DOMWrapper::getCachedWrapper()? (You can do it in a follow-up patch.) We can once we move all callers over to toV8Fast.
Adam Barth
Comment 7 2012-10-05 18:34:44 PDT
Created attachment 167433 [details] Patch for landing
WebKit Review Bot
Comment 8 2012-10-05 19:08:42 PDT
Comment on attachment 167433 [details] Patch for landing Clearing flags on attachment: 167433 Committed r130574: <http://trac.webkit.org/changeset/130574>
WebKit Review Bot
Comment 9 2012-10-05 19:08:45 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.