Last modified: 2013-04-22 18:40:30 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 T49343, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 47343 - VisualEditor: Lazy-update SurfaceFragment
VisualEditor: Lazy-update SurfaceFragment
Status: RESOLVED FIXED
Product: VisualEditor
Classification: Unclassified
Technical Debt (Other open bugs)
unspecified
All All
: High normal
: VE-deploy-2013-04-29
Assigned To: Roan Kattouw
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-18 00:31 UTC by Roan Kattouw
Modified: 2013-04-22 18:40 UTC (History)
4 users (show)

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


Attachments

Description Roan Kattouw 2013-04-18 00:31:29 UTC
For performance reasons, we should lazy-update SurfaceFragments rather than updating them through a transact event handler.

The performance problem occurs when you create a bunch of fragments, then throw them away. The fragments are never actually destroyed, if only because the surface still has references to them because of the event binding. The discarded fragments continue to receive events about transactions and update themselves in response, but no one is using them any more.

Instead, I propose the following:

* The surface should have an ordered array of [transaction, direction] pairs that tracks everything that has ever been applied. This is not the same as the undo/redo stacks: this particular array is append-only
* Every SurfaceFragment has a property indicating which transaction (by index in the big array) is the last one that it knows about
* SurfaceFragments do not listen for transact events
* Instead, every time getRange() is called, the fragment asks the surface whether there are new transactions (with indices greater than the stored index), and if so it translates its range for them
** For this to work, all internal uses of this.range need to be changed to this.getRange()

This'll make the fragment lazy-update its range, and we won't spend any time updating fragments that aren't being used.
Comment 1 Roan Kattouw 2013-04-18 00:32:14 UTC
Trevor is adding a bunch of .destroy() calls to SurfaceFragment uses to mitigate the problem. These should be removed once this is implemented.
Comment 2 Gerrit Notification Bot 2013-04-22 11:34:19 UTC
Related URL: https://gerrit.wikimedia.org/r/60256 (Gerrit Change I9e9818da1baa8319a3002f6d74fd1aad6732a8f5)
Comment 3 James Forrester 2013-04-22 18:40:30 UTC
Now merged.

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


Navigation
Links