r104106 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r104105‎ | r104106 | r104107 >
Date:23:40, 23 November 2011
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
Another major performance improvement for updated annotated retentions
Modified paths:
  • /trunk/extensions/VisualEditor/modules/es/es.TransactionProcessor.js (modified) (history)

Diff [purge]

Index: trunk/extensions/VisualEditor/modules/es/es.TransactionProcessor.js
——@@ -217,10 +217,14 @@
218218 changes++;
219219 }
220220 if ( update && changes ) {
221 - var from = this.model.getNodeFromOffset( this.cursor );
 221+ var fromNode = this.model.getNodeFromOffset( this.cursor ),
 222+ toNode = this.model.getNodeFromOffset( to );
222223 this.model.traverseLeafNodes( function( node ) {
223224 node.emit( 'update' );
224 - }, from );
 225+ if ( node === toNode ) {
 226+ return false;
 227+ }
 228+ }, fromNode );
225229 }
226230 };
227231

Comments

#Comment by Catrope (talk | contribs)   13:21, 24 November 2011

I'm not sure if you realize this, but the nodes preceding fromNode are traversed three times by this code, and the nodes to be annotated are traversed twice:

  1. getNodeFromOffset( this.cursor ) traverses the nodes preceding fromNode (1st time)
  2. getNodeFromOffset( to ) traverses the nodes preceding fromNode (2nd time), and the nodes to be annotated (1st time)
  3. traverseLeafNodes() traverses the nodes preceding fromNode (3rd time) and the nodes to be annotated (2nd time)

Ironically, this means that your performance improvement (updating fewer nodes) might actually be slower in extreme cases (both from and to are close to the end of the document, and the document is very flat, i.e. it has a large number of top-level nodes and relatively few leaf nodes). Regardless of whether it actually is slower (I believe this revision will probably improve performance in almost all cases), there's room for improvement: don't traverse stuff three times.

I think we should make traverseLeafNodes() smarter so it will:

  • allow you to pass a "to" parameter as well as a "from" node
  • allow both nodes and offsets for from and to
  • keep track of offsets so we can avoid calling getNodeFromOffset() for the to node (for the from node it's OK)
  • (bonus) pass offsets as well as the index to the callback function

Status & tagging log