Last modified: 2014-07-09 00:36:16 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T66709, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 64709 - VisualEditor: ve.ce.ProtectedNode.prototype.onProtectedSetup is a performance hog
VisualEditor: ve.ce.ProtectedNode.prototype.onProtectedSetup is a performance...
Status: RESOLVED FIXED
Product: VisualEditor
Classification: Unclassified
ContentEditable (Other open bugs)
unspecified
All All
: High normal
: VE-deploy-2014-05-15
Assigned To: Ed Sanders
: performance
Depends on:
Blocks: ve-performance
  Show dependency treegraph
 
Reported: 2014-05-01 18:59 UTC by Jon
Modified: 2014-07-09 00:36 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Jon 2014-05-01 18:59:02 UTC
Running JavaScript profiling on both desktop and mobile the majority of time is spent in jQuery.extend.css - the majority of which seems to be caused by ve.ce.ProtectedNode.prototype.onProtectedSetup

On mobile this seems to be even more severe.

In desktop 7.9% of execution time to load VisualEditor was spent in jQuery.extend.css - this was the most heavy function. In mobile it was also the most heavy - however in mobile a whopping 23.76% of time was spent in jQuery.extend.css

Stubbing out the contents of the foreach loop 
this.$element.add( this.$element.find( '*' ) ).each( function () {

seems to remedy this problem.
Comment 1 Jon 2014-05-01 19:13:41 UTC
The majority of time is spent in looking up the float property but importNode is also a relatively big performance hog here
Comment 2 Trevor Parscal 2014-05-01 20:10:00 UTC
There may be ways to improve this in the short term, but what we really need to do is experiment with using an SVG overlay.
Comment 3 Juliusz Gonera 2014-05-01 20:34:19 UTC
The question is, why is this more of a problem with mobile skin than with Vector?
Comment 4 Jon 2014-05-01 20:38:19 UTC
We have a lot more DOM nodes in mobile - 2 times the number of DOM nodes in an article. In desktop they replace the page rather than create an overlay..
Comment 5 Jon 2014-05-01 20:48:56 UTC
So I tried removing #content element - no visible difference.
Killing our stylesheets - visible difference (drops 1.59%).

This must be linked to causing reflows, that are more expensive in the Minerva skin.
Comment 7 Jon 2014-05-01 21:45:55 UTC
I went deeper down the rabbit hole... 

Removing all button styling drops this by 23.76% to 16.4%
It drops to 8.6% with removal of 'skins.minerva.chrome.styles', 8.6% when I drop the use of ui.less

Both the stylings for buttons and ui.less use last-child, nth-child, first-child, not selectors. These seem to be the major pain point.

See bug 64719.
Comment 8 Roan Kattouw 2014-05-02 22:53:12 UTC
https://gerrit.wikimedia.org/r/130823 (for bug 52499) should make this perform a bit better as well, by 1) reducing the number of elements protectedNode CSS is applied to and 2) targeting them with a class rather than a star selector: .ve-ce-protectedNode * is an expensive selector just like .header > *:nth-child(3)
Comment 9 James Forrester 2014-05-15 09:16:40 UTC
Additional fixes made for the 2014-05-15 release – but not sure what counts as "fixing" this bug. Jon, could you re-test?
Comment 10 James Forrester 2014-07-09 00:35:54 UTC
Based on the work that Ed did, I'm going to mark this as closed.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links