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

Review most used code in terms of performance #6528

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

Review most used code in terms of performance #6528

jodator opened this issue Apr 1, 2020 · 6 comments · Fixed by ckeditor/ckeditor5-engine#1835
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

Doing the MergeCells command PR review @oleq found out that undoing merge of large selection is pretty slow.

It looks like some significant time is spent in code that is not suspicious at a first glance. Some examples are (from initial report):

  1. Element's is() comparison is doing string.replace() for model: prefix checking. Adding many checks up this is significant time.
  2. Position's constructor is doing Array.concat() that has some performance issues. Changing it to spread operator (as a wild guess - it hasn't that performance boost in synthetic tests) also cut some time.

I see a potential quick win (spending additional 1-2 hour) in reviewing some repeatable code and checking if it has some potential fixes (ie those that will not change logic or API).


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 jodator added this to the iteration 31 milestone Apr 1, 2020
@jodator jodator self-assigned this Apr 1, 2020
@jodator
Copy link
Contributor Author

jodator commented Apr 1, 2020

Repeatable result:

  1. Testing done on "Performance: pasting data" manual test using "full websites" setting.
  2. Test: invoke "full website" -> click to focus -> CTRL-Z (undo).

Result: 466ms to 87.4ms for the line of concatenating parent path (same time reduction in undo execution).

after:

@jodator
Copy link
Contributor Author

jodator commented Apr 1, 2020

And another gain by using for loops instead of spread operator: from 56.6ms self time to 3.5ms self time (I had to extract those to methods to compare this part of code).

after:

Side note: because of the time fluctuation I'd only consider an order of magnitude change (as above) to be significant enough.

@Reinmar
Copy link
Member

Reinmar commented Apr 1, 2020

Waiting for the PRs... 🛎

@jodator
Copy link
Contributor Author

jodator commented Apr 1, 2020

Another finding - the is() method is painfully slow because:

  1. It does string operations 
  2. Calls super...

Below is a result RootElement.is() times:

Before:

After (removing string concatenation and do not call super.is():

@jodator
Copy link
Contributor Author

jodator commented Apr 1, 2020

I've double checked the latter finding and this is bonkers:

Current code:

After changes only in RootElment:

The changes:

is( type, name ) {
	if ( !name ) {
		return type == 'rootElement' || type == 'element' || type == this.name || type == 'node';
	} else {
		return ( type == 'rootElement' && name == this.name ) || ( type === 'element' && name == this.name );
	}
}

@jodator
Copy link
Contributor Author

jodator commented Apr 1, 2020

I've inlined most of the is() checks and I've removed that mode: and view: checks. Gain 10s in the above scenario.

Before (undo callback took 25.1s ):

After (undo callback took 14.89s):

Reinmar added a commit to ckeditor/ckeditor5-engine that referenced this issue Apr 14, 2020
Other: Improved the performance of the `Position` constructor by optimizing for the fast path where the root element is passed as the position root. Closes ckeditor/ckeditor5#6528.
mlewand pushed a commit that referenced this issue May 1, 2020
Other: Improved the performance of the `Position` constructor by optimizing for the fast path where the root element is passed as the position root. Closes #6528.
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.

2 participants