Bug 50865

Summary: GlyphPage::fill() is slow on vertical writing (Mac)
Product: WebKit Reporter: Takumi Takano <takano>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, commit-queue, eric, hyatt, mitz, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Propose patch.
mitz: review-
New proposed patch. none

Takumi Takano
Reported 2010-12-11 00:34:49 PST
On vertical writing, GlyphPage::fill() uses CoreText instead of CG to map characters into glyphs. The CoreText path is obviously complex and slow. When I sampled with natural Japanese text rendering, I saw it is almost 2.5 times slower than CG path (around 100 samples vs around 270 samples.) We can improve the performance by using CG path on ideograph-only page even in vertical writing mode, as we know ideographs don't have a vertical variant glyph.
Attachments
Propose patch. (6.19 KB, patch)
2010-12-11 00:55 PST, Takumi Takano 		mitz: review-
DetailsFormatted DiffDiff
New proposed patch. (7.43 KB, patch)
2010-12-13 16:58 PST, Takumi Takano 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Takumi Takano
Comment 1 2010-12-11 00:55:46 PST
Created attachment 76295 [details] Propose patch. With this change, the count of CoreText path execution reduced to 1/7 in a test with natural Japanese text. Also the sample count becomes almost as same level as a case that forces to only use CG path.
Eric Seidel (no email)
Comment 2 2010-12-12 01:44:35 PST
Ideally we'd write a benchmark for this. WebCore/benchmarks would be a place to put such a thing. I'm not sure the easiest way to test drawing speed.
Dave Hyatt
Comment 3 2010-12-13 14:14:19 PST
Comment on attachment 76295 [details] Propose patch. r=me
mitz
Comment 4 2010-12-13 14:19:50 PST
Comment on attachment 76295 [details] Propose patch. r-, actually. This is correct but I’d rather you didn’t add an anonymous boolean parameter to isCJKIdeograph(). Instead, please rename it to isCJKIdeographOrSymbol(), and factor out the isCJKIdeograph() part (and update call sites).
Takumi Takano
Comment 5 2010-12-13 16:58:27 PST
Created attachment 76469 [details] New proposed patch.
mitz
Comment 6 2010-12-13 17:00:16 PST
Thanks!
WebKit Commit Bot
Comment 7 2010-12-13 19:40:15 PST
The commit-queue encountered the following flaky tests while processing attachment 76469 [details]: animations/suspend-resume-animation-events.html bug 51002 (author: cmarrin@apple.com) fast/workers/storage/use-same-database-in-page-and-workers.html bug 51003 (author: dumi@chromium.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 8 2010-12-13 21:16:58 PST
The commit-queue encountered the following flaky tests while processing attachment 76469 [details]: http/tests/misc/uncacheable-script-repeated.html bug 51009 (author: koivisto@iki.fi) animations/suspend-resume-animation.html bug 51010 (author: cmarrin@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 9 2010-12-13 22:56:06 PST
The commit-queue encountered the following flaky tests while processing attachment 76469 [details]: inspector/timeline-recalculate-styles.html bug 51014 (authors: pfeldman@chromium.org and yurys@chromium.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 10 2010-12-13 22:58:38 PST
Comment on attachment 76469 [details] New proposed patch. Clearing flags on attachment: 76469 Committed r74005: <http://trac.webkit.org/changeset/74005>
WebKit Commit Bot
Comment 11 2010-12-13 22:58:45 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.