Ckeditor5: Improve performance of toMap() && parseAttributes()

[Reinmar]

EDIT: I extended this ticket to cover not only the toMap() implementation in model/Node, but also parseAttributes() in view/Element which has the same issue (IIRC).

---

This function is called extremely frequently and becomes the most time-consuming thing in https://github.com/ckeditor/ckeditor5/issues/4504 and setting editor data with complex content.

99% of time inside it is spent on isPlainObject(). My guess is that it's so slow because it uses something like Object.prototype.toString.apply( obj ) == '...' to check what we've got inside and perhaps cover some other cases that we don't really need in toMap().

According to the docs, toMap() should work with objects or iterables. I'd actually check with what kind of objects it's really executed because I also expect to see maps and arrays (so, iterables too), but we should make sure we're not breaking it for them.

One thing I noticed is that toMap()'s tests don't actually cover the iterables case 🤦‍♂ . I was able to write an implementation that passes all tests but breaks in the engine.

So, this is perhaps more or less what we need:

export default function toMap( data ) {
    if ( !data ) {
        return new Map();
    }

        // toMap() tests pass without this but break in the engine.
    if ( typeof data[ Symbol.iterator ] === 'function' ) {
        return new Map( data );
    }

        // The two following conditions are not necessary since both arrays and maps are iterable.
        // I just want to show that toMap()'s tests are insufficient if these 2 conditions make them green.
    if ( data instanceof Array ) {
        return new Map( data );
    }

    if ( data instanceof Map ) {
        return new Map( data );
    }

    return objectToMap( data );
}

This allowed me to get from 8s to 6s when loading complex data and from 1843ms to 693ms when removing attributes from content taken from https://github.com/ckeditor/ckeditor5-remove-format/issues/7.

[Reinmar]

cc @scofalik @jodator

[mlewand]

@Reinmar I faced the very same thing when testing further tweaks for #4504. Also you can reuse our isIterable util function.

Another relatively expensive but easy to improve (performance-wise) bit was prefix matching using a regexp (which happens in many different view and model types). You can check prefix by simply using String.startsWith(), which is simpler and faster.

[jodator]

I'm trying to understand why/where toMap() is used and in some places it looks like Object.entries():

https://github.com/ckeditor/ckeditor5-engine/blob/0a7597faf59b84afe249e6167dface72f6af43aa/src/model/writer.js#L387:

/**
 * Sets values of attributes on a {@link module:engine/model/item~Item model item}
 * or on a {@link module:engine/model/range~Range range}.
 *
 *      writer.setAttributes( {
 *          bold: true,
 *          italic: true
 *      }, range );
 *
 * @param {Object} attributes Attributes keys and values.
 * @param {module:engine/model/item~Item|module:engine/model/range~Range} itemOrRange
 * Model item or range on which the attributes will be set.
 */
setAttributes( attributes, itemOrRange ) {
    for ( const [ key, val ] of toMap( attributes ) ) {
        this.setAttribute( key, val, itemOrRange );
    }
}

After changing this to Object.entries() the only tests in the engine that failed were tests that check if that method works with new Map() - but this is not what the jsdoc says.

This makes me think if we don't overuse toMap() in places where native methods would do it's job.

The toMap() for Map() is useless, the toMap() for Object is Object.entries().

But now for writer.setAttributes( { foo: 'bar' } ) we will create new Map() then we will use it to iterate over its entries. I don't know how (if at all) this would impact the performance and/or memory but I get a feeling that we should at least inspect the usage of this and similar API.

[ma2ciek]

I'm trying to understand why/where toMap() is used and in some places it looks like Object.entries():

I guess that if we want to simply iterate over object props then the for-in loop will always be the fastest way 😅
https://hackernoon.com/5-techniques-to-iterate-over-javascript-object-entries-and-their-performance-6602dcb708a8

[Reinmar]

I pushed to ckeditor5 a branch on which I was testing the performance: https://github.com/ckeditor/ckeditor5/issues/3767#issuecomment-559446455.

[Reinmar]

One additional note – when doing any performance-oriented work, do always start from a real issue.

There are thousands of ways how we could improve our code as most of it wasn't written with perf in mind and certainly wasn't tested for that. But from my experience, real issues point you to completely different places than intuition.

Also, it's very often that there's no way to improve the performance of a certain code by optimizing that code. Instead, we should be looking at how to not execute it at all or do that far less frequently.

This is all quite well visible in the flame chart combined with some additional simple tests. So, really, start there.