r93515 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r93514‎ | r93515 | r93516 >
Date:06:33, 30 July 2011
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
mediawiki.js request() caching deep level object member access.
- Instead of requesting index 0 of dependancies-argument, and the value of that of the registry, and the dependancies object in that of which we read the length property is very deep. Even worse when doing that both in the head and in the body of a for()-loop.
- Instead caching reference access to dependancies object of the registry item.
- Plus a simple by-value of the length property
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -630,8 +630,12 @@
631631 if ( typeof dependencies === 'string' ) {
632632 dependencies = [dependencies];
633633 if ( dependencies[0] in registry ) {
634 - for ( var n = 0; n < registry[dependencies[0]].dependencies.length; n++ ) {
635 - dependencies[dependencies.length] = registry[dependencies[0]].dependencies[n];
 634+ // Cache repetitively accessed deep level object member
 635+ var regItemDeps = registry[dependencies[0]].dependencies,
 636+ // Cache to avoid looped access to length property
 637+ regItemDepLen = regItemDeps.length;
 638+ for ( var n = 0; n < regItemDepLen; n++ ) {
 639+ dependencies[dependencies.length] = regItemDeps[n];
636640 }
637641 }
638642 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r93654Follows-up r93515 CR: Whitespacekrinkle21:03, 1 August 2011

Comments

#Comment by Nikerabbit (talk | contribs)   09:24, 30 July 2011

Why no var for regItemsDepLen?

#Comment by Dantman (talk | contribs)   09:42, 30 July 2011

...and what happened to the standard arr.push(); Same for the other line, the more readable way to do this would be: 

for ( var n = 0, regItemDepLen = regItemDeps.length; n < regItemDepLen; n++ ) {
  dependencies.push(regItemDeps[n]);
}

..though frankly, you could do this with either of these: <source lang=javascript> dependencies.push.apply(dependencies, regItemDeps); // efficient but perhaps a little advanced to read; I like to create an .append() method to make it readable. dependencies = dependencies.concat(regItemDeps); // less efficient, more readable, may have side effects if the dependencies array is mentioned elsewhere.

Maybe something like mw.util.Array.append?

#Comment by Krinkle (talk | contribs)   12:09, 1 August 2011

@Nikerabbit:

Because that would trigger a syntax error as it's one var statement, not two separate one.

Individual variables are separated by a comma ("var foo, bar; vs. var foo; var bar;).

#Comment by Dantman (talk | contribs)   12:17, 1 August 2011

...that should have been indented. The comment adding an extra line in between the var and the variable doesn't help make it any more understandable.

#Comment by Krinkle (talk | contribs)   21:04, 1 August 2011

Good point. Fixed in r93654.

Status & tagging log