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 created this task.Jan 23 2015, 7:29 AM
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 project: VisualEditor. · View Herald TranscriptJan 23 2015, 7:29 AM
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.
Catrope added a comment.EditedJan 23 2015, 7:21 PM

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 added a subscriber: gerritbot.

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 To Triage to Blocked on the VisualEditor board.Jan 26 2015, 6:13 PM
Catrope moved this task from Blocked to 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

Krenair assigned this task to Catrope.Jan 28 2015, 6:05 AM
Krenair added a subscriber: Krenair.
Jdforrester-WMF edited a custom field.Feb 2 2015, 8:06 PM