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

I/6529: Inline is() checks #1836

Merged
merged 9 commits into from
Apr 8, 2020
Merged

I/6529: Inline is() checks #1836

merged 9 commits into from
Apr 8, 2020

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Apr 3, 2020

Suggested merge commit message (convention)

Other: Inline calls to parent class in models and views is() checks to improve editor performance. Closes ckeditor/ckeditor5#6529.


Additional information

That's a lot of changes but fortunately tests were very helpful here.

The API remained the same.

@jodator jodator requested a review from Reinmar April 3, 2020 08:19
@coveralls
Copy link

coveralls commented Apr 3, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 19cd270 on i/6529 into e74abf7 on master.

@@ -80,7 +80,9 @@ export default class LivePosition extends Position {
* @returns {Boolean}
*/
is( type ) {
return type == 'livePosition' || type == 'model:livePosition' || super.is( type );
return type == 'livePosition' || type == 'model:livePosition' ||
Copy link
Member

Choose a reason for hiding this comment

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

I can see that you were replacing == with === in some places, so let's be consistent and have it in all is() methods.

Copy link
Member

Choose a reason for hiding this comment

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

Still not resolved.

Copy link
Member

Choose a reason for hiding this comment

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

I'll handle that, though.

return cutType == 'element' || cutType == this.name || super.is( type );
} else {
return cutType == 'element' && name == this.name;
return type === 'element' || type === 'model:element' ||
Copy link
Member

Choose a reason for hiding this comment

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

One more optimization that you could consider is ordering those conditions in the likelihood of them being executed. You may need to do some stats and it's a subject of change, so we may need to repeat this. But if it yields nice results, it may be worth doing.

Copy link
Contributor Author

@jodator jodator Apr 3, 2020

Choose a reason for hiding this comment

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

OK, I'll check this.

Side note I've done some quick (I don't have the results saved though) check for the fastest 'false' path. I've counted the checks in the RootElement checks and one of the most used was is( 'text' ) . But adding if( type === 'text' ) return false (fail fast) turned out to be slower than proposed solution in PR. It might be some browser optimization or a simple error in my methodology but this proves that intuition must be followed with tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that this will yield any significant result. I've done some tests and below are number of executions of some is checks in the long semanting + undo scenario:

As you can see there are milions of checks using is() methods and most of them checks if element is text or element. But if you check how much time is spent (self-time) in is methods then in the above scenario (~20 seconds of undo execution) they add up to maybe 150 - 200ms:

I don't see how we could optimize those paths reliably. At this stage I would try to

@@ -116,13 +116,14 @@ export default class Element extends Node {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type.replace( /^model:/, '' );

if ( !name ) {
Copy link
Member

Choose a reason for hiding this comment

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

When we talk about micro optimizations name === null may be faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something doesn't work (maybe undefined is passed sometimes).  Tests breaks after suggested change. Looking that this might be to micro optimization to care (looking on above times) I think we shouldn't dig into this more at this time.

@Reinmar
Copy link
Member

Reinmar commented Apr 3, 2020

👍  for the changes. I left two more ideas what could be checked, since we're reviewing this code anyway.

@Reinmar
Copy link
Member

Reinmar commented Apr 3, 2020

BTW, another change that I wanted to do for some time: ckeditor/ckeditor5#6547. It can actually be tuned with the order of conditions in is() methods.

@jodator
Copy link
Contributor Author

jodator commented Apr 6, 2020

@Reinmar IMO it is ready to be merged now.

@jodator
Copy link
Contributor Author

jodator commented Apr 6, 2020

BTW, another change that I wanted to do for some time: ckeditor/ckeditor5#6547. It can actually be tuned with the order of conditions in is() methods.

Let's do this in ckeditor/ckeditor5#6547 (the order check). Maybe other problems will arise and other fixes will be needed. IE simple flags or direct 'type' check.

@Reinmar Reinmar merged commit ff04509 into master Apr 8, 2020
@Reinmar Reinmar deleted the i/6529 branch April 8, 2020 12:52
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.

Remove model / view check from is() checks.
3 participants