Issue Details (XML | Word | Printable)

Key: MBS-4066
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Normal Normal
Assignee: Ian McEwen
Reporter: Brian Schweitzer
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
MusicBrainz Server

Simple RE parser speed boost

Created: 25/Dec/11 07:36 AM   Updated: 01/Oct/12 01:25 PM   Resolved: 01/Oct/12 01:25 PM
Component/s: JavaScript
Affects Version/s: Bug fixes, 2011-12-05
Fix Version/s: 2012-10-01


 Description  « Hide

Just looking through the source to try and ID how something is added to the RE, I noticed this code in
http://git.musicbrainz.org/gitweb/?p=musicbrainz-server.git;a=blob;f=root/static/scripts/release-editor/MB/Control/ReleaseAddDisc.js;h=7cbce431815ac9a1a291e70d796054354e094c9f;hb=e5b295caa6c9959fa3efc1fb55ccab846fde16a8

64             $.each (data.tracks, function (idx, item) {
  65                 var tr = self.$table.find ('tr.track').eq(0).clone ()
  66                     .appendTo (self.$table.find ('tbody'));
  67 
  68                 var artist = item.artist ? item.artist :
  69                     item.artist_credit ? item.artist_credit.preview : "";
  70 
  71                 tr.find ('td.position').text (idx + 1);
  72                 tr.find ('td.title').text (item.name);
  73                 tr.find ('td.artist').text (artist);
  74                 tr.find ('td.length').text (MB.utility.formatTrackLength (item.length));
  75                 tr.show ();
  76             });

I've noticed that one of the slowest parts of the RE, when it parses, is that each row is created and displayed one by one. There's 4 big ways that this code could be sped up, without much effort/code change at all.

First, it's adding the rows one by one. DOM entity insertions are essentially linear - O. Insertion into the DOM is slow, whereas rendering that insertion is super fast, regardless of the size of the insertion. So inserting rows into the DOM one by one is far slower than inserting them into a fragment, then only inserting the fragment into the DOM once you're done.

Second, it's repeating the cloned track row search for each new row. Again, this is O, where it only needs to be done once. (It's also repeating the self.$table.find ('tbody') search, but that's already eliminated with the first improvement.)

Third, it's using the .eq() function instead of the :eq() selector (or better yet, the :first selector); the former involves an unneeded function call, plus it collects every match, then eliminates all but the matching one (O(n^2) at best). :eq() only collects the matching ones, which is better (O) - and in this case, since you only want :eq(0), best is to just use :first, so it stops after collecting the first match (O(1)).

Fourth, after adding the tr to the DOM, it's then doing four class traversals of the entire tr, each time manipulating the text contents of a td within that tr - class traversal is slow, plus each text manipulation is a DOM replacement (jQuery is internally rewriting the td). Far faster to do just one traversal (there will only ever be one each of position, title, artist, and length, and always in that order), and to do the replacements within a tr in a fragment (#1 above) rather than within the DOM itself.

new1           var rowHolder = $('table');
  new2&3         var trackToClone = self.$table.find ('tr.track:first');
  64             $.each (data.tracks, function (idx, item) {
  65                 var tr = trackToClone.clone ()
  66                     .appendTo (rowHolder);
  67 
  68                 var artist = item.artist ? item.artist :
  69                     item.artist_credit ? item.artist_credit.preview : "";
  70 
  new4               var rowTRs = tr.find ('td.position, td.title, td.artist, td.length');
  71                 rowTRs.eq (0).text (idx + 1);
  72                 rowTRs.eq (1).text (item.name);
  73                 rowTRs.eq (2).text (artist);
  74                 rowTRs.eq (3).text (MB.utility.formatTrackLength (item.length));
  75                 tr.show ();
  76             });
  new1           self.$table.find ('tbody').append(rowHolder.contents());


Sort Order: Ascending order - Click to sort in descending order
Oliver Charles added a comment - 27/Dec/11 12:06 PM

Could you submit this as a code review? We don't really do patches over JIRA, atm.


Brian Schweitzer added a comment - 29/Dec/11 03:33 PM

If you'd like, when I'm back in Toronto, I can.


Oliver Charles added a comment - 02/Jan/12 01:26 PM

Yea, that'd be great. Thanks!


Ian McEwen added a comment - 04/Sep/12 06:26 PM

ping – since it's been months this may need updating but this would still be nice to see as a codereview


Ian McEwen added a comment - 09/Sep/12 08:12 AM

Claiming this; I have a branch – your changes aren't exactly correct in a couple places, but I'll put up a patch with it all tomorrow morning.


Ian McEwen added a comment - 10/Sep/12 10:02 AM