Cull anchor-name registration by viewport proximity while editing - #360
Cull anchor-name registration by viewport proximity while editing#360johannesmutter wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Follow-up fix: gap markers sampled their row-gap-narrowing pair (--_f/--_s) from items 0/1 unconditionally, which dangled once the array's head left the overscan zone — one dangling anchor() invalidates the whole inset declaration (IACVT) and collapsed every marker and the node caret to a zero-width sliver at the left edge when scrolled toward the document tail. Markers now sample the pair from the near set (spacing is uniform per array, so any adjacent pair measures the same gap); with no near pair, the .pair gate skips the narrowing rules. Regression-tested: gap_visibility scrolls a tall document to the tail, confirms the head anchors are culled, and asserts markers and the node caret keep real boxes. |
|
Hey quick request for this and future PRs. Your PRs have a lot of detail in the description, but it's hard for me to understand their purpose/impact/risks. I guess what would help me here, and in other situations is a few lines that motivate the work (preferred in your own words), basically answering those questions:
the additional more complete info you add is great, but very hard to read for me if i'm not in your head (like not have the context).
like that it'd consider a good thing, because a dev can then assume it's always there and be used for positioning. so changing this i'd need to understand the real cost of it, will the existence of anchors (even when not targetted) cause any performance problems? if yes why? 29.3ms vs 31.8ms doesn't seem like a huge gain? so this is less about speed, but more about memory? |
|
Thanks for those guidelines! The basic idea of my PR is to only register anchor-name for nodes that are close enough to the viewport to actually be relevant. Previously, every Node, TextProperty, and CustomProperty registered an anchor unconditionally, which meant the browser had to maintain anchor bookkeeping for the entire document The problem that motivated this PR:I had a doc with ~450.000 characters over ~1600 textblocks. Typing in any block was quite slow and Fable found several reasons for it. One of them was the browser-internal bookkeeping for anchors. My first guess was e.g. because layout related styles like margin, transform or position of e.g. annotations on text spans or annotations on nodes would be the problem, but even after stripping all the css with just the anchor names set it was slow. Stripping the anchor name for all out-of-view nodes however made typing slightly less laggy, not dramatically though, but enough to notice. The core problem in such large documents is still the sheer number of DOM nodes. So this is rather a tiny optimisation. If we would really want to improve performance in large docs something more fundamental has to change. now the question of course is: "Does Svedit need to register all these things as anchors at all times"? The good thing is that position-visibility is by default "anchors-visible". So if you anchor something to e.g. a TextProperty and then that anchor is unregistered because its out of view, then the thing that attached to it would also disappear. Only if you would set "always", then the anchor is now undefined and this can cause problems: “The anchor-dependent position can no longer resolve, so the element may fall back to its static/normal position (or otherwise behave according to the positioning rules), which could potentially place it somewhere unexpected and cause overlap.” So bottom line is: is a small performance optimisation worth that developers might be confronted with not being able to use One such use case could be e.g. a comment UI: the comment element uses the left or right anchor reference always, but top/ bottom only when the anchor itself is in view. When I scroll down and I want the comment element still visible, it could snap to the edge of the window. So in that case if the anchor becoems undefined, it would no longer resolve the left/ right position. The use case feels quite constructed, but it could be one ... In general I think it's rare ... If we find other ways to improve performance in large docs, I think this PR should not land. But if we reached the ceiling of what's possible and rely even on such small improvements then we should consider it. |
|
Thank you, that additional context helps a lot! It's good to have this exploration, but I agree we should check if we can find optimization potential at a more root cause level. Maybe I can dedicate a whole release for that, but first releasing Editable and getting the Sveidt APIs into the hands of more people has priority. |
Every
Node,TextProperty, andCustomPropertyregisters a CSS anchor (anchor-name: --{path}) unconditionally. The browser's anchor bookkeeping runs on every layout pass and every native selection update, and scales with the number of registered anchors. #329 established this for node gaps ("anchor positioning is O(N)") and gated gap anchors by viewport proximity; this PR extends the same gate to the remaining anchor populations.Changes
Node,TextProperty,CustomProperty: whileeditable, registeranchor-nameonly when the owning node is in the overscan set (same source and two-derived pattern as NodeGap's.positioned). View mode is unchanged: every anchor registers, so read-mode overlays (comment layers etc.) are unaffected.NodeSelectionMarkers: render overlays only for selected nodes in the overscan set, so no overlay targets a culled anchor. Select-all no longer creates one positioned element per document node; markers follow the near-set while scrolling.node_visibility:split_node_path_str/split_property_path_strexported (the private#split_pathnow delegates).Positioned gaps already require both neighbor nodes in the same near-set, so gap markers always find their neighbor anchors; the two gates compose without coordination.
Numbers (perftest, 2,000 nodes, editable; A/B on the same machine, patch-independent transaction-apply time as clock control: 29.3 vs 31.8ms)
setBaseAndExtentper keystrokeMemory: the anchor registry shrinks from O(document) to O(viewport) (~2,000 → ~50 registered anchors at this scale) and giant node selections render ~15 overlays instead of 2,000. Both live in renderer internals, not the JS heap, so no script-measurable number; JS heap delta is noise.
Considered and rejected: skipping the selection re-render when the DOM selection already matches the model. Instrumented: it would fire on ~1 of 11 keystrokes, because Blink resets the caret to offset 0 when the text node's data is replaced. Nothing to skip on the hot path.
Note for app authors: in edit mode, overlays anchored to svedit path anchors should target the selection or something near the viewport. Existing consumers (toolbars, link popover, slash menus, selection markers) already do.
Verification: 124/124 unit tests, svelte-check and lint clean. Interactive checks with real input: typing, toolbar anchoring on text selection, node selection overlays including select-all, marker catch-up while scrolling mid-selection.