Bug 43933

Summary: Improved Table Hit Testing and Painting Performance
Product: WebKit Reporter: Fady Samuel <fsamuel>
Component: TablesAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: anicolao, commit-queue, dbates, eric, fsamuel, hyatt, jamesr, kenneth, rjkroege
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Table hit testing graph
none
Benchmark script
none
Patch For Faster Table Hit Testing
hyatt: review-, hyatt: commit-queue-
128*128 table benchmark
none
Revised Patch
none
Revised Patch (Fixes merge conflict) none

Fady Samuel
Reported 2010-08-12 14:22:51 PDT
Created attachment 64260 [details] Table hit testing graph Table hit testing performance in webkit is relatively poor especially when compared to Firefox. It is done currently by visiting every single cell in the table until a cell is found that is interested in the hit. This can perform horribly as the table size gets larger. I have attached a log-log graph showing before and after performance of table hit testing as well as a comparison with firefox.
Attachments
Table hit testing graph (131.35 KB, image/png)
2010-08-12 14:22 PDT, Fady Samuel 		no flags
Details
Benchmark script (2.25 KB, text/html)
2010-08-12 14:24 PDT, Fady Samuel 		no flags
Details
Patch For Faster Table Hit Testing (60.72 KB, patch)
2010-08-12 14:32 PDT, Fady Samuel 		hyatt: review-
hyatt: commit-queue-
DetailsFormatted DiffDiff
128*128 table benchmark (2.25 KB, text/html)
2010-08-12 15:50 PDT, Fady Samuel 		no flags
Details
Revised Patch (106.11 KB, patch)
2010-08-17 11:37 PDT, Fady Samuel 		no flags
DetailsFormatted DiffDiff
Revised Patch (Fixes merge conflict) (106.49 KB, patch)
2010-08-17 12:47 PDT, Fady Samuel 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Fady Samuel
Comment 1 2010-08-12 14:24:25 PDT
Created attachment 64261 [details] Benchmark script
Fady Samuel
Comment 2 2010-08-12 14:32:15 PDT
Created attachment 64263 [details] Patch For Faster Table Hit Testing
Fady Samuel
Comment 3 2010-08-12 14:36:12 PDT
As a real-world demo, this site's performance is SIGNIFICANTLY improved with this patch: http://tekkub.net/
Fady Samuel
Comment 4 2010-08-12 15:50:37 PDT
Created attachment 64277 [details] 128*128 table benchmark
Dave Hyatt
Comment 5 2010-08-16 13:02:53 PDT
Comment on attachment 64263 [details] Patch For Faster Table Hit Testing Minusing. Comments delivered on IRC. The code can incorrectly paint a little too much. Need some precise boundary detection and a simple repaint test etc.
Fady Samuel
Comment 6 2010-08-17 11:37:56 PDT
Created attachment 64612 [details] Revised Patch Fixed as per dhyatt's comments.
Fady Samuel
Comment 7 2010-08-17 12:47:28 PDT
Created attachment 64621 [details] Revised Patch (Fixes merge conflict)
Dave Hyatt
Comment 8 2010-08-17 13:11:09 PDT
Comment on attachment 64621 [details] Revised Patch (Fixes merge conflict) r=me
WebKit Commit Bot
Comment 9 2010-08-17 13:33:16 PDT
Comment on attachment 64621 [details] Revised Patch (Fixes merge conflict) Rejecting patch 64621 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'David Hyatt', u'--force']" exit_code: 255 Last 500 characters of output: ng file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/table/simple_paint.html patching file LayoutTests/platform/chromium-linux/fast/table/simple_paint-expected.checksum literal is only avaliable with the XS version at /Library/Perl/5.8.8/Compress/Zlib.pm line 9 BEGIN failed--compilation aborted at /Library/Perl/5.8.8/Compress/Zlib.pm line 9. Compilation failed in require at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/VCSUtils.pm line 1662. Full output: http://queues.webkit.org/results/3736294
James Robinson
Comment 10 2010-08-17 13:57:36 PDT
Comment on attachment 64621 [details] Revised Patch (Fixes merge conflict) Clearing flags on attachment: 64621 Committed r65539: <http://trac.webkit.org/changeset/65539>
James Robinson
Comment 11 2010-08-17 13:57:43 PDT
All reviewed patches have been landed. Closing bug.
James Robinson
Comment 12 2010-08-17 13:58:37 PDT
Landed by hand. Eric, why did the commit queue barf on this?
Eric Seidel (no email)
Comment 13 2010-08-17 17:19:01 PDT
We thought we had fixed this. I just need to move the commit-queue to SnowLeopard to work around this and the CoreVideo media/compositing crash/timeouts.
Eric Seidel (no email)
Comment 14 2010-08-18 10:52:16 PDT
This appears to be missing Mac results. :(
Fady Samuel
Comment 15 2010-08-18 10:53:00 PDT
(In reply to comment #14) > This appears to be missing Mac results. :( Yup working on it.
Note You need to log in before you can comment on or make changes to this bug.