Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Improved efficiency of Differ. #1762

Merged
merged 2 commits into from
Jul 18, 2019
Merged

Improved efficiency of Differ. #1762

merged 2 commits into from
Jul 18, 2019

Conversation

oskarwrobel
Copy link
Contributor

Suggested merge commit message (convention)

Fix: Fixed problem with handling very large text nodes. Part of ckeditor/ckeditor5#1875.


Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

@coveralls
Copy link

coveralls commented Jul 18, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 7455550 on t/ckeditor5/1875 into 0a268ab on master.

@pjasiun pjasiun requested review from pjasiun and Mgsy July 18, 2019 10:00
@pjasiun
Copy link

pjasiun commented Jul 18, 2019

Thanks to this fix there is no error but the editor still works super slow with the content provided in the ticket. However, it is not a scope of this PR.

@oskarwrobel oskarwrobel changed the title Improved performance of Differ. Improved efficiency of Differ. Jul 18, 2019
@oskarwrobel
Copy link
Contributor Author

Unfortunately, the test takes too long for CI and it leads to a timeout. I need to remove unit tests for this change.

Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

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

LGTM. It doesn't throw anymore, however, as @pjasiun mentioned, after pasting the content from the sample file, it's still impossible to use the editor because of low performance.

@Mgsy Mgsy merged commit a7ae813 into master Jul 18, 2019
@Mgsy Mgsy deleted the t/ckeditor5/1875 branch July 18, 2019 11:08
@Reinmar
Copy link
Member

Reinmar commented Jul 18, 2019

Did you try to use concat() too? Did you measure the performance of this solution?

@oskarwrobel
Copy link
Contributor Author

Nope, I focused on making the editor not crash. The issue is still open for performance improvements.

@oskarwrobel
Copy link
Contributor Author

oskarwrobel commented Jul 18, 2019

Looks like concat may be faster:

image

https://jsperf.com/concat-vs-loop-differ/1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants