Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Used a cheaper type check in toMap function, to improve performance #323

Merged
merged 2 commits into from
Feb 6, 2020

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Feb 5, 2020

Suggested merge commit message (convention)

Other: Improved toMap method performance. This results with improved editor data processing speed. Closes ckeditor/ckeditor5#5854.


Additional information

lodash' isPlainObject is a pretty performance expensive function, so should be avoided in places that are called frequently. This PR reverses the condition so a cheaper and more common condition is used first.

Alternatively we could create our own, more optimized isPlainObject implementation, but since our code uses iterables for the most part here, it's enough to just reverse the condition check.

You can use manual tests added in ckeditor/ckeditor5#5880 to have use performance tests.

There's a sub pr for this: ckeditor/ckeditor5-engine#1822.

Overview

tl;dr

test case total time (ms/%) PR total time (ms/%) total click handler time PR total click handler time
rich editor setData small content 7.6ms / 5.7% 0.5ms / 0.4% 131.92ms 129.39ms (Reduced by ~1.92%)
rich editor setData medium content 805.4ms / 16.2% 21.4ms / 0.5% 4.99s 4.02s (reduced by ~19.4%)
performance/setdata full websites (styled) 632.2ms / 13.7% 20.7ms / 0.5% 4.23s 3.55s (reduced by ~%16)

RichEditor (short semantic setData)

Before:

After:

RichEditor (medium semantic setData)

Before:

After:

setData (full websites)

Before:

After:

The toMap function is used during view element creation (which happens a lot) and lodash's isPlainObject method is pretty expensive.
@coveralls
Copy link

coveralls commented Feb 5, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling b1bf335 on i/5854 into b57fc3f on master.

@mlewand mlewand marked this pull request as ready for review February 5, 2020 14:16
@jodator jodator self-assigned this Feb 5, 2020
@jodator jodator self-requested a review February 5, 2020 14:22
@oleq
Copy link
Member

oleq commented Feb 5, 2020

Guys, make sure these PRs are backed by noticeable performance gain confirmed in Chrome Dev Tools. For instance, do some heavy editor startup check before and after.

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT 👍 - I have added a cross-link to have this method be more discoverable.

@jodator jodator merged commit fef816e into master Feb 6, 2020
@jodator jodator deleted the i/5854 branch February 6, 2020 13:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve performance of toMap() && parseAttributes()
4 participants