Bug 50865 - GlyphPage::fill() is slow on vertical writing (Mac)
Summary: GlyphPage::fill() is slow on vertical writing (Mac)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-11 00:34 PST by Takumi Takano
Modified: 2010-12-13 22:58 PST (History)
6 users (show)

See Also:


Attachments
Propose patch. (6.19 KB, patch)
2010-12-11 00:55 PST, Takumi Takano
mitz: review-
Details | Formatted Diff | Diff
New proposed patch. (7.43 KB, patch)
2010-12-13 16:58 PST, Takumi Takano
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takumi Takano 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.
Comment 1 Takumi Takano 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.
Comment 2 Eric Seidel (no email) 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.
Comment 3 Dave Hyatt 2010-12-13 14:14:19 PST
Comment on attachment 76295 [details]
Propose patch.

r=me
Comment 4 mitz 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).
Comment 5 Takumi Takano 2010-12-13 16:58:27 PST
Created attachment 76469 [details]
New proposed patch.
Comment 6 mitz 2010-12-13 17:00:16 PST
Thanks!
Comment 7 WebKit Commit Bot 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.
Comment 8 WebKit Commit Bot 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.
Comment 9 WebKit Commit Bot 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.
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2010-12-13 22:58:45 PST
All reviewed patches have been landed.  Closing bug.