Issue 29334778: Issue 3532 - Improved preformance of icon animation and fixed a memory leak (Closed)
|
Created:
Jan. 27, 2016, 6:57 p.m. by Sebastian Noack Modified:
Jan. 28, 2016, 12:48 p.m. Reviewers:
kzar Visibility:
Public. |
DescriptionIssue 3532 - Improved preformance of icon animation and fixed a memory leak
Patch Set 1 #
Total comments: 17
Patch Set 2 : Added missing newline #Patch Set 3 : Moved and renamed functions #MessagesTotal messages: 4
https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js File lib/icon.js (left): https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode120 lib/icon.js:120: function playAnimation(frames) 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) https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode122 lib/icon.js:122: let animationStep = 0; Unrelated: We always define variables where we need them, not in any parent scope. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode138 lib/icon.js:138: }; Unrelated: Removed redundant semicolon. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode142 lib/icon.js:142: animationStep = 0; Unrelated: Resetting that value is unneeded, as that variable isn't used anymore after clearing this interval. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#newcode139 lib/icon.js:139: if (opacity != oldOpacity) 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. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#newcode189 lib/icon.js:189: if (onActivated) 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.
https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js File lib/icon.js (left): https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode120 lib/icon.js:120: function playAnimation(frames) On 2016/01/27 19:09:20, Sebastian Noack wrote: > 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) 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.) https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode122 lib/icon.js:122: let animationStep = 0; On 2016/01/27 19:09:20, Sebastian Noack wrote: > Unrelated: We always define variables where we need them, not in any parent > scope. Acknowledged. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode138 lib/icon.js:138: }; On 2016/01/27 19:09:20, Sebastian Noack wrote: > Unrelated: Removed redundant semicolon. Acknowledged. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode142 lib/icon.js:142: animationStep = 0; On 2016/01/27 19:09:20, Sebastian Noack wrote: > Unrelated: Resetting that value is unneeded, as that variable isn't used anymore > after clearing this interval. Acknowledged. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#newcode128 lib/icon.js:128: onActivated = page => { 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 "{"?) https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#newcode139 lib/icon.js:139: if (opacity != oldOpacity) On 2016/01/27 19:09:20, Sebastian Noack wrote: > 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. Of course, good idea. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#newcode189 lib/icon.js:189: if (onActivated) On 2016/01/27 19:09:20, Sebastian Noack wrote: > 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. Dang, good point. (Note, my stopAnimation closure is now looking increasingly like a good idea again.)
https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js File lib/icon.js (left): https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#oldcode120 lib/icon.js:120: function playAnimation(frames) On 2016/01/28 09:51:04, kzar wrote: > On 2016/01/27 19:09:20, Sebastian Noack wrote: > > 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) > > 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.) Done. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#newcode128 lib/icon.js:128: onActivated = page => { On 2016/01/28 09:51:04, kzar wrote: > Arrow functions are for anonymous functions, if it needs a name surely it's > better to use the old syntax? 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 > opening "{"?) Done. https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#newcode189 lib/icon.js:189: if (onActivated) On 2016/01/28 09:51:04, kzar wrote: > On 2016/01/27 19:09:20, Sebastian Noack wrote: > > 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. > > Dang, good point. (Note, my stopAnimation closure is now looking increasingly > like a good idea again.) (Not really, it doesn't matter where you do the cleanup, as long as you do it)
LGTM https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334778/diff/29334779/lib/icon.js#newcode128 lib/icon.js:128: onActivated = page => { On 2016/01/28 10:14:21, Sebastian Noack wrote: > On 2016/01/28 09:51:04, kzar wrote: > > Arrow functions are for anonymous functions, if it needs a name surely it's > > better to use the old syntax? > > 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 > > opening "{"?) > > Done. Acknowledged. 
|
