Bug 98567

Summary:

[V8] toV8(Node*, ...) does more work than needed (6% faster on dom-traverse)

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

haraken, japhet, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
work in progress	none
dom-traverse results (6% better; first 3 runs are with patch, last 3 are without)	none
Patch	none
Patch	none
Patch for landing	none

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	Details Formatted Diff Diff
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	Details Formatted Diff Diff
Patch (4.13 KB, patch) 2012-10-05 16:09 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch for landing (4.25 KB, patch) 2012-10-05 18:34 PDT, Adam Barth	no flags	Details Formatted Diff Diff
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.

Adam Barth

Comment 10 2012-10-07 23:52:00 PDT

Pretty pictures: http://webkit-perf.appspot.com/graph.html#tests=[[40020,2001,2389050],[40020,2001,3001],[40020,2001,173262]]&sel=1349364561286.2532,1349668601792.5823,1351.3513513513512,1891.8918918918919&displayrange=7&datatype=running

Note You need to log in before you can comment on or make changes to this bug.