Cache parent in Position

[jodator]

📝 Provide a description of the improvement

Another path that is used often during intensive model calculations is Path.parent . There was an issue about making positions immutable (reducing memory pressure) and I think that this was a good way to go.

Below is a quick comparison of changes:

@@ -148,6 +148,7 @@ export default class Position {
         * @param {Number} newOffset
         */
        set offset( newOffset ) {
+               this._parent = undefined;
                this.path[ this.path.length - 1 ] = newOffset;
        }
 
@@ -164,6 +165,10 @@ export default class Position {
         * @type {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment}
         */
        get parent() {
+               if ( this._parent ) {
+                       return this._parent;
+               }
+
                let parent = this.root;
 
                for ( let i = 0; i < this.path.length - 1; i++ ) {
@@ -193,6 +198,8 @@ export default class Position {
                        throw new CKEditorError( 'model-position-path-incorrect: The position\'s path is incorrect.', this, { position: this } );
                }
 
+               this._parent = parent;
+
                return parent;
        }

And the performance boost after this change (load "long (semantic)" and undo scenario):

Current:

After the change:

The only problem is that some algorithms modifies path (I've found one in Range class) but the Position API does not allow that - you can only modify offset. I'm confident enough that changing algorithms to operate on non-mutable positions could potentially give another boost.

OTOH I see a stretch in thinking above. I'm not sure whether we treat position as immutable or not (as a concept). We do modify offset by a setter and probably modifying path by a methods would allow to clear parent in a predictable way.

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[jodator]

Related issue about memory pressure: #4032 (comment). This might be related as we do often new Position and when we would change some algorithms those positions could be left (not cloned).

[Reinmar]

What about model structure changing while the parent is cached? I was thinking about caching the parent for a long time but it seems to be incorrect from the conceptual point of view. It may be that don't rely much on the fact that positions are detached from the model structure, but I still don't think that we could make such a change.

[Reinmar]

Another option is to look for places which access position.parent and check if they can be implemented differently, to cache the result there. For instance, there may be a local function which for some reasons access position.parent many times. It may happen that sometimes it access it directly, but many times will be indirectly (through other methods).

[Reinmar]

PS. From my tests, position.parent access is the most costly thing right now when loading more complex data, so it's totally worth looking for bigger changes here. And they don't have to be local to the Position class.

[jodator]

What about model structure changing while the parent is cached?

AFAICS the parent will not change (I might be wrong in some complex cases) - it looks like the path can change because the position's parent is moved somewhere. Modifying the position should invalidate cache (like in above example).

[Reinmar]

The parent may change – e.g. if you replace one element with another in the model.

[scofalik]

Well, the parent can change, that's true. For example: you had a position [1, 2] and now somebody added a paragraph before it. Now your path is still [1, 2] but if you cached parent, then the path will show something else than .parent.

Now when I think about it, the idea I had back then was to have only parent+offset, not both. Having both certainly is awkward. We could have had parent+offset for model but then we would have to transform it to paths when sending through network and still operate on paths when receving and doing OT.

BTW. We could have paths and parent+offset for LivePositions, though. However, this is not the case here, they are very rarely used.

[Reinmar]

OK, but this deserves its own "Rewrite half of the engine" ticket :P 

I'm closing this one as caching the parent is not possible, unless we'll introduce a special type of "cached positions" in which the cache is cleared on any model change. But such positions would need to be destroyed manually so they will be as inconvenient as live positions and would only make sense in very specific cases.