WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
101277
Remove branch from inside RenderObject::view now that renderer() is more expensive
https://bugs.webkit.org/show_bug.cgi?id=101277
Summary
Remove branch from inside RenderObject::view now that renderer() is more expe...
Elliott Sprehn
Reported
2012-11-05 16:43:07 PST
Remove branch from inside RenderObject::view now that renderer() is more expensive
Attachments
Patch
(4.92 KB, patch)
2012-11-05 16:48 PST
,
Elliott Sprehn
no flags
Details
Formatted Diff
Diff
Patch
(5.65 KB, patch)
2012-11-06 12:46 PST
,
Elliott Sprehn
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Elliott Sprehn
Comment 1
2012-11-05 16:48:22 PST
Created
attachment 172438
[details]
Patch
Eric Seidel (no email)
Comment 2
2012-11-05 16:54:09 PST
Comment on
attachment 172438
[details]
Patch LGTM. Do we have numbers for this perf change?
Elliott Sprehn
Comment 3
2012-11-05 16:58:46 PST
(In reply to
comment #2
)
> (From update of
attachment 172438
[details]
) > LGTM. Do we have numbers for this perf change?
Nope, but I can run Parser/html5-full-render. I don't know what other thing to run, have any ideas? I'm also looking into how to run the page cycler.
Eric Seidel (no email)
Comment 4
2012-11-05 17:00:01 PST
It might show up in html5-full-render. Regardless of the perf impact, I think this is a good change and worth the space trade-off (for code-clarity if nothing else).
Eric Seidel (no email)
Comment 5
2012-11-05 17:00:50 PST
Comment on
attachment 172438
[details]
Patch Are there places which call m_renderer = 0 instead of setRenderer that we need to worry about?
Elliott Sprehn
Comment 6
2012-11-05 17:03:55 PST
(In reply to
comment #5
)
> (From update of
attachment 172438
[details]
) > Are there places which call m_renderer = 0 instead of setRenderer that we need to worry about?
Nope, m_renderer is in Node.h and is private, and I made sure Node always goes through setRenderer(0) in the previous patch.
Build Bot
Comment 7
2012-11-05 18:45:13 PST
Comment on
attachment 172438
[details]
Patch
Attachment 172438
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/14721887
Elliott Sprehn
Comment 8
2012-11-06 11:44:51 PST
(In reply to
comment #4
)
> It might show up in html5-full-render. > > Regardless of the perf impact, I think this is a good change and worth the space trade-off (for code-clarity if nothing else).
This looks like about a 1% improvement on Parser/html5-full-render.html
r133501
baseline: 4713.22 ± 0.59% 1.01% Worse 4655.67 ± 0.51% 4666.32 ± 0.47% With this patch: 4608.43 ± 0.68% 1.24% Better 4643.20 ± 0.28% 0.50% Better 4632.21 ± 0.45% 0.73% Better
Eric Seidel (no email)
Comment 9
2012-11-06 11:48:11 PST
Looks like you need to clean up the exported symbols list too: "__ZNK7WebCore8Document10renderViewEv", referenced from: -exported_symbol[s_list] command line option
Elliott Sprehn
Comment 10
2012-11-06 12:46:09 PST
Created
attachment 172632
[details]
Patch Fix exports
WebKit Review Bot
Comment 11
2012-11-06 14:12:41 PST
Comment on
attachment 172632
[details]
Patch Clearing flags on attachment: 172632 Committed
r133671
: <
http://trac.webkit.org/changeset/133671
>
WebKit Review Bot
Comment 12
2012-11-06 14:12:44 PST
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.
Top of Page
Format For Printing
XML
Clone This Bug