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

Performance concerns when dealing with multiple formatting attributes #2336

Closed
oleq opened this issue Apr 1, 2019 · 6 comments
Closed

Performance concerns when dealing with multiple formatting attributes #2336

oleq opened this issue Apr 1, 2019 · 6 comments
Labels
package:remove-format resolution:duplicate This issue is a duplicate of another issue and was merged into it. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@oleq
Copy link
Member

oleq commented Apr 1, 2019

  1. Set the following data in the docs (ckeditor5/build/docs/ckeditor5/12.0.0/features/remove-format.html):
<p><code><i><s><strong><sub><sup><u>You can use the remove format feature to easily clean up text formatting.</u></sup></sub></strong></s></i></code></p><p><code><i><s><strong><sub><sup><u>Some examples of the formatting removed by this feature: bold, italics, underline, strikethrough, code, subscript, superscript, </u></sup></sub></strong></s></i></code><span class="text-tiny"><code><i><s><strong><sub><sup><u>font size</u></sup></sub></strong></s></i></code></span><code><i><s><strong><sub><sup><u> as well as </u></sup></sub></strong></s></i></code><span style="font-family:'Courier New', Courier, monospace;"><code><i><s><strong><sub><sup><u>font family</u></sup></sub></strong></s></i></code></span><code><i><s><strong><sub><sup><u>.</u></sup></sub></strong></s></i></code></p><p style="text-align:center;"><code><i><s><strong><sub><sup><u>Remove format resets the text alignment too.</u></sup></sub></strong></s></i></code></p><p><code><i><s><strong><sub><sup><u>Note: The feature will not erase non-formatting content — </u></sup></sub></strong></s></i></code><a href="https://ckeditor.com"><code><i><s><strong><sub><sup><u>your links</u></sup></sub></strong></s></i></code></a><code><i><s><strong><sub><sup><u> are safe!</u></sup></sub></strong></s></i></code></p>
  1. Click remove format.

Expected: Formatting is removed ASAP

Actual: It takes more than 3s to remove the formatting on a quad core i7. It's just a few paragraphs.

Undoing the operation also takes a lot of time. Probably there are too many deltas produced during the operation.

cc @mlewand @scofalik

@mlewand mlewand self-assigned this Apr 2, 2019
@oleq
Copy link
Member Author

oleq commented Apr 2, 2019

Just to let you know: the sample I provided is not the most common use–case. 4 or 5 formatting attributes on the same chunk of text are not very common, so it's not a high priority issue.

@mlewand
Copy link
Contributor

mlewand commented Apr 2, 2019

I can confirm this problem, experiencing same issue.

Most of the execution time goes to model._runPendingChanges. Looks like a generic issue that should be fixed in the engine:

profiler screenshot

@mlewand mlewand removed their assignment Apr 2, 2019
@pjasiun
Copy link

pjasiun commented Apr 2, 2019

model._runPendingChanges is the method which wraps... basically everything. The first half of this log are all changes in the model and converting it to view and the second half is rendering it to DOM. So it is not surprise it takes most of the time since it do most of the things :D

Don't get me wrong. I do not say there is nothing to be optimise, but everything to be optimised in the editor is somewhere inside _runPendingChanges.

@mlewand
Copy link
Contributor

mlewand commented Apr 2, 2019

@pjasiun sure, I'm just showing that the problem exist. As for pointing out the exact place where issue occurs, I intentionally skipped going into details - we'll get back to this as we have more time to fix this properly.

@Reinmar
Copy link
Member

Reinmar commented Apr 3, 2019

Let's close this as a DUP of https://github.com/ckeditor/ckeditor5-engine/issues/1720.

@Reinmar Reinmar closed this as completed Apr 3, 2019
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-remove-format Oct 9, 2019
@mlewand mlewand added resolution:duplicate This issue is a duplicate of another issue and was merged into it. status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:remove-format labels Oct 9, 2019
@Reinmar
Copy link
Member

Reinmar commented May 5, 2020

Wow, I've just checked this and the result is 0.2s. It was 3s before, so we have more than 10x improvement :exploding_head: 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:remove-format resolution:duplicate This issue is a duplicate of another issue and was merged into it. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

4 participants