| Summary: | Make GlyphBuffers required in the fast text codepath | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | changseok, darin, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, jonlee, koivisto, kondapallykalyan, pdr, sabouhallawa, schenney, sergio, simon.fraser, thorton, webkit-bug-importer, zalan | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Bug Depends on: | |||||||||||||||
| Bug Blocks: | 206208 | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Myles C. Maxfield
2020-08-01 00:19:55 PDT
Created attachment 405771 [details]
Patch
Comment on attachment 405771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405771&action=review > Source/WebCore/platform/graphics/WidthIterator.cpp:84 > if (m_run.length() <= 1 || !(m_enableKerning || m_requiresShaping)) if (m_run.length() <= 1) Created attachment 405789 [details]
Patch
Created attachment 405790 [details]
Patch
Is this going to be a perf regression? Created attachment 405977 [details]
Patch
Created attachment 405981 [details]
Patch
(In reply to Simon Fraser (smfr) from comment #5) > Is this going to be a perf regression? I'm still testing. Will comment here when I know more. % compare-results -a plt-results-Vanilla -b plt-results-RequiredGlyphBuffers a mean = 740.41311 b mean = 743.70648 pValue = 0.7209862553 (Smaller means are better.) 1.004 times worse Results ARE NOT significant The design test of MotionMark shows this to be a progression With patch: 156pt Without patch: 153pt Comment on attachment 405981 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405981&action=review > Source/WebCore/platform/graphics/FontCascade.cpp:450 > + // This is needed only to match the result of the slow path. Same glyph widths but different floating point arithmentics can > + // produce different run width. Existing comment just moving but: - Would look nicer if line break was at sentence break. - "arithmentics" > LayoutTests/ChangeLog:8 > + * platform/win/fast/events/selectstart-by-drag-expected.txt: Test progressed on Windows. Do you know why? Comment on attachment 405981 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405981&action=review >> LayoutTests/ChangeLog:8 >> + * platform/win/fast/events/selectstart-by-drag-expected.txt: Test progressed on Windows. > > Do you know why? I don't have a Windows development environment, so I can't say for sure, but requiring GlyphBuffers is a slight behavior change. When GlyphBuffers aren't specified, shaping doesn't occur at all, so making them mandatory performs shaping when shaping didn't always used to occur. (This is why I made sure to check performance numbers before making this patch as r?.) Because this is a progression, and aligns Windows with Mac and iOS, I didn't look too hard at it. Committed r265413: <https://trac.webkit.org/changeset/265413> |