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

Reduce Position#parent use by 50%+ #6579

Closed
Reinmar opened this issue Apr 8, 2020 · 2 comments · Fixed by ckeditor/ckeditor5-engine#1837
Closed

Reduce Position#parent use by 50%+ #6579

Reinmar opened this issue Apr 8, 2020 · 2 comments · Fixed by ckeditor/ckeditor5-engine#1837
Assignees
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature. type:performance This issue reports a performance issue or a possible performance improvement. type:refactor This issue requires or describes a code refactoring.

Comments

@Reinmar
Copy link
Member

Reinmar commented Apr 8, 2020

📝 Provide a description of the improvement

This is a performance graph for undoing removal of the "long (semantic)" content in http://localhost:8125/ckeditor5/tests/manual/performance/setdata.html.

As you can see Position#parent is the most expensive thing here. We could not find ways to make it much faster by changing its implementation.

However, then I noticed when it's used and after looking at implementation of #nodeAfter, #nodeBefore, and #textNode I realised that we're executing the #parent getter many times per each of these prop getters calls.

You can find the current implementation of these functions here: https://github.com/ckeditor/ckeditor5-engine/blob/ff045099fdea35c2af90a6da3d1a546845aeb3ea/src/model/position.js#L199-L242

Let's see if we can save 50%+ of time by rewriting them into uglier, but more optimal forms :).


If you'd like to see this improvement implemented, add a 👍 reaction to this post.

@Reinmar Reinmar added the type:improvement This issue reports a possible enhancement of an existing feature. label Apr 8, 2020
@Reinmar
Copy link
Member Author

Reinmar commented Apr 8, 2020

After playing with the code for 15 minutes I got down to this:

From 1165ms to 378ms :).

The entire undo action got down from 3.2s to 2.2s.

PR's coming!

@Reinmar
Copy link
Member Author

Reinmar commented Apr 8, 2020

Loading "long (semantic)" content into the editor:

  • #parent: 2300ms -> 995ms
  • entire action: 5.5s -> 4.0s

@Reinmar Reinmar added type:performance This issue reports a performance issue or a possible performance improvement. type:refactor This issue requires or describes a code refactoring. package:engine labels Apr 8, 2020
@Reinmar Reinmar self-assigned this Apr 8, 2020
@Reinmar Reinmar added this to the iteration 31 milestone Apr 8, 2020
Reinmar added a commit to ckeditor/ckeditor5-engine that referenced this issue Apr 10, 2020
Other: Improved performance of `Position` getters (~60% gain). Reduced time of some common tasks (like loading complex content) by up to 30%. Closes ckeditor/ckeditor5#6579.
mlewand pushed a commit that referenced this issue May 1, 2020
Other: Improved performance of `Position` getters (~60% gain). Reduced time of some common tasks (like loading complex content) by up to 30%. Closes #6579.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature. type:performance This issue reports a performance issue or a possible performance improvement. type:refactor This issue requires or describes a code refactoring.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant