Uploaded image for project: 'MusicBrainz Server'
  1. MusicBrainz Server
  2. MBS-4066

Simple RE parser speed boost

XMLWordPrintable

    • Icon: Improvement Improvement
    • Resolution: Fixed
    • Icon: Normal Normal
    • 2012-10-01
    • Bug fixes, 2011-12-05
    • JavaScript
    • None

      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());
      

            ianmcorvidae Ian McEwen
            brianfreud Brian Schweitzer
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

              Created:
              Updated:
              Resolved:

                Version Package
                2012-10-01