Page MenuHomePhabricator

Work around global state thrashing bugs in Sizzle
Closed, ResolvedPublic8 Estimated Story Points

Description

Per Paul Irish: "inside ve.dm.MWBlockImageNode.static.toDataElement is some call that ends up calling jQuery.filter... I'm not sure what the filtering is for but because the DOM is SO HUGE this takes a long time.". Later: "yep, the cost is enormous".

Event Timeline

ori raised the priority of this task from to Needs Triage.
ori updated the task description. (Show Details)
ori added a project: VisualEditor-Performance.
ori added subscribers: ori, Catrope, Krinkle.
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
ori triaged this task as High priority.Jan 23 2015, 7:31 AM
ori updated the task description. (Show Details)
ori set Security to None.

This seems to be due to several .children() calls:

$imgWrapper = $figure.children( 'a, span' ).eq( 0 ),
$img = $imgWrapper.children( 'img' ).eq( 0 ),
$caption = $figure.children( 'figcaption' ).eq( 0 ),

Why is this so expensive? Surely the DOM being huge shouldn't matter for .children()? Or does the <figure> in question have an insane number of direct children? I checked the Parsoid DOM for Barack Obama, and all of the <figure>s on that page have exactly 2 children. This seems like a bug in jQuery / Sizzle perhaps?

Ori tells me this is due to a bug in Sizzle where setDocument is called and expensive processing is performed pretty much every time jQuery is used on a document that isn't window.document. I'll tally how often setDocument is called and try to reduce this number by reducing jQuery usage on foreign documents in VE.

Catrope renamed this task from Investigate jQuery.filter call with ve.dm.MWBlockImageNode.static.toDataElement in call stack to Work around global state thrashing bugs in Sizzle.Jan 26 2015, 5:41 PM
gerritbot subscribed.

Change 186795 had a related patch set uploaded (by Catrope):
dm.MWBlockImageNode: Use DOM methods rather than jQuery

https://gerrit.wikimedia.org/r/186795

Patch-For-Review

Change 186796 had a related patch set uploaded (by Catrope):
Use DOM rather than jQuery for <base> resolution

https://gerrit.wikimedia.org/r/186796

Patch-For-Review

Change 186797 had a related patch set uploaded (by Catrope):
ce.GeneratedContentNode: Import DOM elements into target document early

https://gerrit.wikimedia.org/r/186797

Patch-For-Review

These three patches reduce the number of setDocument calls in my test case from 79 to 1. (1 is the minimum since setDocument is needed for initialization.)

Change 186795 merged by jenkins-bot:
dm.MWBlockImageNode: Use DOM methods rather than jQuery

https://gerrit.wikimedia.org/r/186795

Change 186796 merged by jenkins-bot:
Use DOM rather than jQuery for <base> resolution

https://gerrit.wikimedia.org/r/186796

Catrope moved this task from Blocked to FY 18-19 Q3/Q4 on the VisualEditor board.
Catrope moved this task from Next up to Done on the VisualEditor-Performance board.

Change 186797 merged by jenkins-bot:
ce.GeneratedContentNode: Import DOM elements into target document early

https://gerrit.wikimedia.org/r/186797