fix: skip redundant text marks when the range spans multiple style ranges - #1059
Open
frioux wants to merge 1 commit into
Open
fix: skip redundant text marks when the range spans multiple style ranges#1059frioux wants to merge 1 commit into
frioux wants to merge 1 commit into
Conversation
…nges mark_with_txn already skips marks that would not change anything, but the check relied on StyleRangeMap::get_styles_of_range, which returns None whenever the marked range spans more than one style range. On any document whose style ranges are fragmented (i.e. any document with styles), re-asserting an identical mark recorded a new op every time. Each redundant mark op leaves a pair of style anchors in the container state forever - they survive snapshots and are never consolidated - and every styled read walks all of them, so callers that re-assert marks (e.g. editor bindings syncing mark state) permanently degraded styled reads without bound: reading a 724-char paragraph went from ~5us to ~4ms after 1000 redundant marks. Add StyleRangeMap::range_has_key_value, which scans the style ranges covering the range and reports whether every position already resolves the key to the given value, and fall back to it (for both attached and detached texts) when the single-leaf fast path returns None.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TextHandler::mark_with_txnalready tries to skip marks that would not change anything, but the check relies onStyleRangeMap::get_styles_of_range, which returnsNonewhenever the queried range spans more than one style-range leaf. On any document whose style ranges are fragmented — i.e. any document that has styles — the dedup silently never fires, and re-asserting a byte-identical mark records a newStyleStart/StyleEndpair every time.That matters because each redundant mark op leaves a pair of style anchors in the text container's state forever: they survive (shallow) snapshots, are never consolidated even when fully superseded, and every styled read (
get_richtext_value/toDelta) walks all of them, uncached. A well-meaning caller that re-asserts mark state (asloro-prosemirrorupdateLoroTextdoes on every change — see loro-dev/loro-prosemirror#82) can permanently and invisibly ruin a container. Measured with the included perf guard on a 724-char paragraph:(On a real heavily-edited page we saw 16–25 ms per styled read and a 135 KB snapshot for 4 KB of visible text.)
Fix
Add
StyleRangeMap::range_has_key_value, which scans the style ranges covering the queried range (mirroring the existingrange_contains_keytraversal) and reports whether every position already resolves the key to the given value.mark_with_txnandmark_for_detachedfall back to it when the single-leaf fast path returnsNone, using exactly the same skip predicate as before — the LWW winner per range must equal the new value — so this only generalizes the existing dedup semantics to fragmented ranges. No entity indices, encoding, or merge semantics change; marks that alter any part of the range still apply.This doesn't GC anchors that already accumulated (that would require entity-index-safe compaction), but it closes the path by which they accumulate from redundant marks.
Validation
crates/loro-internal/tests/richtext.rs: three new tests — redundant mark and redundant unmark spanning style boundaries record no ops (both fail before this change), and a mark that changes part of the range still applies.crates/loro/tests/perf_redundant_marks.rs:#[ignore]d perf guard in the style ofperf_text_insert_quadratic.rs(numbers above; run withcargo test -p loro perf_redundant_marks_do_not_degrade_styled_reads --release -- --ignored --nocapture).cargo test -p loro-internal(318 lib + all integration suites) andcargo test -p loropass;cargo fmt --all;cargo clippy -p loro-internalintroduces no new warnings.🤖 Generated with Claude Code
https://claude.ai/code/session_01KQw7nfh6KaUhQy6ZQUZwMm