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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
36a629a
Inline RootElement.is() calls to parent class and remove model: check.
jodator Apr 1, 2020
2e21618
Inline is() checks in view and model elements.
jodator Apr 1, 2020
e9071aa
Restore logic for model: check in Element#is() and RootElement#is().
jodator Apr 2, 2020
4d6669c
Inline is() checks in view elements.
jodator Apr 3, 2020
13bdf60
Update missing checks in model Text#is checks.
jodator Apr 3, 2020
b99b7a0
Add info about not calling super in is() methods.
jodator Apr 3, 2020
4e8b70c
Merge branch 'master' into i/6529
Reinmar Apr 8, 2020
029e8b2
Use tripple equal for consistency.
Reinmar Apr 8, 2020
19cd270
Aligned the last super.is() use to the changes in this PR.
Reinmar Apr 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/model/documentfragment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ export default class DocumentFragment {
* @returns {Boolean}
*/
is( type ) {
return type == 'documentFragment' || type == 'model:documentFragment';
return type === 'documentFragment' || type === 'model:documentFragment';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/model/documentselection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ export default class DocumentSelection {
* @returns {Boolean}
*/
is( type ) {
return type == 'selection' ||
return type === 'selection' ||
type == 'model:selection' ||
type == 'documentSelection' ||
type == 'model:documentSelection';
Expand Down
11 changes: 6 additions & 5 deletions src/model/element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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 •
edited
Loading

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

type === this.name || type === 'model:' + this.name ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'node' || type === 'model:node';
}

return name === this.name && ( type === 'element' || type === 'model:element' );
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/model/liveposition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type == 'position' || type === 'model:position';
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/model/liverange.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ export default class LiveRange extends Range {
* @returns {Boolean}
*/
is( type ) {
return type == 'liveRange' || type == 'model:liveRange' || super.is( type );
return type === 'liveRange' || type === 'model:liveRange' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type == 'range' || type === 'model:range';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/model/markercollection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ class Marker {
* @returns {Boolean}
*/
is( type ) {
return type == 'marker' || type == 'model:marker';
return type === 'marker' || type === 'model:marker';
}

/**
Expand Down
11 changes: 1 addition & 10 deletions src/model/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays';

// To check if component is loaded more than once.
import '@ckeditor/ckeditor5-utils/src/version';

Expand Down Expand Up @@ -432,7 +431,7 @@ export default class Node {
* @returns {Boolean}
*/
is( type ) {
return type == 'node' || type == 'model:node';
return type === 'node' || type === 'model:node';
}

/**
Expand Down Expand Up @@ -500,11 +499,3 @@ export default class Node {
this._attrs.clear();
}
}

/**
* The node's parent does not contain this node.
*
* This error may be thrown from corrupted trees.
*
* @error model-node-not-found-in-parent
*/
2 changes: 1 addition & 1 deletion src/model/position.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ export default class Position {
* @returns {Boolean}
*/
is( type ) {
return type == 'position' || type == 'model:position';
return type === 'position' || type === 'model:position';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/model/range.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ export default class Range {
* @returns {Boolean}
*/
is( type ) {
return type == 'range' || type == 'model:range';
return type === 'range' || type === 'model:range';
}

/**
Expand Down
16 changes: 12 additions & 4 deletions src/model/rootelement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,19 @@ export default class RootElement extends Element {
* @returns {Boolean}
*/
is( type, name ) {
const cutType = type.replace( 'model:', '' );
if ( !name ) {
return cutType == 'rootElement' || super.is( type );
} else {
return ( cutType == 'rootElement' && name == this.name ) || super.is( type, name );
return type === 'rootElement' || type === 'model:rootElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'element' || type === 'model:element' ||
type === this.name || type === 'model:' + this.name ||
type === 'node' || type === 'model:node';
}

return name === this.name && (
type === 'rootElement' || type === 'model:rootElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'element' || type === 'model:element'
);
}

/**
Expand All @@ -105,3 +112,4 @@ export default class RootElement extends Element {
// @if CK_DEBUG_ENGINE // console.log( 'ModelRootElement: ' + this );
// @if CK_DEBUG_ENGINE // }
}

2 changes: 1 addition & 1 deletion src/model/selection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ export default class Selection {
* @returns {Boolean}
*/
is( type ) {
return type == 'selection' || type == 'model:selection';
return type === 'selection' || type === 'model:selection';
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/model/text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,9 @@ export default class Text extends Node {
* @returns {Boolean}
*/
is( type ) {
return type == 'text' || type == 'model:text' || super.is( type );
return type === 'text' || type === 'model:text' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'node' || type === 'model:node';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/model/textproxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ export default class TextProxy {
* @returns {Boolean}
*/
is( type ) {
return type == 'textProxy' || type == 'model:textProxy';
return type === 'textProxy' || type === 'model:textProxy';
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/model/writer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1360,14 +1360,14 @@ export default class Writer {
const markerRange = marker.getRange();
let isAffected = false;

if ( type == 'move' ) {
if ( type === 'move' ) {
isAffected =
positionOrRange.containsPosition( markerRange.start ) ||
positionOrRange.start.isEqual( markerRange.start ) ||
positionOrRange.containsPosition( markerRange.end ) ||
positionOrRange.end.isEqual( markerRange.end );
} else {
// if type == 'merge'.
// if type === 'merge'.
const elementBefore = positionOrRange.nodeBefore;
const elementAfter = positionOrRange.nodeAfter;

Expand Down
14 changes: 10 additions & 4 deletions src/view/attributeelement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,12 +157,18 @@ export default class AttributeElement extends Element {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type && type.replace( /^view:/, '' );

if ( !name ) {
return cutType == 'attributeElement' || super.is( type );
return type === 'attributeElement' || type === 'view:attributeElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === this.name || type === 'view:' + this.name ||
type === 'element' || type === 'view:element' ||
type === 'node' || type === 'view:node';
} else {
return ( cutType == 'attributeElement' && name == this.name ) || super.is( type, name );
return name === this.name && (
type === 'attributeElement' || type === 'view:attributeElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'element' || type === 'view:element'
);
}
}

Expand Down
13 changes: 10 additions & 3 deletions src/view/containerelement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,18 @@ export default class ContainerElement extends Element {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type && type.replace( /^view:/, '' );
if ( !name ) {
return cutType == 'containerElement' || super.is( type );
return type === 'containerElement' || type === 'view:containerElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === this.name || type === 'view:' + this.name ||
type === 'element' || type === 'view:element' ||
type === 'node' || type === 'view:node';
} else {
return ( cutType == 'containerElement' && name == this.name ) || super.is( type, name );
return name === this.name && (
type === 'containerElement' || type === 'view:containerElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'element' || type === 'view:element'
);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/view/documentfragment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export default class DocumentFragment {
* @returns {Boolean}
*/
is( type ) {
return type == 'documentFragment' || type == 'view:documentFragment';
return type === 'documentFragment' || type === 'view:documentFragment';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/view/documentselection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ export default class DocumentSelection {
* @returns {Boolean}
*/
is( type ) {
return type == 'selection' ||
return type === 'selection' ||
type == 'documentSelection' ||
type == 'view:selection' ||
type == 'view:documentSelection';
Expand Down
15 changes: 12 additions & 3 deletions src/view/editableelement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,20 @@ export default class EditableElement extends ContainerElement {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type && type.replace( /^view:/, '' );
if ( !name ) {
return cutType == 'editableElement' || super.is( type );
return type === 'editableElement' || type === 'view:editableElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'containerElement' || type === 'view:containerElement' ||
type === this.name || type === 'view:' + this.name ||
type === 'element' || type === 'view:element' ||
type === 'node' || type === 'view:node';
} else {
return ( cutType == 'editableElement' && name == this.name ) || super.is( type, name );
return name === this.name && (
type === 'editableElement' || type === 'view:editableElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'containerElement' || type === 'view:containerElement' ||
type === 'element' || type === 'view:element'
);
}
}

Expand Down
8 changes: 5 additions & 3 deletions src/view/element.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,13 @@ export default class Element extends Node {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type.replace( /^view:/, '' );
if ( !name ) {
return cutType == 'element' || cutType == this.name || super.is( type );
return type === this.name || type === 'view:' + this.name ||
type === 'element' || type === 'view:element' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'node' || type === 'view:node';
} else {
return cutType == 'element' && name == this.name;
return name === this.name && ( type === 'element' || type === 'view:element' );
}
}

Expand Down
12 changes: 9 additions & 3 deletions src/view/emptyelement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,17 @@ export default class EmptyElement extends Element {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type.replace( /^view:/, '' );
if ( !name ) {
return cutType == 'emptyElement' || super.is( type );
return type === 'emptyElement' || type === 'view:emptyElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === this.name || type === 'view:' + this.name ||
type === 'element' || type === 'view:element' ||
type === 'node' || type === 'view:node';
} else {
return ( cutType == 'emptyElement' && name == this.name ) || super.is( type, name );
return name === this.name && (
type === 'emptyElement' || type === 'view:emptyElement' ||
type === 'element' || type === 'view:element'
);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/view/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ export default class Node {
* @returns {Boolean}
*/
is( type ) {
return type == 'node' || type == 'view:node';
return type === 'node' || type === 'view:node';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/view/position.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ export default class Position {
* @returns {Boolean}
*/
is( type ) {
return type == 'position' || type == 'view:position';
return type === 'position' || type === 'view:position';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/view/range.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ export default class Range {
* @returns {Boolean}
*/
is( type ) {
return type == 'range' || type == 'view:range';
return type === 'range' || type === 'view:range';
}

/**
Expand Down
17 changes: 14 additions & 3 deletions src/view/rooteditableelement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,22 @@ export default class RootEditableElement extends EditableElement {
* @returns {Boolean}
*/
is( type, name = null ) {
const cutType = type.replace( /^view:/, '' );
if ( !name ) {
return cutType == 'rootElement' || super.is( type );
return type === 'rootElement' || type === 'view:rootElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'editableElement' || type === 'view:editableElement' ||
type === 'containerElement' || type === 'view:containerElement' ||
type === this.name || type === 'view:' + this.name ||
type === 'element' || type === 'view:element' ||
type === 'node' || type === 'view:node';
} else {
return ( cutType == 'rootElement' && name == this.name ) || super.is( type, name );
return name === this.name && (
type === 'rootElement' || type === 'view:rootElement' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'editableElement' || type === 'view:editableElement' ||
type === 'containerElement' || type === 'view:containerElement' ||
type === 'element' || type === 'view:element'
);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/view/selection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,7 @@ export default class Selection {
* @returns {Boolean}
*/
is( type ) {
return type == 'selection' || type == 'view:selection';
return type === 'selection' || type === 'view:selection';
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/view/text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ export default class Text extends Node {
* @returns {Boolean}
*/
is( type ) {
return type == 'text' || type == 'view:text' || super.is( type );
return type === 'text' || type === 'view:text' ||
// From super.is(). This is highly utilised method and cannot call super. See ckeditor/ckeditor5#6529.
type === 'node' || type === 'view:node';
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/view/textproxy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ export default class TextProxy {
* @returns {Boolean}
*/
is( type ) {
return type == 'textProxy' || type == 'view:textProxy';
return type === 'textProxy' || type === 'view:textProxy';
}

/**
Expand Down
Loading