Bug 101277

Summary: Remove branch from inside RenderObject::view now that renderer() is more expensive
Product: WebKit Reporter: Elliott Sprehn <esprehn>
Component: New BugsAssignee: Elliott Sprehn <esprehn>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 100057    
Attachments:
Description Flags
Patch
none
Patch none

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
DetailsFormatted DiffDiff
Patch (5.65 KB, patch)
2012-11-06 12:46 PST, Elliott Sprehn 		no flags
DetailsFormatted DiffDiff
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.