Skip to content

Make Line::items() glyph-run iteration linear and cluster-free - #21

Open
nicoburns wants to merge 7 commits into
mainfrom
devin/1788398127-glyph-run-iter
Open

Make Line::items() glyph-run iteration linear and cluster-free#21
nicoburns wants to merge 7 commits into
mainfrom
devin/1788398127-glyph-run-iter

Conversation

@nicoburns

Copy link
Copy Markdown
Member

LLM Contributions: This PR was written with Devin, which also ran the benchmarks below.

Line::items() (the positioned GlyphRun iterator used by every renderer, e.g. Blitz/Vello) rebuilt run.visual_clusters().flat_map(|c| c.glyphs()).skip(self.glyph_start) on every next(). That is O(glyphs × glyph runs) per run, and since the parley_engine refactor each skipped element is a by-value Cluster (Run + Atom + Grapheme) plus a large composed glyph-iterator state, so this path went from ~200 ms to ~4 s of a Blitz layout of the Barack Obama Wikipedia page (see benchmark comment on DioxusLabs/blitz#832).

Two independent fixes, as separate commits so they can be measured separately:

  1. Cursor (28ea7e7): GlyphRunIter keeps a peekable cluster cursor for the current run and resumes from it instead of restarting + skip. Removes the quadratic term.
  2. Reference / slice (5a2c020): GlyphRunIter no longer materialises Clusters or calls Cluster::glyphs(). A small AtomCursor walks the run's ShapedSlice atoms (atoms_start()/atoms_end(), prev() for RTL), reads the style index from atom.characters()[0], and sums glyph counts/advances from shaped_cluster_glyphs() plus EffectiveSpacing::gaps(atom) — the same quantities Run::glyphs_in produces, without constructing the composed iterator. Glyph runs always end on atom boundaries (style is per character, Cluster::style_index already used the atom's first character), so resuming at atom granularity is exact.

Semantics (style_index, glyph_start, glyph_count, offset, advance, RTL order, letter/word spacing, justification, ligatures/glyph-less clusters) are unchanged; GlyphRun::glyphs() still uses glyphs_in(...).skip(glyph_start).take(glyph_count). The Clusters iterator is untouched for other consumers.

Also:

  • parley_bench: new Styled Items benchmark that walks line.items() / positioned_glyphs() of a pre-built styled layout. None of the existing benchmarks executed GlyphRunIter::next, which is why this regression was invisible to Tango.
  • parley/src/tests/test_glyph_runs.rs: checks items() against a naive split of visual_clusters().flat_map(glyphs) (style, glyph ids, advances, offsets) for latin/arabic/japanese/mixed text × spacing on/off × start/justify.

cargo fmt --all --check, cargo test --workspace, cargo clippy --workspace --all-targets -- -D warnings pass.

Benchmarks

Variants (all include the Styled Items bench commit f224cc7 so Tango can compare it):

Revision Description
base f224cc7 main (83acf8f) + bench only
cursor 28ea7e7 fix 1 only
ref-only scratch (not pushed): 5a2c020 with the cursor disabled — AtomCursor::new(run) per next() and skipping glyph_start glyphs atom by atom fix 2 only
both 5a2c020 this PR

Tango, parley_bench Styled Items (<variant>/target/benchmarks/main compare <base>/target/benchmarks/main -t 1 -o; columns [base ... variant])

Benchmark cursor ref-only both
arabic 20 chars [12.1 us ... 6.2 us] -49% [11.5 us ... 1.6 us] -86% [11.4 us ... 886 ns] -92%
latin 20 chars [11.6 us ... 5.9 us] -50% [11.8 us ... 1.6 us] -86% [11.6 us ... 713 ns] -94%
japanese 20 chars [11.1 us ... 5.9 us] -47% [11.3 us ... 1.6 us] -86% [12.2 us ... 971 ns] -92%
arabic 1 paragraph [67.5 us ... 32.8 us] -51% [71.3 us ... 9.7 us] -86% [66.8 us ... 5.0 us] -93%
latin 1 paragraph [51.7 us ... 29.1 us] -44% [56.8 us ... 7.7 us] -87% [52.2 us ... 3.1 us] -94%
japanese 1 paragraph [125.3 us ... 62.3 us] -50% [117.5 us ... 16.4 us] -86% [116.7 us ... 9.1 us] -92%
arabic 4 paragraph [275.6 us ... 133.7 us] -52% [268.4 us ... 37.0 us] -86% [266.5 us ... 20.5 us] -92%
latin 4 paragraph [207.2 us ... 116.3 us] -44% [211.4 us ... 28.5 us] -87% [213.5 us ... 13.0 us] -94%
japanese 4 paragraph [161.5 us ... 80.5 us] -50% [163.3 us ... 22.8 us] -86% [171.1 us ... 13.4 us] -92%

