Issue 29995555: Issue 7253 - Pre-render icons for badge on Chromium (Closed)
|
Created:
Feb. 1, 2019, 2:28 p.m. by Manish Jethani Modified:
Feb. 3, 2019, 11:09 a.m. Reviewers:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionChrome seems to render the icon each time browserAction.setIcon is called. If we render the icon in advance onto a canvas, we save 5-10% in CPU usage.
This patch pre-renders the icons into ImageData objects and passes the relevant ImageData object to browserAction.setIcon.
Memory footprint: ~12 KB
Time for initial render: <0.1% of startup time
Patch Set 1 #
Total comments: 1
Patch Set 2 : Restrict to Chromium #
Total comments: 8
Patch Set 3 : Use only 16x16 and 32x32 icons #Patch Set 4 : Update comment #Patch Set 5 : Update for loop #
Total comments: 1
Patch Set 6 : Use iconPath and iconImageData separately #Patch Set 7 : Do not use 19x19 and 38x38 icons on any platform #Patch Set 8 : Remove 19x19 and 38x38 icons entirely #Patch Set 9 : Remove icons from metadata.chrome #
Total comments: 8
Patch Set 10 : Rebase on Rietveld patch 29997602 #Patch Set 11 : Split into two methods #Patch Set 12 : Rewrite renderIcons #
Total comments: 3
MessagesTotal messages: 26
Patch Set 1 See description above. I'll file an issue. https://codereview.adblockplus.org/29995555/diff/29995556/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29995556/lib/icon.js#newcode34 lib/icon.js:34: let icons = [null, null]; Not whitelisted (0) and whitelisted (1).
If you want to see what difference this patch makes, follow these steps before and after: 1. Restart the extension 2. Run the script below (to fill up your caches/cookies so there's no before vs. after bias) for domain in $(curl https://www.alexa.com/topsites | sed -nE 's/.*\/siteinfo\/([^"]*).*/\1/p') do open -a 'Google Chrome' "http://$domain" done 3. Edit lib/synchronizer.js in adblockpluscore and set INITIAL_DELAY to 30 minutes (so nothing messes with the test) 4. Restart the extension 5. Open DevTools, go to the Performance tab, start profiling 6. Run the script below (note: this time there's a sleep) for domain in $(curl https://www.alexa.com/topsites | sed -nE 's/.*\/siteinfo\/([^"]*).*/\1/p') do open -a 'Google Chrome' "http://$domain" sleep 2 done 7. After about 120 seconds, once all tabs have opened up, stop the profiler and observe the "Scripting" figure in the Summary tab (pie chart) I'm seeing an improvement of 5-10%. I am convinced this is a good idea, let me open a ticket.
Patch Set 2: Restrict to Chromium
Also filed an issue now: https://issues.adblockplus.org/ticket/7253
https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js#n... ext/background.js:338: 40: this._changes.icon.path.replace("$size", "40") This should go after trying to use imageData. If we use imageData, those string replacements are redundant. https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js#n... ext/background.js:345: try Why do we need try...catch here? Can't we just check in imageData have been provided? https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js#newcode68 lib/icon.js:68: loadImage("icons/abp-40-whitelisted.png"), The icons in the sizes 20 and 40 are only used on Microsoft Edge, hence we don't need to pretender them if we only use this code path on Chromium. The sizes 19 and 38 were used by older Chromium versions. We might no longer need them anymore, but please double check.
Patch Set 3-4 https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js#n... ext/background.js:338: 40: this._changes.icon.path.replace("$size", "40") On 2019/02/03 06:43:18, Sebastian Noack wrote: > This should go after trying to use imageData. If we use imageData, those string > replacements are redundant. Done. https://codereview.adblockplus.org/29995555/diff/29996576/ext/background.js#n... ext/background.js:345: try On 2019/02/03 06:43:18, Sebastian Noack wrote: > Why do we need try...catch here? Can't we just check in imageData have been > provided? Yes, makes sense. Done. https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js#newcode68 lib/icon.js:68: loadImage("icons/abp-40-whitelisted.png"), On 2019/02/03 06:43:18, Sebastian Noack wrote: > The icons in the sizes 20 and 40 are only used on Microsoft Edge, hence we don't > need to pretender them if we only use this code path on Chromium. Done. > The sizes 19 and 38 were used by older Chromium versions. We might no longer > need them anymore, but please double check. I found the bug that led to support for arbitrary sizes: https://bugs.chromium.org/p/chromium/issues/detail?id=564926 I think this probably made it into Chrome 49, but I'm not sure. I have removed 19x19 and 38x38 in this patch. QA will test on Chrome 49 anyway (I'll make a note in the Trac ticket), and if it gives problems we can add those sizes back. Done.
Patch Set 5: Update for loop
https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js#newcode68 lib/icon.js:68: loadImage("icons/abp-40-whitelisted.png"), On 2019/02/03 07:26:49, Manish Jethani wrote: > On 2019/02/03 06:43:18, Sebastian Noack wrote: > > The icons in the sizes 20 and 40 are only used on Microsoft Edge, hence we > don't > > need to pretender them if we only use this code path on Chromium. > > Done. > > > The sizes 19 and 38 were used by older Chromium versions. We might no longer > > need them anymore, but please double check. > > I found the bug that led to support for arbitrary sizes: > > https://bugs.chromium.org/p/chromium/issues/detail?id=564926 > > I think this probably made it into Chrome 49, but I'm not sure. I have removed > 19x19 and 38x38 in this patch. QA will test on Chrome 49 anyway (I'll make a > note in the Trac ticket), and if it gives problems we can add those sizes back. > > Done. I'm certain that it will work on Chrome 49. But Chrome changed the UI at some point, rendering the browser action icons in 16px instead of 19px (respectively 32px and 38px on high-res displays). The worst that could happen (if some supported versions are still rendering the browser action icon in 19px/38px) is that the icon will look blurry/pixelated. Also if these sizes are in fact no longer relevant we should remove them completely (not only from this code path), as Firefox won't use them either. https://codereview.adblockplus.org/29995555/diff/29997567/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29995555/diff/29997567/ext/background.js#n... ext/background.js:413: this._addChange("icon", {path, imageData}); How about using _addChange("iconPath") and _addChange("iconImageData") rather than creating an object and branching off withing the "icon" case?
https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29996576/lib/icon.js#newcode68 lib/icon.js:68: loadImage("icons/abp-40-whitelisted.png"), On 2019/02/03 07:45:15, Sebastian Noack wrote: > On 2019/02/03 07:26:49, Manish Jethani wrote: > > On 2019/02/03 06:43:18, Sebastian Noack wrote: > > > The icons in the sizes 20 and 40 are only used on Microsoft Edge, hence we > > don't > > > need to pretender them if we only use this code path on Chromium. > > > > Done. > > > > > The sizes 19 and 38 were used by older Chromium versions. We might no longer > > > need them anymore, but please double check. > > > > I found the bug that led to support for arbitrary sizes: > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=564926 > > > > I think this probably made it into Chrome 49, but I'm not sure. I have removed > > 19x19 and 38x38 in this patch. QA will test on Chrome 49 anyway (I'll make a > > note in the Trac ticket), and if it gives problems we can add those sizes > back. > > > > Done. > > I'm certain that it will work on Chrome 49. But Chrome changed the UI at some > point, rendering the browser action icons in 16px instead of 19px (respectively > 32px and 38px on high-res displays). The worst that could happen (if some > supported versions are still rendering the browser action icon in 19px/38px) is > that the icon will look blurry/pixelated. Alright, let's test have QA test it out on Chrome 49. I'm personally OK with it appearing blurred on really old versions of Chrome if it means better performance on the newer versions. > Also if these sizes are in fact no longer relevant we should remove them > completely (not only from this code path), as Firefox won't use them either. Let's make this a separate issue? If QA finds that the icons looks OK on Chrome 49, we can make this change for the next release.
On 2019/02/03 08:00:02, Manish Jethani wrote: > > Also if these sizes are in fact no longer relevant we should remove them > > completely (not only from this code path), as Firefox won't use them either. > > Let's make this a separate issue? If QA finds that the icons looks OK on Chrome > 49, we can make this change for the next release. While indeed somewhat unrelated here, I like too keep a consistent state with each change. As for QA, they have to test the behavior of the browser action icon across different browsers anyway, if it's just to make sure that the logic is switched as intended. So we might as well, just remove the 19/38px icons with the same change.
On 2019/02/03 08:09:01, Sebastian Noack wrote: > On 2019/02/03 08:00:02, Manish Jethani wrote: > > > Also if these sizes are in fact no longer relevant we should remove them > > > completely (not only from this code path), as Firefox won't use them either. > > > > Let's make this a separate issue? If QA finds that the icons looks OK on > Chrome > > 49, we can make this change for the next release. > > While indeed somewhat unrelated here, I like too keep a consistent state with > each change. As for QA, they have to test the behavior of the browser action > icon across different browsers anyway, if it's just to make sure that the logic > is switched as intended. So we might as well, just remove the 19/38px icons with > the same change. Are you sure about this? The patch would become huge. We would have to make changes to lib/icon.js and delete the 19x19 and 38x38 image files. Unless you mean to make the change only in ext/background.js for now.
Patch Set 6: Use iconPath and iconImageData separately
Patch Set 7: Do not use 19x19 and 38x38 icons on any platform I'm assuming you mean that we should stop using 19x19 and 38x38 icons in ext/background.js only. As I said, if we want to remove these from lib/icon.js then it's going to create a huge diff and I don't see how it's related to the issue anymore. I don't mind doing it for this release but at least it should be a separate issue.
On 2019/02/03 08:21:13, Manish Jethani wrote: > Patch Set 7: Do not use 19x19 and 38x38 icons on any platform > > I'm assuming you mean that we should stop using 19x19 and 38x38 icons in > ext/background.js only. > > As I said, if we want to remove these from lib/icon.js then it's going to create > a huge diff and I don't see how it's related to the issue anymore. I don't mind > doing it for this release but at least it should be a separate issue. On second thoughts, we should just revert Patch Set 7 here. It's not about optimizing anything for Chrome, it's about other browsers now. This issue is about Chrome specifically.
How about removing the redundant icons first and the base this patch on top. My points are: 1. It is relevant to the changes here as less pre-rendered icons add less memory footprint. 2. It's better to change thing consistently, i.e. everywhere at the same time.
Patch Set 8: Remove 19x19 and 38x38 icons entirely Patch Set 9: Remove icons from metadata.chrome Alright, let's do this in this patch itself.
https://codereview.adblockplus.org/29995555/diff/29997590/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/ext/background.js#n... ext/background.js:411: if (imageData) Maybe it would be better, to just have two different methods than this switch. https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; Not: This can be inlined below. https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode159 lib/icon.js:159: let sizes = [16, 20, 32, 40]; Not: While at it, same here.
On 2019/02/03 08:33:02, Manish Jethani wrote: > Patch Set 8: Remove 19x19 and 38x38 icons entirely > Patch Set 9: Remove icons from metadata.chrome > > Alright, let's do this in this patch itself. Changed my mind again, here's the patch: https://codereview.adblockplus.org/29997602/ https://issues.adblockplus.org/ticket/7256 I'll rebase this one.
https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; On 2019/02/03 08:47:16, Sebastian Noack wrote: > Not: This can be inlined below. Do you mean unroll the loop?
Patch Set 10: Rebase on Rietveld patch 29997602 Patch Set 11: Split into two methods https://codereview.adblockplus.org/29995555/diff/29997590/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/ext/background.js#n... ext/background.js:411: if (imageData) On 2019/02/03 08:47:16, Sebastian Noack wrote: > Maybe it would be better, to just have two different methods than this switch. Done.
Patch Set 12: Rewrite renderIcons This a better, there's no need to use Promise.all and then do all kinds of gymnastics. https://codereview.adblockplus.org/29995555/diff/29997621/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997621/lib/icon.js#newcode65 lib/icon.js:65: let [, size, whitelisted] = /\/abp-(16|32)(-whitelisted)?\./.exec(path); Note: size is a string, not a number, but that's fine because JS APIs handle it well.
https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; On 2019/02/03 09:09:35, Manish Jethani wrote: > On 2019/02/03 08:47:16, Sebastian Noack wrote: > > Not: This can be inlined below. > > Do you mean unroll the loop? Never mind, I have rewritten the function.
https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; On 2019/02/03 09:09:35, Manish Jethani wrote: > On 2019/02/03 08:47:16, Sebastian Noack wrote: > > Not: This can be inlined below. > > Do you mean unroll the loop? Not unrolling it but avoiding the temporary variable writing "for (let size of [16, 32])" instead. FWIW, the reason the original implementation didn't do that was how jsHydra (a fairly limited transpiler we used at the time) converted for..of loops. BTW, I meant "Not", not "Not". https://codereview.adblockplus.org/29995555/diff/29997621/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997621/lib/icon.js#newcode69 lib/icon.js:69: let imageData = icons[!!whitelisted | 0] || {}; Nit: Isn't coercing to boolean (through !!) redundant as you already coerce the value to a number (with |0)?
https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997590/lib/icon.js#newcode75 lib/icon.js:75: let sizes = [16, 32]; On 2019/02/03 10:07:53, Sebastian Noack wrote: > On 2019/02/03 09:09:35, Manish Jethani wrote: > > On 2019/02/03 08:47:16, Sebastian Noack wrote: > > > Not: This can be inlined below. > > > > Do you mean unroll the loop? > > Not unrolling it but avoiding the temporary variable writing "for (let size of > [16, 32])" instead. FWIW, the reason the original implementation didn't do that > was how jsHydra (a fairly limited transpiler we used at the time) converted > for..of loops. I see, I remember JSHydra. > BTW, I meant "Not", not "Not". I see you meant "Not" :D https://codereview.adblockplus.org/29995555/diff/29997621/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29995555/diff/29997621/lib/icon.js#newcode69 lib/icon.js:69: let imageData = icons[!!whitelisted | 0] || {}; On 2019/02/03 10:07:54, Sebastian Noack wrote: > Nit: Isn't coercing to boolean (through !!) redundant as you already coerce the > value to a number (with |0)? `"-whitelisted" | 0` is 0. Because "-whitelisted" as a number is NaN, and `NaN | 0` is 0.
LGTM 
|
