Use nscoord variables rather than double-precision gfxFloats for text measurements that are actually integer appunits
Categories
(Core :: Layout: Text and Fonts, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox146 | --- | fixed |
People
(Reporter: jfkthame, Assigned: jfkthame, NeedInfo)
References
Details
(Keywords: perf-alert)
Attachments
(4 files)
It appears that we use gfxFloat for some values that are in fact always integer appUnits. In particular, the gfxFont::Spacing fields are currently defined as gfxFloat, but hold appUnit values; as we create large arrays of Spacing records, reducing the size of this struct (from 16 to 8 bytes) will significantly reduce the memory footprint of such spacing arrays.
The comment here suggests that "we specify them in floats in case very large spacing values are required", but this is pointless: if we have glyph spacing values so large that they overflow a 32-bit integer appUnit value, we'll be encountering overflow or clamping elsewhere as well, and "correct" layout cannot be expected.
Similarly, while gfxFont::Metrics holds values in (floating-point) device pixels, the gfxFont::RunMetrics (aka gfxTextRun::Metrics) struct that is used to return text run measurements to layout works in terms of appUnits; we can convert its fields from gfxFloat to nscoord values, for both clarity and compactness.
|
Assignee | |
Comment 1•3 months ago
|
||
|
||
Updated•3 months ago
|
|
|
Assignee | |
Comment 2•3 months ago
|
||
|
|
Assignee | |
Comment 3•3 months ago
|
||
|
||
Comment 6•3 months ago
|
||
Reverted this because it was causing assertion failures in nsTextFrame.cpp.
- Revert link
- Push with failures
- Failure Log
- Failure line: Assertion failure: aISize >= 0 (unexpected negative hangable advance), at /builds/worker/checkouts/gecko/layout/generic/nsTextFrame.cpp:X
Also, please check these wpt failures.
|
|
Assignee | |
Comment 7•3 months ago
|
||
Looks like the assertion failures here are happening due to huge metrics that end up overflowing the integer textrun advance metric, and turning negative. So to proceed with this, we'll need to be more careful about handling coordinate overflow.
|
|
Assignee | |
Comment 8•3 months ago
|
||
There's no point in doing these calculations with gfxFloat values,
as they're destined to become nscoord in layout, which restricts the
possible range. With huge font sizes/spacing/etc, we can end up with
values that are out of range for nscoord, which will lead to broken
layout (at best) or possibly assertions (e.g. about advances that
appear hugely negative after nscoord overflow).
Doing all the advance computations in nscoord values, with saturating
addition to handle potentially huge values, should result in more stable
and predictable results (although layout will still not really be
meaningful when dimensions exceed what nscoord can handle).
(This resolves the assertions that were seen when I originally tried
to land the patch stack here.)
|
|
||
Updated•3 months ago
|
|
||
Comment 10•3 months ago
|
||
|
||
Comment 11•3 months ago
|
||
Backed out for causing wr failures @text-combine-upright-gen-con-001.html.
|
|
||
Comment 12•3 months ago
|
||
|
||
Comment 13•3 months ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/6daf311db928
https://hg.mozilla.org/mozilla-central/rev/713d9f3a5771
https://hg.mozilla.org/mozilla-central/rev/9a06ba9c63f8
https://hg.mozilla.org/mozilla-central/rev/08b700ae5349
|
||
Comment 14•3 months ago
|
||
(In reply to Pulsebot from comment #9)
Pushed by jkew@mozilla.com:
https://github.com/mozilla-firefox/firefox/commit/8c9d04dcc3a2
https://hg.mozilla.org/integration/autoland/rev/7a1e17d80be9
patch 1 - Use integer nscoord values rather than gfxFloat for
gfxFont::Spacing fields. r=layout-reviewers,TYLin
https://github.com/mozilla-firefox/firefox/commit/995e0ca47eb8
https://hg.mozilla.org/integration/autoland/rev/e602047d7bd5
patch 2 - Convert more text measurement to use integer nscoord values.
r=layout-reviewers,TYLin
https://github.com/mozilla-firefox/firefox/commit/c2168d7fb894
https://hg.mozilla.org/integration/autoland/rev/2765c4fa843a
patch 3 - Convert gfxTextRun text advance computation to nscoord values.
r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/d4927b88663c
https://hg.mozilla.org/integration/autoland/rev/2ffd31537656
patch 4 - Also use integer nscoords for textrun ascent/descent metrics and
bounding box. r=firefox-svg-reviewers,longsonr
Perfherder has detected a browsertime performance change from push 7a25d20f5e811cc75c91b018ed7da883535d969d. As author of one of the patches included in that push, we need your help to address this regression.
If you have any questions or need any help with the investigation, please reach out to a performance sheriff. Alternatively, you can find help on Slack by joining #perf-help, and on Matrix you can find help by joining #perftest.
Regressions:
| Ratio | Test | Platform | Options | Absolute values (old vs new) | Performance Profiles |
|---|---|---|---|---|---|
| 3% | speedometer3 Editor-TipTap/Long/Sync | macosx1500-aarch64-shippable | fission webrender | 24.62 -> 25.31 | Before/After |
| 3% | speedometer3 Editor-TipTap/Long/total | macosx1500-aarch64-shippable | fission webrender | 25.26 -> 25.93 | Before/After |
| 2% | speedometer3 Editor-TipTap/total | macosx1500-aarch64-shippable | fission webrender | 46.61 -> 47.61 | Before/After |
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.
If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.
You can run all of these tests on try with ./mach try perf --alert 47207
The following documentation link provides more information about this command.
|
|
||
Updated•3 months ago
|
|
||
Comment 15•2 months ago
|
||
(In reply to Pulsebot from comment #9)
Pushed by jkew@mozilla.com:
https://github.com/mozilla-firefox/firefox/commit/8c9d04dcc3a2
https://hg.mozilla.org/integration/autoland/rev/7a1e17d80be9
patch 1 - Use integer nscoord values rather than gfxFloat for
gfxFont::Spacing fields. r=layout-reviewers,TYLin
https://github.com/mozilla-firefox/firefox/commit/995e0ca47eb8
https://hg.mozilla.org/integration/autoland/rev/e602047d7bd5
patch 2 - Convert more text measurement to use integer nscoord values.
r=layout-reviewers,TYLin
https://github.com/mozilla-firefox/firefox/commit/c2168d7fb894
https://hg.mozilla.org/integration/autoland/rev/2765c4fa843a
patch 3 - Convert gfxTextRun text advance computation to nscoord values.
r=layout-reviewers,emilio
https://github.com/mozilla-firefox/firefox/commit/d4927b88663c
https://hg.mozilla.org/integration/autoland/rev/2ffd31537656
patch 4 - Also use integer nscoords for textrun ascent/descent metrics and
bounding box. r=firefox-svg-reviewers,longsonr
Perfherder has detected a browsertime performance change from push 2ffd3153765697af9294a75dc6eb938c7b595c7a.
No action is required from the author, this comment is provided for information purposes only.
Regressions:
| Ratio | Test | Platform | Options | Absolute values (old vs new) | Performance Profiles |
|---|---|---|---|---|---|
| 2% | speedometer3 Editor-TipTap/Highlight/Sync | macosx1500-aarch64-shippable | fission webrender | 19.96 -> 20.37 | Before/After |
Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.
If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a performance sheriff to do that for you.
You can run all of these tests on try with ./mach try perf --alert 47284
The following documentation link provides more information about this command.
|
||
Updated•2 months ago
|
Description
•