Add `parley_core` itemizer, refactor `parley` to use it by tomcur · Pull Request #670 · linebender/parley

tomcur · 2026-07-02T00:10:24Z

This adds Analysis::itemize, returning an iterator of items to be shaped individually.

The itemizer takes care of splitting text on changing bidi levels and script (and more in the future, like glyph orientation). I've ported the heuristics parley currently uses to fill in script for characters without any particular script (i.e., Common, Inherit, or Unknown). We may at some point want to perform bracket matching (see e.g. UAX 24 § 5.2), which will require some scratch space. At that point, an API like Itemizer::itemize(&mut self, analysis: &Analysis, split_after: ...) -> Items<...> might make sense instead.

This benches neutral on my machine

$ cargo bench -q --bench=main -- compare ../target/benchmarks/main -t 8.
Default Style - arabic 20 characters               [   8.7 us ...   8.7 us ]      -0.36%
Default Style - latin 20 characters                [   4.2 us ...   4.2 us ]      +0.31%
Default Style - japanese 20 characters             [   8.1 us ...   8.1 us ]      -0.31%
Default Style - arabic 1 paragraph                 [  47.4 us ...  47.5 us ]      +0.23%
Default Style - latin 1 paragraph                  [  16.2 us ...  16.1 us ]      -0.14%
Default Style - japanese 1 paragraph               [  69.8 us ...  69.1 us ]      -1.03%*
Default Style - arabic 4 paragraph                 [ 199.4 us ... 200.8 us ]      +0.72%
Default Style - latin 4 paragraph                  [  61.0 us ...  61.3 us ]      +0.56%
Default Style - japanese 4 paragraph               [  98.5 us ...  97.7 us ]      -0.90%
Styled - arabic 20 characters                      [   9.7 us ...   9.7 us ]      -0.24%
Styled - latin 20 characters                       [   5.4 us ...   5.4 us ]      +0.08%
Styled - japanese 20 characters                    [   8.7 us ...   8.5 us ]      -1.76%*
Styled - arabic 1 paragraph                        [  49.9 us ...  50.0 us ]      +0.23%
Styled - latin 1 paragraph                         [  20.7 us ...  20.7 us ]      -0.00%
Styled - japanese 1 paragraph                      [  75.7 us ...  75.0 us ]      -0.87%
Styled - arabic 4 paragraph                        [ 217.4 us ... 218.8 us ]      +0.64%
Styled - latin 4 paragraph                         [  80.0 us ...  80.8 us ]      +0.94%
Styled - japanese 4 paragraph                      [ 106.9 us ... 105.9 us ]      -0.93%

tomcur · 2026-07-02T00:16:35Z

+            let bidi_level = if self.bidi_levels.is_empty() {
+                self.paragraph_bidi_level
+            } else {
+                self.bidi_levels[0]
+            };


Possibly we can start thinking about #649 (comment).

To clarify, this review comment means that this 'consuming' side implements that, but not the creation side?

That's right. I've not verified whether there are any other places still consuming it, but probably it should be relatively simple to also optimize/symmetrize the RTL case now.

DJMcNab

This looks like a good split to me. None of the concerns are blocking, although I think a little bit of clarify improvement might help things here.

DJMcNab · 2026-07-06T01:16:29Z

+                    // Inline boxes *before* this indexed are popped (this occurs if the itemizer
+                    // split a run and we were not called). We push all boxes to the layout when we
+                    // loop over the produced items.


What do you think of expanding the parenthetical, e.g. to this occurs if the itemiser split a run and we were not called, such as at a bidi boundary

The second sentence of this comment also probably could just be removed, but idk.

Done, and removed the second line, too. I initially wrote it, as "popped" reads a bit as if they're ignored, but I suppose the code is clear enough that at this point it's just doing splitting (as opposed to pushing to layout).

DJMcNab · 2026-07-06T01:26:16Z

+            let bidi_level = if self.bidi_levels.is_empty() {
+                self.paragraph_bidi_level
+            } else {
+                self.bidi_levels[0]
+            };


To clarify, this review comment means that this 'consuming' side implements that, but not the creation side?

DJMcNab · 2026-07-06T01:31:46Z

+    /// The per-char info, parallel to [`Self::char_indices`].
+    char_info: &'a [CharInfo],
+    /// The per-char bidi level, parallel to [`Self::char_indices`].
+    bidi_levels: &'a [BidiLevel],


