Spike: Research reconversion scope

[jodator]

Provide a description of the task

We need to check what to actually do and which parts might cause problems, like:

• Conversion API updates.
• Renderer changes.

[jodator]

Check slot conversion alongside.

[jodator]

Rendering performance

The first aspect that I've wanted to tackle is performance. The way the current renderer works is to make minimal changes to the DOM to have good editing experience:

Its main responsibility is to make only the necessary, minimal changes to the DOM. However, unlike in many virtual DOM implementations, the primary reason for doing minimal changes is not the performance but ensuring that native editing features such as text composition, autocompletion, spell checking, selection's x-index are affected as little as possible.

This plays nice with atomic converters and a minimal set of changes. Which is OK in most of the aspects of the editing features. However, we already know that some (possibly small) subset of features is almost impossible to have some features written this way. (ekhm tables).

Test data

To test how costly is this re-rendering stuff I've investigated "big" table of 30 rows x 40 columns. In this table 10 first rows are headings. Both heading and body sections has two rows with every cell merged. The action is to merge those two cells from adjacent rows (in one section):

1. Merge cells in the body - atomic converters, a minimal set of changes: move content, remove a row.
2. Merge cells in heading - trigger full table re-render.

Note: From the user perspective, both actions look as similarly complex - just two cells merged.

Analyzed data:

• execute - whole time spend in MergeCellsCommand.execute() - total time of action. Anything more than 100-200ms might feel slow.
• logic - just the command logic which updates the model.
• convert - time spend in convertChanges() associated with model changes.
• render - time spent rendering view change to the DOM.

Impact on performance

Merge in body

• execute: 24.60ms
• logic: 5.52ms
• convert: 4.90ms
• render: 9.29ms

Merge in thead:

• execute: 572.31ms
• logic: 37.84ms
• convert: 119.99ms
• render: 399.03ms

The command action is perceived as sloooow. Noticeable lag, but no freeze:

• 70% of the time spent in rendering
• 20% in conversion.

Below are the flame chart of actions. As you can see there are many style recalculations (after _updateAttrs()) because renderer tries to reuse & update existing DOM nodes. Note that reusing existing DOM nodes is dumb - it just checks if there is a similar DOM node and then updates its attributes (and I think contents as well). A "smart" way would be searching for existing DOM node that is 1:1 (in the table example in a different parent).

One of many _updateAttrs() calls - 18.30ms per one call.

Every change triggers recalculate style:

Comparison to editor.setData()

The control group here is setting editor data because the action is very similar to what rendering the whole thing does.

• _handleChangeBlock: 261.56ms
• convert: 162ms
• render: 29.27ms

The render time here is curious as it is just a fraction of the time needed to render the model.change().

Possible required improvements

Re-rendering whole thing might need re-rendering the DOM completely

It looks like trying to re-use existing DOM nodes doesn't work as expected for big structural changes. There's a need for either "smart" DOM tracking or re-rendering the DOM from scratch.

Improved DOM elements re-using

A "smart" change could find existing DOM nodes (or previously rendered) elsewhere in the DOM either by:

• search (comparing might be cumbersome)
• weak view-to-dom mapping (probably not doable as refresh() does re-insert)

I don't see this feasible as we remove DOM mappings and remove the binding, also we're re-creating the view.

Re-rendering whole DOM node

To check my re-rendering hypothesis I've changed Renderer so it would always remove <figure> DOM nodes before rendering. This stops re-using existing DOM nodes (as they are removed from the DOM) and forces Renderer to create DOM structure for the <table> from scratch.

Hypothesis check

The change I've used to tests:

diff --git a/packages/ckeditor5-engine/src/view/renderer.js b/packages/ckeditor5-engine/src/view/renderer.js
index 008f7e0796..7284cfea10 100644
--- a/packages/ckeditor5-engine/src/view/renderer.js
+++ b/packages/ckeditor5-engine/src/view/renderer.js
@@ -257,6 +257,15 @@ export default class Renderer {
                        return;
                }

