Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid transforming dirty paths if the op won't touch them #4735

Merged
merged 2 commits into from
Dec 18, 2021

Conversation

steve-codaio
Copy link
Contributor

Description
While large bulk edits are not a common scenario, we noticed that the dirty path tracking during batches has an O(n^2) behavior in relation to the number of ops. This is because as every op gets applied, we loop over all previous dirty paths and transform them. This implements an optimization where we can bypass the no-op transform entirely if we know the op won't affect paths at all.

In a simple test where I had a slate with 1000 texts and I applied an 'insert_text' op appending a character to each text, I saw about a 50% time savings (went from ~2s down to ~1s)

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Dec 16, 2021

🦋 Changeset detected

Latest commit: 1d22cb8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

for (const path of oldDirtyPaths) {
const newPath = Path.transform(path, op)
add(newPath)
if (canTransformPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was hoping we could optimize this more but I realized we have to rebuild the set for de-duping each time. Maybe the key set should be persisted with the DIRTY_PATHS so this doesn't need to be reconstructed each time. This code path can be so hot on large edits.

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 can investigate storing the path keys alongside the dirty paths and seeing what additional savings we can get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to reuse the array/set entirely if the op won't touch them. Shaved an additional ~10% of time off the test mentioned in the summary

Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

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

Thanks, this looks promising.

I'll plan to land it in a couple of days unless anyone objects.

@dylans dylans merged commit e5427dd into ianstormtaylor:main Dec 18, 2021
This was referenced Dec 18, 2021
z2devil pushed a commit to z2devil/slate that referenced this pull request Dec 6, 2024
…ylor#4735)

* Avoid transforming dirty paths if the op won't touch them

* Avoid the loop entirely if op doesn't affect path
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.

3 participants