Toolbar grouping should recalculate the styles less frequently

[mlewand]

📝 Provide detailed reproduction steps (if any)

Currently toolbar recalculates the styles after adding each toolbar item.

It would be good to optimize it, and ideally it should do it only once.

Style recalculations make for 7.4% of total _initToolbar function time (in an editor with small semantic content) and this function makes a significant contribution to editor load time today.

Here is a devtools profiler dump that can be viewed later on: editor-init-small.zip.

✔️ Expected result

Less style recalculations 🏃‍♀️

❌ Actual result

Styles are recalculated after adding each toolbar item 🐢 🐢 🐢 🐢 🐢 🐢 🐢 🐢 🐢

📃 Other details

This is a follow-up of #5880.

• Browser: Chrome
• OS: Windows (should be cross system)
• CKEditor version: latest master

---

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

[mlewand]

Reducing _updateGrouping() call count alone will not cut it.

I made a quick and a dirty demo (i/6194-workaround) to check what are the benefits of reducing _updateGrouping call count. And reducing call count alone will not bring significant improvements, but first some data:

Rich editor initialization

(note, in order to do that you need to remove shouldNotGroupWhenFull config property first)

This editor contains a lot of icons so the difference will be the biggest. Total time show is relative to Evaluate Script.

The master branch

i/6194-workaround branch

The impact is reduced almost twice.

Editor initialization

master branch

i/6914 branch

As expected, for a smaller toolbar the improvement is less significant.

Why so little gain?

I didn't dive deep into DynamicGrouping class internals yet, but from a greater picture I see that we're doing DOM changes that are trashing the layout and it has to be re-rendered for each overflow request.

Instead it should do operations in batches, deferring DOM modifications for as long as possible.

Ideally we should also use smarter algorithm for checking whether all DOM elements fits the toolbar. E.g. check the last firsts if it doesn't then go with bisect-like algorithm, so jump into the middle -> check -> jump to a proper half based on previous result.

Finally Collection API doesn't play well here, as it only allows you to add one by one item, and only notifies about this. It would be easier if the collection could do batch changes.

[Reinmar]

My question would be – do you think you can have a meaningful improvement (something above 50ms for a typical editor setup) in max 2 days of work?

[mlewand]

Now, for a reference here's a closer look at the execution timing on i/6194 branch in case of a typical editor initialization, when no toolbar grouping occur.

Almost entire total time of _updateGrouping() comes from recalculating styles and layout event.

As of what we can do in limited time:

• We can get around this problem by adding a way to disable DynamicGrouing behavior. Simple and quick, although will degrade the API a bit.
• We could make sure that toolbar's items collection is initialized once (no multiple .add calls). There's a PR which introduces a possibility to initialize collection though the constructor. From there we'd need to either change the way fillFromConfig handles items property, or add a different method that initializes props rather than add them.

I see that we're doing DOM changes that are trashing the layout and it has to be re-rendered for each overflow request.

Unfortunately optimizing DynamicView rendering to reduce DOM modifications will be costly, as the heuristics handled by DynamicView are not trivial (it has to account not only for buttons, but also the grouping button as well which might or might not be available; it both adds and removes buttons depending on state etc.). So it's more like week of time work.

However reducing number of _updateGrouping() calls will be quick and easy.

[Reinmar]

• We can get around this problem by adding a way to disable DynamicGrouing behavior. Simple and quick, although will degrade the API a bit.

If you mean disabling grouping while the loop inside fillFromConfig() works, I'm 👍. Simple, local and we can always change it to something better.

To sum up – it reduces the time _updateGrouping() takes from 10.8% to 8.1%, yes?

[oleq]

If you mean disabling grouping while the loop inside fillFromConfig() works, I'm 👍. Simple, local and we can always change it to something better.

The right thing to do (without increasing the tech debt) is:

1. Resurrect I/4992: Added support for creating a Collection with initial items ckeditor5-utils#309
2. Remove changes made to Collection#add(), only leave changes made to the constructor() allowing passing the initial content of the collection.
3. Change toolbarView.fillFromConfig()->static ToolbarView.createItemsFromCofig() (that will return now an array of Views)
4. Pass the output of ToolbarView.createItemsFromCofig() in the ClassicEditor to the constructor of ClassicEditorUIView
5. Let ClassicEditorUIView pass items through to its toolbar.
6. Toolbar will take actual views as an option and pass them when creating ToolbarView.items collection.

Voilá. No avalanche of collection events and stuff when creating the toolbar. Which means no subsequent _updateGrouping(), only called once.

Additional profit: because the Collection (also ViewCollection) supports initial items we can save a lot of time when e.g. creating the table insertion grid (create all Views, then pass them all at once to the collection -> 1 event instead of 100). Same with special character grids etc.

[mlewand]

To sum up – it reduces the time _updateGrouping() takes from 10.8% to 8.1%, yes?

Yes, in this particular case yes.

Additional profit: because the Collection (also ViewCollection) supports initial (...)

I did some testing to verify this claim, and I can confirm that we'll see some gains in this area for table insertion dropdown. Results are not ground breaking, but it's clearly more optimal now.

Tests performed using paste performance manual test by full refresh + clicking the table toolbar button.

First 3 tests done on the latest master branch, the latter three performed on i/6194 branch.