+               // Naive re-rendering children of every <figure> element.
+               if ( viewElement.name == 'figure' ) {
+                       const domFigure = this.domConverter.mapViewToDom( viewElement );
+                       remove( domFigure );
+                       this.domConverter.unbindDomElement( domFigure );
+
+                       return;
+               }
+
                const actualDomChildren = this.domConverter.mapViewToDom( viewElement ).childNodes;
                const expectedDomChildren = Array.from(
                        this.domConverter.viewChildrenToDom( viewElement, domElement.ownerDocument, { withChildren: false } )

Results are below:

Merge in body

Re-checked to ensure nothing worsened.

• execute: 45.03ms
• logic: 14.62ms
• converter: 11.46ms
• render: 11.30ms

Merge in thead

• execute: 285.60ms (-50%)
• logic: 55.33ms (± same)
• converter: 137.77ms (± same)
• render: 73.16ms (-80%)

A cleaner flame chart:

Comparison with deeper structure

To further check this I've tested how this would behave with bigger content. So the times below are measured for the same table structure, but every table cells have additionally 3 paragraphs with a short text and a single-item list.

Execution time for render():

• on master: 703.71ms
• using hypothesis fix: 235.19ms (-66%)

---

As for the Renderer:

• We must check if re-rendering will not cause "flickering" or selection problems - I've only validated the performance side of things.
• Enabling re-rendering "whole thing" and re-rendering associated DOM tree from scratch might be optional (how?).
• I don't see how this would work with re-rendering the whole list (but I didn't research that).

[jodator]

I had some random thoughts on how we can approach this whole re-rendering stuff:

