Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance research #5880

Closed
9 tasks done
Reinmar opened this issue Dec 2, 2019 · 8 comments · Fixed by #6176
Closed
9 tasks done

Performance research #5880

Reinmar opened this issue Dec 2, 2019 · 8 comments · Fixed by #6176
Assignees
Labels
type:performance This issue reports a performance issue or a possible performance improvement. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@Reinmar
Copy link
Member

Reinmar commented Dec 2, 2019

Let's start from creating a set of manual tests on which we'll be able to reliably and quickly test typical performance-oriented scenarios.

Some ideas for those scenarios:

  • Loading large HTML data with a lot of attributes.
    • Coming from an editor (so, only elements and structures that the editor could've produced).
    • Coming from external sources (e.g. other website), so with structures, elements and attributes that the editor doesn't understand and many inline styles (so we can test the new style normalization).
  • An editor with a long content (same cases as above) but focused on testing features inside the editor (so the content should be already preloaded).
  • Inserting content (like pasting) into an editor.
    • Also, 3-4 data sets.
    • The editor content should be diverse as well so you can test various scenarios (pasting into a table, image caption, middle of something, etc.)
  • Loading a couple of editors at once. We expect to see a linear time growth, so we could test just one, but let's have ~3-5 as at those points the load times start to be noticeable.

Other considerations:

  • The action to be tested should be triggered by a button (instead of happening automatically).
  • Use rich setups of editor plugins (so we cover diverse types of those).

Subtasks:

@Reinmar Reinmar added this to the iteration 29 milestone Dec 2, 2019
@Reinmar Reinmar self-assigned this Dec 2, 2019
@zadam
Copy link

zadam commented Jan 1, 2020

It's great to see effort on this topic. As an example I'm attaching a HTML file which takes couple of seconds (5-10 on my machine) to load into CKEditor. The file is big-ish, but still something you might expect to work with in the CKEditor.

BMesh Operators.zip

@Reinmar Reinmar assigned mlewand and unassigned Reinmar Jan 17, 2020
@Reinmar Reinmar added type:performance This issue reports a performance issue or a possible performance improvement. type:task This issue reports a chore (non-production change) and other types of "todos". status:confirmed labels Jan 17, 2020
@mlewand
Copy link
Contributor

mlewand commented Jan 22, 2020

Added subtasks list for better tracking.

jodator added a commit that referenced this issue Jan 23, 2020
Internal: Added editor setData manual performance test. Part of #5880.
jodator added a commit that referenced this issue Jan 23, 2020
Tests: Added editor setData manual performance test. Part of #5880.
oleq added a commit that referenced this issue Jan 28, 2020
Tests: Added a test with an editor with lots of plugins to make testing interaction between plugins easier for the team (see #5880).
@Reinmar
Copy link
Member Author

Reinmar commented Jan 28, 2020

One note – I saw the newly added tests. They use fetch() in getFileContents(). AFAIK you can do a simple import 'path' as foo or something similar. IIRC we have a loader for txt files. You can check how it works in PFO tests.

@mlewand
Copy link
Contributor

mlewand commented Feb 4, 2020

tl;dr; for small editors most of the time goes to editor UI rendering (e.g. color dropdowns, table dropdown). In case of editors with a longer content we'd need to optimize data processing.

You can load profiles in your inspector from this archive: editor-init-profiles.zip.

editorinit - short, semantic content

Performance chart - small semantic content

Bottoms up - small semantic content

Total click handler time 1.14s.

Most of the time is consumed by initializing the toolbar (582ms), with font bg/color dropdown taking the longest time to render:

Which is similar to #6161.

editorinit - medium, semantic content

Click handler execution time 5.94s

As editor contains more content, it's no longer the UI - instead lodash's getRawTag and src/model/position#get parent function starts to be visible.

The first issue comes from our toMap calls, and was also mentioned in #5854.

Taking a step back and looking at the bigger picture, it's _runPendingChanges where the 85% (both calls combined) of total time goes, so this is where we should look for improvements.

editorinit - long, semantic content

Content stats: 6174 elements, 9.228 text nodes.

tl;dr; src/model/position#get parent (17.1 self %) and getRawTag (16.9 self %) hit the town.

Click handling took 19.4 s.

As the content gets larger you can see that, intuitively, model and editable output takes more time.

One thing to note is that src/model/position#get parent function is a relatively small and fast one, but it's called a staggering 858k times.

edit: added bottom-up list (note, the bottom-up is taken from a different run, where the timing turned out a bit different then above 24.7s in click handler)

editorinit - short, styled content

Click handling took 1.48s.

In this case time distribution is almost 50%/50% - half going for editor UI view initialization and another half on editable handling (toView, toModel, etc.).

editorinit - full website styled content

Click handling took 6.28s.

Call graph goes much deeper than previous ones, probably because there's more nested content.

Again, lodash's getRawTag and src/model/position#get parent function came first when looking at the self execution time.

View/model generation takes the most of the time.

Conditions

Tests were performed on latest master branch today, in Chrome 79.0.3945.130 running on Windows 10.

@mlewand
Copy link
Contributor

mlewand commented Feb 4, 2020

There are some quick wins possible. I recall seeing getRawTag (which is a result of calling isPlainObject) as a performance issue longer time ago.

Time given in seconds.

case master i/4504-rebased branch improvement
medium semantic content 5.94 4.62 22.22%
full styled website 6.28 5.11 18.63%
short styled content 1.48 1.22 15.27%

Pushed the changes to i/4504-rebased engine and utils repos.

https://github.com/ckeditor/ckeditor5-utils/tree/i/4504-rebased
https://github.com/ckeditor/ckeditor5-engine/tree/i/4504-rebased

@oleq
Copy link
Member

oleq commented Feb 4, 2020

Related ckeditor/ckeditor5-utils#322.

@mlewand
Copy link
Contributor

mlewand commented Feb 5, 2020

There are couple of issues created from this one:

  • #6195: Improve listenTo() and fire() function execution time
  • #6194: Toolbar grouping should recalculate the styles less frequently
  • #6193: Table's InsertTableView takes a long time to render
  • #6192: Font color/background UI rendering should be improved

Also couple other things could be noted based on research but these probably couldn't be fixed with reasonable effort:

  • SVG parsing (DomParser().parseFromString()) takes a little of time in case of initializing editor with small amount of data.
  • view.(element/node) / model.(element/node) is regexps usage could be optimized (reuse regexps or try to use less performance hungry functions, e.g. String.startsWith / String.endsWith). It starts to take 2-3% of total time in case of medium and longer, semantic content. But is irrelevant for smaller content cases.

@oleq
Copy link
Member

oleq commented Feb 6, 2020

It saved ~10ms startup time when switched to innerHTML in an editor with an average toolbar (8 icons). AFAIR this caused some problems in Edge but since Edge will be officially Chromium soon, we may consider this small tweak (works in Chrome, FF and Safari).


~29ms gain in http://localhost:8125/ckeditor5/tests/manual/performance/richeditor.html.


Diff for future reference

diff --git a/src/icon/iconview.js b/src/icon/iconview.js
index c33f95a..43dd9cb 100644
--- a/src/icon/iconview.js
+++ b/src/icon/iconview.js
@@ -95,19 +95,13 @@ export default class IconView extends View {
           */
          _updateXMLContent() {
                   if ( this.content ) {
-                           const parsed = new DOMParser().parseFromString( this.content.trim(), 'image/svg+xml' );
-                           const svg = parsed.querySelector( 'svg' );
-                           const viewBox = svg.getAttribute( 'viewBox' );
+                           this.element.innerHTML = this.content.trim();
+
+                           const viewBox = this.element.firstChild.getAttribute( 'viewBox' );

                            if ( viewBox ) {
                                     this.viewBox = viewBox;
                            }
-
-                           this.element.innerHTML = '';
-
-                           while ( svg.childNodes.length > 0 ) {
-                                    this.element.appendChild( svg.childNodes[ 0 ] );
-                           }
                   }
          }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:performance This issue reports a performance issue or a possible performance improvement. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants