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

Improve performance of _insertNodes #17522

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

filipsobol
Copy link
Member

@filipsobol filipsobol commented Nov 22, 2024

Suggested merge commit message (convention)

Other (engine): Improve performance of _insertNodes.

Other (utils): Change the implementation of spliceArray to modify the target array for better performance.

MINOR BREAKING CHANGE (utils): spliceArray now modifies the target array and doesn't accept a fourth (count) argument.


In #17456 I improved the performance of spliceArray. However, it turned out that it still does more than we need and because it's called often, we pay a price for it. This PR reduces the number of intermediate arrays we make whenever _insertNodes is called.

Below are the improvements (while on the remove-registerViewToModelLength branch, the percentages will be different on master)

Test name Change in % Change in ms
formattingLongP -6.1 -260
inlineStyles -7.9 -650
mixed -5.0 -120
wiki -4.3 -140

@scofalik
Copy link
Contributor

How does the custom insertAt compare to native splice for parameters volume that does not crash splice?

Question two: maybe we could reimplement custom splice as what you proposed for insertAt?

scofalik
scofalik previously approved these changes Nov 26, 2024
@filipsobol
Copy link
Member Author

How does the custom insertAt compare to native splice for parameters volume that does not crash splice?

Native .splice() is slower by about 25ms in inline-styles test.

Question two: maybe we could reimplement custom splice as what you proposed for insertAt?

This is the only place where the spliceArray helper was used, but it did not modify the original array. Changing its implementation would be a breaking change, which I want to avoid.

Also, in the first loop, is this direction like this because this way you change the targetArray length only once? If so, maybe it would be better to change the length before the loop and then go from the beginning to the end (for clarity purposes, it's easier to read).

Changing the array's length upfront is (surprisingly) much slower.

@scofalik
Copy link
Contributor

Changing the array's length upfront is (surprisingly) much slower.

No comments.

How does the custom insertAt compare to native splice for parameters volume that does not crash splice?

Native .splice() is slower by about 25ms in inline-styles test.

Question two: maybe we could reimplement custom splice as what you proposed for insertAt?

This is the only place where the spliceArray helper was used, but it did not modify the original array. Changing its implementation would be a breaking change, which I want to avoid.

I am for replacing it and having this as minor breaking change. In general, people should not have a need to use spliceArray from our library even though it was available. Changing code from spliceArray() to insertAt() should be relatively easy. If anyone needs spliceArray() for some reason, they can even copy it from our code.

@filipsobol filipsobol force-pushed the improve-performance-of-_insertNodes branch from 64642f8 to 2b1a75c Compare November 27, 2024 14:45
@filipsobol filipsobol force-pushed the improve-performance-of-_insertNodes branch from 2b1a75c to e80e213 Compare November 27, 2024 14:46
scofalik
scofalik previously approved these changes Nov 27, 2024
@filipsobol filipsobol merged commit fbf4a17 into master Nov 27, 2024
10 checks passed
@filipsobol filipsobol deleted the improve-performance-of-_insertNodes branch November 27, 2024 15:13
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