Issue 3532 - Improved preformance of icon animation and fixed a memory leak

lib/icon.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /** @module icon */
 
 "use strict";
 
 const frameOpacities =  [0.0, 0.1, 0.2, 0.3, 0.4, 0.5, 0.6, 0.7, 0.8, 0.9,
                          1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0,
                          0.9, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2, 0.1, 0.0];
 const numberOfFrames = frameOpacities.length;
 const safariPlatform = require("info").platform == "safari";
 
 let frameInterval = null;
 let animationInterval = null;
+let onActivated = null;
 let whitelistedState = new ext.PageMap();
 
 function loadImage(url)
 {
   return new Promise((resolve, reject) =>
   {
     let image = new Image();
     image.src = url;
     image.addEventListener("load", () =>
     {
@@ skipping 70 matching lines @@
         frames["" + opacity + whitelisted] = imageData;
       }
     }
 
     return frames;
   });
 }
 
 function runAnimation(notificationType)
 {
   function playAnimation(frames)

[Sebastian Noack, 2016/01/27 19:09:20]

If you don't mind, I'd also like to move that function to the top-level. Yes, it
would need an additional argument then. But IMO less nested blocks make this
code somewhat easier to read.

(The reason I didn't go ahead, was that it would make the changes below harder
to review, if doing so in the same patch set)

[kzar, 2016/01/28 09:51:04]

Sure I don't mind if you'd like to do that. (The functions could also use better
names, I just couldn't think of anything better.)

[Sebastian Noack, 2016/01/28 10:14:21]

Done.

   {
-    let animationStep = 0;

[Sebastian Noack, 2016/01/27 19:09:20]

Unrelated: We always define variables where we need them, not in any parent
scope.

[kzar, 2016/01/28 09:51:03]

Acknowledged.

     ext.pages.query({active: true}, pages =>
     {
-      function appendActivePage(page)
-      {
+      let animationStep = 0;
+      let opacity = 0;
+
+      onActivated = page => {

[kzar, 2016/01/28 09:51:04]

Arrow functions  are for anonymous functions, if it needs a name surely it's
better to use the old syntax? (Also shouldn't there be a newline before the
opening "{"?)

[Sebastian Noack, 2016/01/28 10:14:21]

We have to assign to the global variable onActivated here. So |function
onActivated() {}| wouldn't work. So yes, we need an anonymous function here,
that we assign to a global variable.

(Also shouldn't there be a newline before the
Done.

[kzar, 2016/01/28 10:19:57]

Acknowledged.

         pages.push(page);
-      }
-      ext.pages.onActivated.addListener(appendActivePage);
+        setIcon(page, notificationType, opacity, frames);
+      };
+      ext.pages.onActivated.addListener(onActivated);
 
       frameInterval = setInterval(() =>
       {
-        let opacity = frameOpacities[animationStep++];
-        for (let page of pages)
+        let oldOpacity = opacity;
+        opacity = frameOpacities[animationStep++];
+
+        if (opacity != oldOpacity)

[Sebastian Noack, 2016/01/27 19:09:20]

On my notebook (X230 with i7), I observed significant CPU usage during the 3
seconds the icon is animating. It was usally around 25% (which is already too
much) and while switching tabs during the animation it got up 90%.

Things didn't look too bad in the profiler (peaks of 1-3ms every 100ms). So I
assume that the bottleneck is in the native code that runs in Chrome's UI thread
after chrome.pageAction.setIcon() has been called.

However, not calling setIcon when the opacity didn't change seems to
significantly improve the performance.

[kzar, 2016/01/28 09:51:04]

Of course, good idea.

         {
-          if (whitelistedState.has(page))
-            setIcon(page, notificationType, opacity, frames);
-        };

[Sebastian Noack, 2016/01/27 19:09:20]

Unrelated: Removed redundant semicolon.

[kzar, 2016/01/28 09:51:04]

Acknowledged.

+          for (let page of pages)
+          {
+            if (whitelistedState.has(page))
+              setIcon(page, notificationType, opacity, frames);
+          }
+        }
 
         if (animationStep > numberOfFrames)
         {
-          animationStep = 0;

[Sebastian Noack, 2016/01/27 19:09:20]

Unrelated: Resetting that value is unneeded, as that variable isn't used anymore
after clearing this interval.

[kzar, 2016/01/28 09:51:04]

Acknowledged.

           clearInterval(frameInterval);
-          frameInterval = null;
-          ext.pages.onActivated.removeListener(appendActivePage);
+          ext.pages.onActivated.removeListener(onActivated);
+          frameInterval = onActivated = null;
         }
       }, 100);
     });
   }
 
   renderFrames(notificationType).then(frames =>
   {
     playAnimation(frames);
     animationInterval = setInterval(() => { playAnimation(frames); }, 10000);
   });
@@ skipping 16 matching lines @@
 let stopIconAnimation =
 /**
  * Stops to animate the browser action icon.
  */
 exports.stopIconAnimation = function()
 {
   if (frameInterval != null)
     clearInterval(frameInterval);
   if (animationInterval != null)
     clearInterval(animationInterval);
-  frameInterval = animationInterval = null;
+  if (onActivated)

[Sebastian Noack, 2016/01/27 19:09:20]

Yeah, that was the memory leak mentioned in the issue and review title. This
listener was previously only removed when the animation finished. Not when it
was stopped while animating.

[kzar, 2016/01/28 09:51:04]

Dang, good point. (Note, my stopAnimation closure is now looking increasingly
like a good idea again.)

[Sebastian Noack, 2016/01/28 10:14:21]

(Not really, it doesn't matter where you do the cleanup, as long as you do it)

+    ext.pages.onActivated.removeListener(onActivated);
+  frameInterval = animationInterval = onActivated = null;
 };
 
 /**
  * Starts to animate the browser action icon to indicate a pending notifcation.
  *
  * @param {string} type  The notification type (i.e: "information" or "critical")
  */
 exports.startIconAnimation = function(type)
 {
   stopIconAnimation();
   runAnimation(type);
 };