Ckeditor5: Improve performance of attribute removal

[mlewand]

While working on remove formatting (ckeditor/ckeditor5-remove-format#7) we found that removing attributes from relatively simply code takes too much time.

For repro case see ckeditor/ckeditor5-remove-format#7.

[Reinmar]

How much content did you test?

Because I tested removing styles (quite a lot of them – I used a duplicated content from the remove-formatting.md docs page) from 50 paragraphs:

It took 225ms to remove all the styles. I don't think this is a problem because those 50 paragraphs represent are not a short piece:

[Reinmar]

OK, I can see that you used the content from https://github.com/ckeditor/ckeditor5-remove-format/issues/7.

Sounds for me like an edge case. This content is causing some turbo deopts, but it does not look like a real content. We definitely have a problem, but I'd check with some real-life scenarios (copy-paste from other websites/word/etc) and prioritise it based on the results from these tests.

[Reinmar]

Or maybe this is actually a pretty realistic scenario...

[scofalik]

The question would be whether it's simply a lot of operations to make or if attribute operations are slower than other.

This could be affected by text nodes splitting and joining after each operation.

[Reinmar]

OK, I found the main culprit. It was actually quite funny. I reduced the execution time from 34s to 1.5s which still isn't great, but it's much better.

BEFORE:

AFTER:

The issue was this bit of code in ImageLoadObserver:

        viewRoot.on( 'change:children', ( evt, node ) => {
            // Wait for the render to be sure that `<img/>` elements are rendered in the DOM root.
            this.view.once( 'render', () => this._updateObservedElements( domRoot, node ) );
        } );

Why is it a problem? It's actually 3 problems:

1. We add a listener on every child change – and in this case, we have thousands of these.
2. We use once() so on every #render we now need to remove those thousands callbacks. This isn't cheap too.
3. We call the _updateObservedElements() function thousands times too.

We need to rewrite this observer completely. The first thing that came to my mind was – can we use event capturing for image#load?

The next thing to look into would be _getRawTag() (which is the most significant position in the "self time" category) which is actually a lodash's function so there's a big chance we don't need to call it at all. Most likely we use some overcomplicated (for our case) lodash function instead of doing something simple with our own code.

[jodator]

I've found out that the longer the text is in single paragraph the longer the method take. This is counterintuitive to me as I's suspect the same number of operations - but maybe there's something more going on which is dependent on the text length not the number of attributes.

[scofalik]

The same happens the other way around too -- when you add multiple attributes at the same time. This is a case in track changes. I debugged it and got a very similar performance chart.

[Reinmar]

Unfortunately, https://github.com/ckeditor/ckeditor5-image/pull/335 does not resolve this issue completely. The performance is still fundamentally wrong.

I did some profiling and out of 8s that the whole thing takes, 3.3s are spent in parseAttributes() in the view element constructor. That'd be the first thing to check because we can gain the most here. But it'd still not be all.

Here are stats of view element constructor calls, split per element name. In the arrays I stored the attributes param of the constructor:

As you can see, in summary, the constructor is called 70k times 🤯

👉 70.000 times 👈

Most of the time, in this case, with undefined as attributes. Hence... perhaps we can add a condition for it.

[Reinmar]

Most of the time, in this case, with undefined as attributes. Hence... perhaps we can add a condition for it.

Nope. The number of times we call this with undefined is actually around 500 compared to 70k with some empty map/object. We could still find ways to optimize this locally but I think that we should figure out why that many elements are created and whether we can prevent that.

[Reinmar]

I closed https://github.com/ckeditor/ckeditor5-image/pull/335 but I think we should keep this issue opened. I think that a reasonable time for this content is around 1s. So there's still 80% to go.

[Reinmar]

I extracted the image load observer to https://github.com/ckeditor/ckeditor5/issues/5766 and I'd remove this ticket from the iteration as there's more to do here.

[Reinmar]

I was testing loading a quite long content (50 pages) into the editor and I can also see the getRawTag there:

Interesting if it would be possible to optimise that bit 🤔

[Reinmar]

In this case, there are 30k model nodes (10k elements and 20k text nodes) created, even though the final document has 3k nodes. Again, we're executing toMap() just too many times.

[Reinmar]

Okeydokey – I was able to take toMap()'s execution time from something around 20-30% of the total task's time to close to 0%. Details in https://github.com/ckeditor/ckeditor5/issues/5854.

However, we should still check if we can save some time here. Unfortunately, in both cases (remove format and data loading) this will not be a micro-optimization any more (unless someone knows how to shove some time off the Position#parent getter). This time we have to figure out:

• how to do fewer things (e.g. create fewer elements),
• how to not do some things at all.

[Reinmar]

Actually, I'm wrong, there's still more to do in here (will affect only remove formatting case):

This is parseAttributes() from view element and it also does the isPlainObject() thing. We can change the condition there too.

[jodator]

@Reinmar would you mind creating a manual sample similar to the ones I've created for the memory leak tests?

• It might be just one editor build with some complex content to parse.
• to make tests easier it could initialize the editor on a button (to setup performance test in dev tools)
• it should have as many features (in the editor and in loaded content) to test in some full setup
• it could also load the same content to different setups (like current Classic and some with many features enabled) so one button will tests subset of features from the content the other would test all of them.

This would help to manually test various improvements and compare results in the team.

[Reinmar]

As I commented in https://github.com/ckeditor/ckeditor5/issues/2336#issuecomment-623907622. We reduced the time over 10x. NICE :)