Is there a particular reason for these not to use core::slice::Iter? I guess you need to peek into both, so it doesn't actually win anything?

Yeah, I considered making them iterators, but they always need peeking, so it doesn't really matter which way you go (under the assumption Peekable<slice::Iter> is as efficient - I haven't measured!). Instead I wrote the code to reslice to the tail every time.

Also Peekable of slice is meaningfully less efficient!

DJMcNab · 2026-07-06T01:32:53Z

+                self.current_script = script;
+            }
+
+            if bidi_level != item_bidi_level || real_script(script) && script != self.current_script


Can this real_script(script) here ever be false?

Whoops, good catch (went through a few iterations of script handling!). I believe dropping this additional check is also valid if we ever default the text's initial script differently (i.e., if there's no "first real script", we currently set it to Latin somewhat arbitrarily, following what parley already did, and this still works if we set it to Common instead).

DJMcNab · 2026-07-06T01:39:36Z

+    const LATN: Script = Script::from_str_unchecked("Latn");
+    const GREK: Script = Script::from_str_unchecked("Grek");
+    const ARAB: Script = Script::from_str_unchecked("Arab");


Have we checked whether:

const LATN: Script = Script::from_bytes(*b"Latn");

works? In that scenario, I might be in favour of removing from_str_unchecked entirely (obviously not in this PR)

(removing from_str_unchecked doesn't sound half bad to me!)

DJMcNab · 2026-07-06T01:47:49Z

-        if level != item.level || script != item.script {
-            break_run = true;
+        let item_style_index = char_style_indices[item_range.char_range.start];
+        let style_index = char_style_indices[item_range.char_range.end];


I feel like this should be a char_range.end + 1, but the fact that none of our tests indicate that suggest I'm mistaken. The name split_after certainly indicates that it semantically means "do we need to split after this range"

The range given is already decided to be an item. I started writing an explanation here, but wrote them as the following docs on itemize instead.

/// The predicate is given a range encoding the current item and considers whether to split /// after that item based on the next character. Iff the predicate returns `true`, the text is /// split after that item; i.e., given a range of `start..end`, the predicate controls whether /// that item is now finished, or whether it is extended to include `end`, at which point it'll /// be `start..end+1` (though note [`TextRange`] encodes both byte and character offsets).

(I wouldn't be opposed to renaming the predicate to something wordier like extend_item_to_next_character and flipping the returned bool's semantics, but merging this for now.)

Gotcha! It's an exclusive range is the key thing I was missing.
Which of course makes sense, as every other range is exclusive!

tomcur force-pushed the parley-core-itemizer branch from c64636c to 18b88f2 Compare July 2, 2026 00:14

tomcur commented Jul 2, 2026

View reviewed changes

tomcur added 3 commits July 3, 2026 11:38

Init itemization

f49a047

Refactor parley to use parley_core::itemizer

0a2b1bc

Clippy

94eff51

tomcur force-pushed the parley-core-itemizer branch from 18b88f2 to 94eff51 Compare July 3, 2026 09:38

DJMcNab approved these changes Jul 6, 2026

View reviewed changes

tomcur added 3 commits July 6, 2026 10:36

Improve wording

ae9376d

Drop unnecessary check

9b21155

debug_assert invariant

95debf3

tomcur force-pushed the parley-core-itemizer branch from 85e9cf6 to 95debf3 Compare July 6, 2026 09:57

tomcur added 2 commits July 6, 2026 11:59

Use Script::from_bytes

a8b7045

Clarify item range param

435ceea

tomcur force-pushed the parley-core-itemizer branch from 6114084 to 435ceea Compare July 6, 2026 10:19

tomcur enabled auto-merge July 6, 2026 10:21

tomcur added this pull request to the merge queue Jul 6, 2026

Merged via the queue into linebender:main with commit 099f40e Jul 6, 2026
24 checks passed

tomcur deleted the parley-core-itemizer branch July 6, 2026 10:28

Uh oh!

Conversation

tomcur commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DJMcNab Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomcur Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tomcur commented Jul 2, 2026 •

edited

Loading

tomcur Jul 6, 2026 •

edited

Loading

tomcur Jul 6, 2026 •

edited

Loading

DJMcNab Jul 6, 2026 •

edited

Loading

tomcur Jul 6, 2026 •

edited

Loading