Repository navigation
perf(core): eliminate line-breaking bottleneck on massive unspaced text blocks (#28) - #29
Conversation
vietnguyentuan2019
left a comment
There was a problem hiding this comment.
Thanks for this, the speedup is real! 🙏 I reproduced #28 on main and re-ran the same benchmark on this branch (one <p>, 400px, mode: sync, flutter test):
| chars | CJK main → PR | Latin main → PR |
|---|---|---|
| 8,000 | 2,094 → 158 ms | 423 → 72 ms |
| 16,000 | 4,699 → 68 ms | 1,898 → 53 ms |
I also dumped debugLineFragments() on main and on this branch for the same inputs (300px wide), and checked every line's visible text width against a fresh TextPainter. That turned up one blocker, plus a set of behavior changes that need to be listed rather than described as parity.
Blocker: text after an inline element ends up on a single line
<p><b>Note:</b> lorem ipsum dolor sit amet consectetur … (≈60 words)</p>On this branch, line 1 is a single 377-char fragment that reports width: 221 but is really ~6,000px wide. The rest of the paragraph never wraps. CJK after a <b> does the same (184 chars on one line). On main both wrap normally.
Cause: in _splitTextFragment, when secondPart.length > safeLimit the tail gets a fake size Size(maxWidth + 1.0, …). Here maxWidth is the remaining width of the line where the split happened, not the container width. After <b>Note:</b> that remaining width is small (~220px). On the next, full-width line the tail looks like it fits, so it is placed whole. That's the most common paragraph shape there is (bold lead-in, link, <code> at the start), so it has to be fixed before merge. Using the container width (or double.infinity) for the estimate, or re-measuring when the estimate is used to decide "fits", would avoid it. Please add a regression test that asserts no line fragment's real width exceeds the container, for <p><b>x</b> + long text</p> in both Latin and CJK.
Line overflow in the fallback path
Beside a float (<img style="float:left;width:120px"> + long text), one line is "consectetur " at x=128 with a real width of 176, so it ends at 304px in a 300px box. It looks like the breakIndex++ when a space sits at the break, plus <= in the space binary search. getPositionForOffset rounds to the nearest caret (300/16 = 18.75 → 19), so a space at 19 now lets a 19-glyph word through. main has the same rounding problem for CJK (every full CJK line on main is 304px), and your native path fixes that, but the fallback path should not reintroduce it for Latin.
Behavior changes (mostly good, but please list them)
The parity tests compare the new fast path against the new fallback, not against main. Every sample I tried breaks differently from main, which is why 8 goldens fail. Most of the differences come from real bugs on main that the current goldens captured:
- RTL wrapping: on
mainan Arabic paragraph puts 72 chars (1,152px) on line 1, then lines of one glyph each (rtl_arabic.pngshows it). Fixed here. word-break: break-all: onmainit produces dozens of one-character lines. Fixed here.- CJK lines overflow by a fraction of a glyph on
main(304px in 300px). Fixed in the native path. - Trailing spaces no longer count toward line width. This shifts
text-align: center/right(correct per CSS, since trailing spaces hang), but it changes every wrapped Latin line, so it's a visible change.
I'm fixing these pre-existing bugs myself on release/1.13.0: RTL wrapping, break-all, the nearest-caret overflow, trailing spaces counted in line width, and a word next to a float being split after its first letter. The fixes are small and live in _splitTextFragment / _forceSplitTextFragment, sharing one helper that finds the longest prefix that fits. I've changed this PR's base to release/1.13.0. Please don't rebase yet. I'll ping you here when the fixes are in, and then the rebase should be mostly mechanical. Your rewritten split code can call the same helper.
After that rebase, the bar for merging is: this PR should not change layout compared to release/1.13.0. That means identical debugLineFragments() output on the same inputs and no golden diffs. If something still differs on purpose (for example, ICU breaking vs KinsokuProcessor in the native path), list it in the PR description with the reason. The text-overflow: ellipsis change in _tokenizeBlock (the new ||, which now always emits a block-start fragment) needs a reason too. Goldens only match on the CI runner, so please don't try to update them locally.
Smaller notes
- The native path's
isPreformattedincludesnowrapbut notbreak-spaces, and_getCandidatePainteronly checkspre/pre-wrap. Please use one helper for both. HyperRenderDebugHooks.onTextPainterLayout/onLineLayoutTextPainterare new public API. Fine for testing, but please add a doc line and a CHANGELOG entry.- Once it's in, this ships with 1.13.0 together with #26 and the fixes above.
Thanks again, this is a big improvement once the tail-width issue is sorted!
…er one letter Found while reviewing #29 by dumping debugLineFragments() on a 300px box and checking each line's real width. - Lines ran up to half a glyph past the box. getPositionForOffset returns the nearest caret, not the last one that fits; every full CJK line was 304px in a 300px box. The new _fitPrefixLength helper corrects the probe against real prefix widths. - RTL paragraphs did not wrap. The x = maxWidth probe read an RTL painter from its logical end, so an Arabic paragraph put 72 chars (1,152px) on line one, then one glyph per line. The probe is mirrored for RTL. - A word wider than the line was split after its first letter, leaving a column of one-letter lines. It now breaks after as many characters as fit, as Flutter's Text does. - A word that does not fit beside a float was split the same way ("c" / "onsectetur"). The line now moves below the float, as CSS line boxes do. - Force-split lines kept their trailing space, so centered and right-aligned lines were off by a space. Trailing collapsible spaces are now trimmed, as _splitTextFragment already did. Copying is unaffected; it reads the unsplit fragments. - word-break: break-all fills the line by character instead of first looking for a space. _prefixWidth reads the caret offset: summing getBoxesForSelection(0, k) is O(k) per call and slowed long paragraphs.
|
@DrkXo the line-breaking fixes are in on What changed in
The goldens on the branch are regenerated and green, and |
…ed CJK text Resolves severe UI freezes and layout slowness when rendering documents containing long paragraphs without whitespace (e.g. CJK EPUBs). - Add single-pass native multi-line layout fast path via ICU LineMetrics when no floats encroach on available width. - Bound candidate prefix measurement in fallback line-breaker loop to eliminate quadratic HarfBuzz text shaping overhead. - Ensure loop termination on non-positive line widths by clearing floats or flooring to 1.0. - Exclude trailing whitespace in caret measurement to match native LineMetrics. - Add deterministic parity, intrinsic width, and call-counting tests in test/line_breaking_perf_and_parity_test.dart.
fc8f17c to
2a98a2f
Compare
|
Thanks 🙏 for landing the upstream line-breaking fixes and regenerating the golden references! The PR has been rebased cleanly onto 1. Blocker: Text after an inline element wrapped onto a single line
2. Integration with upstream
|
vietnguyentuan2019
left a comment
There was a problem hiding this comment.
Thanks for the quick rebase, this is very close. 🙏 The <b>Note:</b> blocker is fixed. I checked out 2a98a2f and compared it against release/1.13.0 on 24 inputs (same harness as before: debugLineFragments() at 300px, plus the existing suites). Everything is identical except one regression and three differences that need a decision or a test.
1. Regression (blocker): a floated block's text is now measured
test/flagship_execution_audit_test.dart fails on this branch (it passes on release/1.13.0):
Float layout KNOWN GAP — a floated block sized by its own text is not floated
Expected: <208.0> Actual: <216.0>
<div style="float:left;width:150px">One two three … thirteen.</div><p>One two three … thirteen.</p>The first line is now 24px tall instead of 16px, so the document is 248px instead of 240px. I bisected it to one hunk, in processFragment just before fragment.offset = Offset(currentX, currentY):
if (fragment.measuredSize == null || fragment.width.isInfinite) {
_measureFragment(fragment);
}The measuredSize == null half is what changes layout. The floated div's text fragment reaches this point with measuredSize == null (width 0 on main), and it is now measured and gets a height. The isInfinite half is the one the oversized-tail fix needs. With the condition narrowed to just if (fragment.width.isInfinite), flagship_execution_audit_test.dart passes again and all 30 tests in line_breaking_perf_and_parity_test.dart and line_breaking_regression_test.dart still pass. Please make that change. Why the float's text fragment is unmeasured is a separate known gap (LIMITATIONS.md), so it's not for this PR.
(The CI "no checks" on this PR is because the base isn't main. Please run the whole flutter test test/ packages/hyper_render_core/test/ --exclude-tags golden locally before pushing. This test is in that command.)
2. Intended differences, please list them in the PR description
I ran the golden workflow on this branch plus the one-line fix above. Only one golden changes: cjk_kinsoku.png. The Japanese paragraph now fills line 2 after the space between two sentences (the 。 + newline in the HTML) instead of starting the next sentence on a new line. The same thing happens in mixed text: <p>lorem ipsum dolor sit amet 日本語のテキストが混在する文章 …</p> now continues sit amet 日本語のテキストが on one line, where main moved the whole CJK run down. Both look right to me (that's how a browser breaks CJK), so I'm fine with them, but they come from ICU breaking instead of _splitTextFragment, so they should be named in the description and in the 1.13.0 CHANGELOG entry. I'll regenerate that golden on our side.
3. Ellipsis change needs a test
The _tokenizeBlock change (emit _BlockStartFragment when textOverflow == ellipsis) is a real fix: on main, <div style="overflow:hidden;text-overflow:ellipsis;white-space:nowrap">…long text…</div> as the first block wraps onto 9 lines instead of truncating, and with this PR it gives one line ending in …. Thanks for finding it! But the only ellipsis test builds the style by hand (textOverflow: TextOverflow.ellipsis), so it never goes through the resolver or the first-block path, and it would pass without the fix. Please add one that parses that HTML as the first element (no preceding <p>) and asserts a single line ending with …. Then mutation-check it: remove the || and make sure the test fails.
Small
- Regression test names in the PR description say "2,541 / 2,541 passing". With my one-line fix, and including
flagship_execution_audit_test.dart, the full run onrelease/1.13.0is 2542 + your 15 new tests. Just re-run the count after the changes.
After 1 and 3, I'll re-run the same comparison and merge. 🙌
… to #29) #29 re-measured every fragment with `measuredSize == null` right before it is placed. A floated block's own text arrives unmeasured (a known gap), so it got a real height and the first line grew from 16px to 24px; this broke flagship_execution_audit_test. Only an oversized tail (estimated infinite width) needs measuring there. Also adds regression tests for the first-block `text-overflow: ellipsis` fix from #29 (parsed HTML, no preceding block) and for the float case, and notes the CJK line-filling behavior change in the core changelog.
|
Merged into Two things I changed on top, so you know what happened to your branch: (1) narrowed |
|
Thank you so much for the thorough review, catching that float edge case, and especially for taking the time to add the tests and follow-up fixes to get this across the finish line! 🙏 I really appreciate the detailed explanation of the measurement condition and why the floatDiv behaved that way—that was a great catch. Thanks again for the guidance and wonderful collaboration! |
|
"Hi @vietnguyentuan2019, Update: I’ve rebased the customizable_selection_anchors branch with the latest upstream main (commit a762026). On a related note, I explored customizable selection handles in this branch (feat(selection): add customizable start and end anchor builder), supporting HyperSelectionAnchorBuilder across single-box and virtualized modes with reactive drag states. Would this feature be a welcome addition to hyper_render?" |
|
Thanks, this is welcome. The builder API looks good and the default path is untouched. I ran your branch on current
This touches core, so it will go out as a minor release (core 1.14.0). Please open it as a PR against |
Fixes #28
Summary
Addresses the severe layout freeze (~13.4s down to ~250ms) when rendering documents containing long paragraphs without whitespace (CJK literature, raw text dumps, and unspaced strings), as reported in #28.
Thanks 🙏 for the detailed feedback.
Root Cause
Previously, line breaking for long text blocks measured candidates via repeated substring slicing and queue bouncing across every line. For paragraphs without whitespace, this resulted in quadratic$O(N^2)$ HarfBuzz text-shaping passes over thousands of characters per line.
Changes
TextPainterpass using native ICU line-breaking (computeLineMetrics()), completing in_safeCandidateCharLimit), eliminating the quadratic overhead when measuring line breaks.1.0.LineMetrics.widthto ensure identical geometry between fast-path and fallback breaker.Reviewer Feedback & Verification Checklist
getMinIntrinsicWidthandgetMaxIntrinsicWidthfor long unspaced fragments inside shrink-to-fit contexts (<td>table cells anddisplay: inline-blockcontainers). Full text is preserved and measured without artificial prefix clamping.text-align: justifytext-overflow: ellipsis(truncates on single line; does not multi-line wrap)textScaler(1.5×)globalOffsetcontinuity (charEnd == next.charStart),getBoxesForCharRangecross-line rects, and unbroken string extraction ingetSelectedText.TextPainter.layoutcalls during line layout (flutter test test/ packages/hyper_render_core/test/ --exclude-tags golden) runs 100% green (2,541 / 2,541 passing).