Bug 15148

Summary: Poor performance on crazy DOM raytracer
Product: WebKit Reporter: David Smith <catfish.man>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer
Priority: P2 Keywords: InRadar
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
URL: http://nontroppo.org/timer/progressive_raytracer.html
Attachments:
Description Flags
Speeds up the "full render" from >400 seconds to ~45 seconds for me
none
Fix my email address, and remove a stray formatting change
mjs: review-
Addresses review comments mjs: review+

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