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

Remove model / view check from is() checks. #6529

Closed
jodator opened this issue Apr 1, 2020 · 4 comments · Fixed by ckeditor/ckeditor5-engine#1836
Closed

Remove model / view check from is() checks. #6529

jodator opened this issue Apr 1, 2020 · 4 comments · Fixed by ckeditor/ckeditor5-engine#1836
Assignees
Labels
package:engine type:performance This issue reports a performance issue or a possible performance improvement.

Comments

@jodator
Copy link
Contributor

jodator commented Apr 1, 2020

📝 Provide a description of the improvement

This was introduced by #4479. Right now I can see this cause performance lags as RootElement.is() is called quite often. This method uses string.replace which done hundred thousend times isn't good.

Another case is that I don't see any usage of this so 🤷‍♂️ why we have this. We can have another way of checking this (ie is it model position, element or whatever) - I'd suggest some flag or a helper on model. But I didn't research that.

📃 Other details

Below is a comparison of a "Performance: pasting data" test using "long (semantic)" input. Test consist invoking paste and doing undo.

Before (self times):

After  (self times):

And impact on whole code:

Before RootElement.is(): total time 9463 ms (includes super.is() calls)

After RootElement.is(): Total time: 3163ms (down by 3)


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

@jodator jodator added package:engine type:performance This issue reports a performance issue or a possible performance improvement. labels Apr 1, 2020
@jodator
Copy link
Contributor Author

jodator commented Apr 2, 2020

@Reinmar I've changed some code to verify the approach and the speed up is visible. Now the question is do we want to keep model: & view: checks?

Changes are here: https://github.com/ckeditor/ckeditor5-engine/compare/i/6529?expand=1. Ps it is not PR ready code so minor work is needed there.

@jodator
Copy link
Contributor Author

jodator commented Apr 2, 2020

And the speedup comparison is here: #6528 (comment).

@Reinmar
Copy link
Member

Reinmar commented Apr 2, 2020

@Reinmar I've changed some code to verify the approach and the speed up is visible. Now the question is do we want to keep model: & view: checks?

IMO, yes. It was introduced later, that's why it's not used. But it should be. I had many cases myself when I had (for some reason) a view element passed in place of a model element or vice versa. If existing checks (especially in writers) were more precise, that would be caught ealier.

@Reinmar
Copy link
Member

Reinmar commented Apr 2, 2020

I reported #6538 and bumped #3818 higher as both would be a great improvement in our API.

@jodator jodator self-assigned this Apr 2, 2020
@Mgsy Mgsy added this to the iteration 31 milestone Apr 6, 2020
Reinmar added a commit to ckeditor/ckeditor5-engine that referenced this issue Apr 8, 2020
Other: Inlined calls to parent classes in model and view `is()` checks to improve editor performance. Closes ckeditor/ckeditor5#6529.
mlewand pushed a commit that referenced this issue May 1, 2020
Other: Inlined calls to parent classes in model and view `is()` checks to improve editor performance. Closes #6529.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:performance This issue reports a performance issue or a possible performance improvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants