Opened 7 months ago

Closed 7 months ago

Last modified 6 months ago

#7257 closed change (fixed)

Throttle badge updates

Reported by: mjethani Assignee: mjethani
Priority: P2 Milestone: Adblock-Plus-3.5-for-Chrome-Opera-Firefox
Module: Platform Keywords:
Cc: sebastian, kzar Blocked By:
Blocking: #7000 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29996582/

Description (last modified by mjethani)

Background

Despite the changes in #7253, the extension still does a lot of unnecessary and avoidable updating of the badge. In this case, it is the badge text, which is updated every time there is a filter hit. The fact is that a lot of filter hits happen too close together such that the human eye could barely perceive the update to the text, assuming the browser even rendered it. The other fact is that a lot of filter hits happen in background tabs (especially when a link is opened in a new tab that is kept in the background) and tabs in other browser windows that are out of focus.

We could do a couple of things to minimize CPU usage in such cases:

  1. Throttle updates to the badge text at a lower refresh rate. In my tests, 4 fps (frames per second) seemed OK and did not feel like it was too slow.
  2. Update the badge text for only the active tab in the focused window; alternatively, update it for all active tabs in all windows.

I have written a patch that does both of the above. In order to test it, create the following HTML document:

<!DOCTYPE html>
<html>
  <body>
    <script>
      for (let i = 1; i <= 100; i++)
        setTimeout(() => fetch("https://example.com/test"), i * 50);
    </script>
  </body>
</html>

Then load the extension, open Chrome DevTools for the background page, go to the Performance tab, and start profiling. Then run the following Bash script to open the same document in 10 tabs:

#!/bin/bash
for i in $(seq 10); do
  open -a 'Google Chrome' test.html
done

After about 15 seconds, stop the profiler. If you look at the Summary tab, you'll find that the extension spends significantly less time executing script code with the patch applied. On my machine it cut the CPU usage in half.

What to change

In lib/stats.js:

  1. Keep track of all the active tabs and the windows to which they belong.
  2. When there's a filter hit, instead of updating the badge right away, schedule an update at a timeout of 250 ms (4 fps); on timeout, update all active tabs with their values from blockedPerPage.

Hints for testers

After this change, the badge will be updated for only any active tabs. There should be no visible difference in behavior to the user. In order to ensure that this is working as intended:

  1. Open two windows and two tabs in each of the windows; load some website where there are a number of filter hits (e.g. nytimes.com) in each tab; then switch between the tabs/windows and make sure that every time a tab becomes active it gets updated with the latest number of hits for the tab.
  2. Close one of the windows and repeat the above; make sure there are no errors in the background page console.
  3. Close all windows and make sure there are no errors in the background page console.
  4. Open one window and repeat the steps in 1; again, make sure the badge is updated correctly and there are no errors in the background page console.
  5. Turn off the "Show number of ads blocked in icon" option on the options page, reload the page, and make sure the number of hits is no longer shown
  6. Turn on the "Show number of ads blocked in icon" option on the options page, reload the page, and make sure the number of hits is shown now

Change History (12)

comment:1 Changed 7 months ago by mjethani

  • Review URL(s) modified (diff)

comment:2 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:3 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:4 Changed 7 months ago by mjethani

  • Cc sebastian kzar added
  • Component changed from Unknown to Platform

comment:5 Changed 7 months ago by mjethani

If we can get agreement on this, I'd like to get this into ABP 3.5. It will make a difference to the benchmarks.

comment:6 Changed 7 months ago by abpbot

A commit referencing this issue has landed:
Issue 7257 - Throttle badge updates

comment:7 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:8 Changed 7 months ago by mjethani

  • Description modified (diff)

comment:9 Changed 7 months ago by mjethani

  • Owner set to mjethani

comment:10 Changed 7 months ago by mjethani

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from new to closed

comment:11 Changed 7 months ago by sebastian

  • Priority changed from Unknown to P2
  • Ready set

comment:12 Changed 6 months ago by ukacar

  • Verified working set
Note: See TracTickets for help on using tickets.