Branch #run	InsertTableView total time	% of InsertTableView in total click handling time
master run 1	127.5ms	64.3%
master run 2	102.9ms	77.9%
master run 3	112.7ms	75.1%
i/6194 run 1	82.0ms	51.8%
i/6194 run 2	93.9ms	50.2%
i/6194 run 3	76.8ms	48.4%

master run

Optimized run

Bear in mind that profiling was performed on a development desktop with some apps (mail client, music, IDE etc) running in the background, so the % share of InsertTableView in entire click handling time is more relevant than execution time expressed in ms.

Try it for yourself

An archive containing each run can be found here.

Is it worth it?

IMHO it makes sense to introduce a possibility to init collections at a construction time (I was already missing this a couple of times, even though I'm not working on CKE5 for that long) rather than creating a new feature in DynamicGrouping.

This means we're adding code that will be reused in many other places, which will benefit us more in a long run and lower our technical debt.

I'll push the PR tomorrow once we agree on which direction we go.

[Reinmar]

LGTM 👍 Thanks for verifying this.

[mlewand]

Moving to iteration 31 as this issue is blocked by #6319.

[mlewand]

• Pass the output of ToolbarView.createItemsFromCofig() in the ClassicEditor to the constructor of ClassicEditorUIView

There's a problem with this, our new createItemsFromConfig() function requires a component factory, just like current fillFromConfig() does.

Unfortunately ComponentFactory is created by EditorUi subclass instance, which is not available at the time of EditorUiView subclass instance creation (see that editor UI is created after it, because it requires view in the constructor).

So this way we're hitting a cyclic dependency here. I'll investigate further what are the options to get around this.

[mlewand]

Talked with @oleq today, and some ideas here:

A brute-force solution might be to pull out ComponentFactory component creation into the ClassicEditor constructor, where it would pass it into both ClassicEditorUI and ClassicEditorUIView.

Another solution is to create UIView after ClassicEditorUI, and initialize this.ui.view after UIView is created (sort of initialize ClassicEditorUI in two steps),

[oleq]

const factory = new ComponentFactory( editor );

const view = new ClassicEditorUIView( this.locale, this.editing.view, factory, {
    shouldToolbarGroupWhenFull
} );

this.ui = new ClassicEditorUI( this, view, factory );

or

this.ui = new ClassicEditorUI( this );
this.ui.view = new ClassicEditorUIView( this.locale, this.editing.view, this.ui.componentFactory, {
    shouldToolbarGroupWhenFull
} );

[mlewand]

I have implemented our conception outlined in previous comments (#6194 (comment), #6194 (comment), #6194 (comment)) on the i/6194 branch.

The problem is that at a time of initialization of ClassicEditorUI/ClassicEditorUiView constructors components factory doesn't have any components registered yet.

So the toolbar doesn't know how to create any button, which results with:

toolbarview.js:323 toolbarview-item-unavailable: The requested toolbar item is unavailable. Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-toolbarview-item-unavailable
 {name: "heading"}

(...)

toolbarview.js:323 toolbarview-item-unavailable: The requested toolbar item is unavailable. Read more: https://ckeditor.com/docs/ckeditor5/latest/framework/guides/support/error-codes.html#error-toolbarview-item-unavailable
 {name: "redo"}

all-features.js:134 TypeError: Cannot read property 'toolbarItems' of undefined
    at new ToolbarView (toolbarview.js:85)
    at addToolbarToDropdown (utils.js:134)
    at DynamicGrouping._createGroupedItemsDropdown (toolbarview.js:828)
    at new DynamicGrouping (toolbarview.js:529)
    at new ToolbarView (toolbarview.js:225)
    at new ClassicEditorUIView (classiceditoruiview.js:54)
    at new ClassicEditor (classiceditor.js:78)
    at classiceditor.js:200
    at new Promise (<anonymous>)
    at Function.create (classiceditor.js:199)

This is a second problem that surfaces only after implementation (first one was with component factory that has to be moved around).

Collection.batchAdd approach

Another idea that I had at the beginning was to adjust the Collection class, so that it supports batch adding.

The benefits of proposed change:

• dead simple
• doesn't introduce breaking changes in the API (for classes ClassicEditorUI and ClassicEditorUIView)
• small diff

I have created a i/6194-collection branch.

If we are to go with it we need complementary batchRemove.

I made some benchmarks:

sample/test case	_initToolbar before	_initToolbar after	relative before	relative after
editorinit (short, semantic content)	452ms	402ms (-50ms, -11%)	46.8% of entire click handling time	42.8% of entire click handling time
all features	898ms	738ms (-160ms, -17.8%)	Browser splits initialization into two tasks, comparison would be misleading.	N/A

You can explore the above performance results yourself: collection-batch-add.zip

Before

After

requestAnimationFrame

Another tossed idea was to use requestAnimationFrame. At this point I'm not sure whether it would work.

I imagine that in _updateGrouping() method we'd neet to add an rAF (request animation frame) listener to do the grouping upon layout recalc. We'd also need to make sure that no multiple pending rAF listeners are added (beacause it doesn't make sense to perform multiple _updateGrouping subsequently).

[Reinmar]

I have a feeling that the first option (collection's extension) does not really address the general issue that we have with the UI – that during its initialization we do not control when some things related to the UI happen, so they happen at random times creating many reflows. That's a general issue that we may need to address one day.

However, addressing it may require a major design effort, so most likely this is not the right moment to do that. And while I initially thought that rAF would be a part of the general solution, I'm having second doubts now.

So, if the addBatch/removeBatch work for you and the results prove that, I don't see an issue with that approach for now.