Skip to content

Performance updates#717

Open
zoltanhosszu wants to merge 15 commits intomainfrom
performance-updates
Open

Performance updates#717
zoltanhosszu wants to merge 15 commits intomainfrom
performance-updates

Conversation

@zoltanhosszu
Copy link
Contributor

Performance and Memory Management Improvements for Lexxy

This PR addresses performance bottlenecks and memory leaks identified in Lexxy, particularly noticeable when editing large documents.

Key Changes

DOM Query Caching in Toolbar

  • Cache button and menu element references on initialization instead of querying the DOM on every update
  • Removes ~15 querySelector calls per keystroke

Table State Consolidation

  • Consolidate scattered table state reads into a single cached tableState getter
  • Previously, each table operation triggered separate editor state reads; now they share one cached result per update cycle

Memory Leak Fixes

  • Add destroy() method to CommandDispatcher with proper cleanup of drag/drop handlers and command registrations
  • Store and call unregister functions for all registerUpdateListener and registerCommand calls
  • Fix event listener cleanup in toolbar, code language picker, and table tools

Update Listener Optimization

  • Skip update listener callbacks when dirtyElements and dirtyLeaves are both empty (no actual content changes)
  • Reduces unnecessary work during selection-only changes

This prevents querySelector × buttons calls on each selection change
Loading state for sandbox for delayed update
`discrete: true` forces synchronous execution - it prevents the update from being batched with other pending updates.
Instead of each table selection doing a separate `getEditorState().read()` we combine them into a single tableState() function, returning a combined object for the current state.
Copy link
Collaborator

@samuelpecher samuelpecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A great step in the right direction. I particularly like the tableState extraction if we avoid any issues around batched updates potentially calling stale state.

// fails because no root node detected. This is a workaround to deal with the issue.
requestAnimationFrame(() => this.editor?.update(() => { }))
})
}, { discrete: true })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm onboard with the idea here, especially removing the frame await, but this can't be a large part of the performance win since it's only on programmatic set value ?

Do we have a test that fails without the previous code, and then passes with { discrete: true } to be sure this handles the problem?

I'd also port the comment to explain the use of discrete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest, this was a suggestion from Claude, after analyzing Lexical+Lexxy code. Tests pass in all 3 variants, so we might not have a proper test for this?

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

Comments