1. Keep editing simpler than data view - table abstraction problems comment on this.
2. Research renderer possible improvements (It looks like most modern JS libs, like React have some thoughts on that).
3. Implement support for slot conversion (I'd change the name as "slot" implies multiple slots but I think that we rather think of something different here).

Ad. 2.:

• React's virtual DOM idea (link).
• React's view reconciliation (other link) and fiber concepts.
• More on React's internals: [1], [2].

Ad. 3.:

I've updated the slot conversion POC and AFAICS it works like most of the "complex" view structures. But at least in tables we only have a one slot there because of <figure> and I'm not sure how improving this might help. Anyway simpler API for converting layered structures into single model element is welcome.

[jodator]

I've tried to compare how CKEditor inspector renders view (using declarative React API) and it looks like we're on par with it (however I might mis-read what's happening there since the code is obfuscated).

• View.render() 681.96 ms
• CKEditor inspector onViewRender() : 614.75ms but that covers Redux thingy + rendering

[jodator]

After team talks around this:

So the renderer is not the only thing that might need refreshment. The other problem is that we do not have a mechanism for re-using view tree structure. The thing is that as defined by a slot conversion marking some part of view tree as a slot might help with re-rendering complex tree structures. The initial idea behind this is to research possible API that could ease marking part of a tree as "not-changed" or outside the change.

In other words

• changing structure of a complex model/view (ie. moving table rows or cells around) should not require re-rendering whole tree (ie table cells contents should be re-used).
• the API behind this should be developer friendly, complexity should be handled behind the scenes
• the view conversion (and/or DOM renderer) should optimize for such conversion
• re-rendering must be able to retain view state (view might be changed independently of the model changes - ie collapsing some boxes), or can have other complexity behind (like using <iframe> in the view).

[Reinmar]

• the API behind this should be developer friendly, complexity should be handled behind the scenes

That ❤️

I'd research it first from this angle. Let's see what we can design in terms of API and paradigms involved in the conversion. And if we'll be happy with that we can think what we can change on the internal/low level in the engine to make such changes possible.

Right now, most of the conversion API was designed bottom up – from the lowest level first to the sugar being added later. It worked in this sense that the conversion mechanism is not completely oriented around a couple end-developer cases that would came to our mind back when the whole thing was born. However, the end result is not very friendly and thus we need to reiterate now by looking from the other perspective to shape the sugar layer. We may need to change the internals too, but I think that we don't need too big changes.

[jodator]

So for a POC / spike research I've came with a complex "box" element which:

• can have multiple, different meta elements
• each meta can have different view structure
• or meta elements are stored in a meta attribute of a box as object key-values
• a box has one content element which holds many boxField elements
• each boxField is a host for any $block content (editable)

The model is quite elegant this way:

<box meta="{foo:'foo',bar:{baz: 'bar-baz'}}">
	<boxField>
		<p>Foo bar</p>
		<ul>
			<li>baz</li>
		</ul>
	</boxField>
	...
	<boxField></boxField>
</box>

But in order to properly render meta attribute change we need to re-render whole box.

Now, an elegant way would be to create an elementToElement() converter in such way that it could return view structure that can be easily mapped to an existing view. The idea behind it is to map boxField ("slots") to existing view elements and to not re-render their children but rather move them to an associated "slot".

[jodator]

In order to start working on this, I've started with implementing upcast & downcast converters to have a real-world scenario of new API usage.

At first, I've tried to use .elementToElement() but .convertChildren() fails to insert children either due to:

• it expects a parent to be present in the model
• schema is wrong and does not allow to convert $block content directly into the detached box.

0b1e50e: So I had to write full event-based converter for upcast. The downcast doesn't require me to do a similar thing as downcast conversion has model-to-view mappings. In order to re-render box on attribute or children changes I've used differ.refreshItem() API.

Additionally, I've added a downcast converter decorator that adds data-render-id to each downcasted element in order to see which items were re-rendered using CKEditorInspector.

Next steps:

• try to replace differ.refreshItem() usage with a declarative API as suggested here.
• make downcast conversion aware of slots so the view will have a minimal set of changes.
• explore if elementToElement() upcast conversion could introduce "slots" to the conversion process (similar to how downcast converter works with mappings now).

[jodator]

7d85e32: After some battle with Differ and UpcastConverter I have some initial updates to the Differ that would enable 'refresh' action (don't mind the name for now) that is capable of:

• Marking a node to be refreshed/swapped/re-rendered on attribute change.
• Differs marks those as as 'x' change in internal diff representation.
• The attribute changes on the object are removed from conversion.

I had to fix some Table post fixer as it has wrongly change type detection.

At this moment the Box feature stopped being refreshed (as expected).

Next steps:

• run insert conversion on refresh action re-using insert and try to replace existing view in a minimal way.
  • try to use existing elements mapping (the previous state was not reset as in differ.refreshItem()
• use the new refresh logic for children re-rendering on add and remove

[jodator]

7be4b06: I think that this is going in a good direction - a working example of a slot conversion with a minimal API changes:

• The view is basically the same as normal .elementToElement() converter with one change required to bind element as a slot rather as a normal element.
• The view elements under "slot" parent remains untouched (are re-used).

In the GIF below on UI action I'm changing meta attribute of the box which in turn triggers re-rendering of the whole box structure.

Notice that although main <div> with <div class="meta"> are re-rendered (seen in elements view on the right and in CKEditor inspector updated data-rendered-id attribute) the contents of the box-content-field are left untouched (same data-rendered-id on inner <p> element).

The funny thing is that my decorator doesn't catch updated on inner meta & filed elements, but maybe it is OK.

The second interesting thing is that fist attribute change updates the DOM structure (all <div>  are lighten up, but any subsequent does not touch them. The inner <p> stays the same:

[jodator]

Idea: how slot should be defined rendered?

I'm thinking where the slot re-rendering should happen? Assuming that slot-holder needs to be re-rendered but the slot itself remained the same (ie was only moved around) maybe we can also re-use that? Re-creating big view structures (tables) will be time-consuming so I still see a place for atomic converters. Also, table conversion might be simpler if it's logic would be split to separate components. So table converter would only define the layout of the view and where each "slot" (table cell) should be rendered. And after writing this it seems that I want JSX here as well. But let me write it in code.

Instead of writing downcast like this:

ckeditor5/tests/manual/article.js

Lines 144 to 147 in 9bd831f

	const viewField = writer.createContainerElement( 'div', { class: 'box-content-field' } );
	writer.insert( writer.createPositionAt( contentWrap, field.index ), viewField );
	conversionApi.mapper.bindSlotElements( field, viewField );

I'd image writing this as:

for ( const field of modelElement.getChildren() ) {
    const insertPosition = writer.createPositionAt( contentWrap, field.index )
    const slot = writer.createSlotFor( field );

    writer.insert( insertPosition, slot );
}

or even:

writer.insertSlot( field, contentWrap, field.index );
// model element	- for which model element this slot is
// view parent		- for insert position (so positionOrElement pattern here)
// offset			- for insert position

Having this would require me to implement two converters though, one for layout and second for slot element:

editor.conversion.for( 'downcast' )
    .elementToElement( { model: 'box', view: boxLayoutConverter() } )
    .elementToElement( { model: 'boxField: view: { name: 'div', class: 'box-content-field' } );

Now, having this the downcast conversion may just re-use whole view element for a "slot" based on some heuristic (ie no attribute changes or rendered element is the same as current). On second though - heuristic based on model would be better since "slot" view element might also contains "slots" (but I don't know if I want to go deeper for now) or it's view structure might be a multi-level <div>.

[jodator]

Also, table conversion might be simpler if it's logic would be split to separate components.

Doh.. I've just realised that table cell might be <td> or <th> based on it's position in parent - so it might need re-rendering (well name change only) if it get's moved to a header section.

[jodator]

Another thing to consider: binding model and view elements need to be updated as well.

In most cases we have 1:1 mapping since most features have "flat" view representation. However for multi-level views, we do:

conversionApi.mapper.bindElements( model, viewOuter ); // 1
conversionApi.mapper.bindElements( model, viewInner ); // 2

// A mapper.bindElements( modelElement, viewElement ):
this._modelToViewMapping.set( modelElement, viewElement );
this._viewToModelMapping.set( viewElement, modelElement );

Which basically will do:

1.
model -> view   (model)      -> viewOuter
view  -> model  (viewOuter)  -> model
                (viewInner)  -> undefined
2.
model -> view   (model)      -> viewInner
view  -> model  (viewOuter)  -> model
                (viewInner)  -> model

Again, this looks like a missing API and I wonder how this even work now :D From my understanding we need those mapping, among other things, to map positions.

I think that for multi-level structures we need new kind of binding, right now I'm thinking about:

conversionApi.mapper.bindElements( model, outer, [ inner ] );

This basically would allow us to properly support multi-level elements (tables, images, anything that goes into a <figure>).

[jodator]

I'm investigating triggerBy possibilty. The attribute:foo:element seems trival but child events would probably require some heuristic to trigger parent element converter.

[jodator]

aa55061: The trigger by was fairly easy to implement, probably would need internals though.

The converter definition looks like that:

ckeditor5/tests/manual/article.js

Lines 201 to 209 in aa55061

	editor.conversion.for( 'downcast' ).elementToElement( {
	model: 'box',
	view: downcastBox,
	triggerBy: [
	'attribute:meta:box',
	'insert:boxField',
	'remove:boxField'
	]
	} );

And the "slot" binding is done here:

ckeditor5/tests/manual/article.js

Lines 120 to 128 in aa55061

	for ( const field of modelElement.getChildren() ) {
	const viewField = writer.createContainerElement( 'div', { class: 'box-content-field' } );
	writer.insert( writer.createPositionAt( contentWrap, field.index ), viewField );
	conversionApi.mapper.bindSlotElements( field, viewField );
	// Might be simplified to:
	//
	// writer.defineSlot( field, viewField, field.index );

Ps.: Add this stage the conversion does not support children add/remove but I don't see potential problems there.

[jodator]

I've compiled an initial proposal for slot conversion: #6510 (comment). I'm almost sure that I've missed something. I might update it after re-reading the whole story.

[jodator]

Consider also horizontal changes (right now viertical one are considered). Ie change around list item should trigger re-converting a Range not a single element.

@Reinmar idea on this is

get list of all events to be fired and pre-process them

there was a problem with checking big inserts (ie table inside a section) by inspecting differ changes

maybe there should be a way to get non-flat list of changes

This is inline with the POC - I need to get list of events to implement triggerBy.

Potential problem in current code: the events are generated in loop from differ list. The problem is that single insert differ change may trigger events for

• the inserted element
• it's attributes
• inserted element children + it's attributes + their children...

[jodator]

Checkout Babel's JSX convertion (how to re-use).

[jodator]

TypeScript 4.0 supports custom JSX factories.

[Reinmar]

AFAIK, this is moving to the development phase, so I'm closing this spike.