All other benchmarks (Default Style, Styled, Word + Letter Spacing, Repeated Justification) don't call items(); they moved within ±3% in either direction across the three comparisons (noise/code layout).

Blitz, Barack Obama Wikipedia page, headless, 1280 px (same harness/inputs as the DioxusLabs/blitz#832 report)

Blitz main (1884860, with the API-compat edits needed for Parley main) built --release with [patch] pointing at each Parley worktree; 3 warm-ups + 20 iterations per run, 3 interleaved rounds (base cursor ref both ×3), medians of the per-round medians:

Variant First parse+style+layout median / p90 (ms) Relayout median / p90 (ms) Peak RSS (MB)
base (Parley main) 1051 / 1109 929 / 986 186.8
cursor 473 / 510 352 / 371 186.7
ref-only 303 / 328 183 / 194 186.7
both (this PR) 263 / 282 151 / 172 186.8

Per-round relayout medians: base 929/926/946, cursor 352/348/353, ref-only 185/181/183, both 144/151/156. For reference, Blitz main with crates.io Parley 0.11.1 measured 147 ms relayout on the same machine, so this restores the pre-refactor cost of the renderer's item walk.

Interpretation: the slice walk alone buys more than the cursor alone (the constant per skipped element was ~18× the quadratic-term exposure at Wikipedia-paragraph glyph-run counts), but only both together make the iteration O(n) with cheap elements; the remaining line.items() cost in the Blitz profile is now negligible.

Changelog

Changed

  • Line::items() no longer re-scans a run's glyphs for every glyph run it yields, and no longer builds Clusters to do so; iterating a line's positioned items is now linear in the number of glyphs.

Link to Devin session: https://dioxus.staging.devinenterprise.com/sessions/3a0b8cc210c041d8955c841fb2a76a8e
Open in Devin Desktop: https://dioxus.staging.devinenterprise.com/desktop/session/3a0b8cc210c041d8955c841fb2a76a8e?variant=devin-insiders
Requested by: @nicoburns

@staging-devin-ai-integration

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

nicoburns and others added 5 commits September 3, 2026 12:19
Adds long-single-line benchmarks to measure cases where performance is
`O(line length)`
These have been used to test
linebender#762

**LLM Contributions**: Generated with Fable 5.1 Low

**Changelog**: None
<!-- Please ensure that you have reviewed our LLM ("AI") policy at
https://linebender.org/wiki/llm-policy/.

If you did not use any LLM tools, please replace `Unspecified` with
`None`. -->
LLM Contributions: Mostly generated.

Follow-up to
linebender#738 (comment).

This is also what Gecko and Blink do (with whitespace collapsing off).
Compare:
https://developer.mozilla.org/en-US/play?id=gilITaZ%2B0hCJVuNWyCoME9xqzkpiKGs0u2alPNKuvKXYzUEm2XX1efjm7j5qJPFZdiEZU%2FYl7eSQxZlN.

<!--
If our users need to know about this change, please describe that in the
quote block below.
What you write here will be edited by us later - it doesn't need to be
perfect.
If this change doesn't need a changelog entry, please replace the next
line with `**Changelog: None**`.
-->
**Changelog: None**
… hard/explicit newline (linebender#746)

LLM Contributions: Bug identified and code generated with Devin Ultra.

Fix `calculate_content_widths` when an inline boxes immediately follows
a hard/explicit newline. They were previously incorrectly counted as
being before the newline because the "Mandatory Break" is only set on
the character *following* the line break. **This change mirrors what the
line-breaking is already doing into the content width calculation.**

**Changelog**

> ### Added
>
> - Fix `calculate_content_widths` when an inline boxes immediately
follows a hard/explicit newline. They were previously incorrectly
counted as being before the newline.

---------

Signed-off-by: Nico Burns <nico@nicoburns.com>
Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
… glyphs

Each call to GlyphRunIter::next rebuilt the run's cluster/glyph iterator and
skipped glyph_start elements, making iteration over a run's glyph runs
O(glyphs * glyph runs). Keep a peekable cluster cursor across calls instead.
@staging-devin-ai-integration
staging-devin-ai-integration Bot force-pushed the devin/1788398127-glyph-run-iter branch from 5a2c020 to 8253301 Compare September 3, 2026 18:56
…lusters

Reading style index, glyph counts and advances from the run's ShapedSlice
avoids constructing a Cluster (Run + Atom + Grapheme) and its composed glyph
iterator for every cluster, which dominated the cost of iterating a line's
positioned items.
@staging-devin-ai-integration
staging-devin-ai-integration Bot force-pushed the devin/1788398127-glyph-run-iter branch from 8253301 to ef346d5 Compare September 3, 2026 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants