Bug 15148 - Poor performance on crazy DOM raytracer
Summary: Poor performance on crazy DOM raytracer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL: http://nontroppo.org/timer/progressiv...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2007-09-06 00:18 PDT by David Smith
Modified: 2007-09-09 23:10 PDT (History)
1 user (show)

See Also:


Attachments
Speeds up the "full render" from >400 seconds to ~45 seconds for me (9.81 KB, patch)
2007-09-06 00:23 PDT, David Smith
no flags Details | Formatted Diff | Diff
Fix my email address, and remove a stray formatting change (9.05 KB, patch)
2007-09-06 00:58 PDT, David Smith
mjs: review-
Details | Formatted Diff | Diff
Addresses review comments (8.19 KB, patch)
2007-09-08 14:47 PDT, David Smith
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Smith 2007-09-06 00:18:15 PDT
WebKit loses to Opera by a very large margin on the linked benchmark.
Comment 1 David Smith 2007-09-06 00:23:57 PDT
Created attachment 16205 [details]
Speeds up the "full render" from >400 seconds to ~45 seconds for me

Thanks to everyone on irc for their suggestions and prodding :) Particularly the suggestion of using ListHashSet from Maciej.
Comment 2 David Smith 2007-09-06 00:58:44 PDT
Created attachment 16206 [details]
Fix my email address, and remove a stray formatting change

Minor tweaks, sorry for the spam.
Comment 3 Maciej Stachowiak 2007-09-08 05:31:57 PDT
This seems like a gratuitous change:

-        for ( ; (r = it.current()); ++it) {
+        for ( ; (r = it.current()); ++it ) {

There's no need to use new here, you can just make the Vector a local variable:

+    Vector<RenderObject*>* deadObjects = new Vector<RenderObject*>();

In fact, it might be worthwhile giving it an inline capacity to handle the common case effeciently. Something like:

Vector<RenderObject*, 16> deadObjects;

r- to address these points.
Comment 4 David Smith 2007-09-08 14:47:39 PDT
Created attachment 16229 [details]
Addresses review comments

>This seems like a gratuitous change:
>
>-        for ( ; (r = it.current()); ++it) {
>+        for ( ; (r = it.current()); ++it ) {

Oops, leftover from a find & replace style fixup. Fixed.

>There's no need to use new here, you can just make the Vector a local variable:
>
>+    Vector<RenderObject*>* deadObjects = new Vector<RenderObject*>();
>
>In fact, it might be worthwhile giving it an inline capacity to handle the
>common case effeciently. Something like:
>
>Vector<RenderObject*, 16> deadObjects;

Fixed.
Comment 5 Maciej Stachowiak 2007-09-09 20:21:30 PDT
Comment on attachment 16229 [details]
Addresses review comments

r=me
Comment 6 Oliver Hunt 2007-09-09 20:29:36 PDT
<rdar://problem/5470512>
Comment 7 David Smith 2007-09-09 23:10:42 PDT
Fixed in r25464