Questions about performance of windows.get/tabs.query and keeping state in storage vs background

lionelw · December 6, 2019, 3:31am

Two somewhat related questions.

First question

Let’s say I’m making an addon that will display the tab count of a window on the icon badge. I could do it in two ways:

Every time tabs are created/removed/moved, do a tabs.query() for every window and count tabs anew.
Do windows.getAll() once and count the tabs to get the initial numbers, which live in the background long-term, where they are then incremented and decremented whenever tabs are created/removed/moved.

Which is more performant? See, I just want tab counts, I don’t need the entire array of tab objects. I’m basically asking if these methods are costly enough to not prefer calling them so frequently, like in #1.

Second question

I came across this Chrome extension dev article Migrate to Event Driven Background Scripts and wonder if the advice in it applies to Firefox too.

I know that Firefox does not support "persistent": false so we can skip that section.

In particular, the “Record State Changes in Storage” section discourages keeping state in the background – e.g. a bunch of tab counts as in #2 above – in favour of using storage.local.

Is it sound advice? Is it better to keep temporary data in storage rather than living in the background?

jscher2000 · December 6, 2019, 3:49am

(1) Hmm, you might try both and see which is more accurate. I’m not sure you’ll be able to measure a performance impact.

(2) I think variables have to be cheaper as they are not persisted to disk.

juraj.masiar · December 6, 2019, 10:13am

Oh man, I love performance questions!

Let’s see for ourselves:

(async () => { 
  // configuration:
  const ITERATIONS = 32;
  const DELAY_BETWEEN_RUNS = 0;
  const TESTING_FUNCTIONS = [
    () => browser.tabs.query({}),
    () => browser.windows.getAll({populate: true}),
  ];
  // algorithm:
  async function start() {
    const results = TESTING_FUNCTIONS.map(() => []);
    for (let i = 0; i < ITERATIONS; i++) {
      for (let j = 0; j < TESTING_FUNCTIONS.length; j++) {
        const testingFunction = TESTING_FUNCTIONS[j];
        const time = await withHighResolutionTimer(testingFunction);
        results[j].push(time);
        if (DELAY_BETWEEN_RUNS) await timeoutPromise(DELAY_BETWEEN_RUNS);
      }
    }
    const averageResult = results.map(average);
    console.warn('results: ', results);
    console.warn('average results: ', averageResult);
    return averageResult;
  }
  // common methods:
  const timeoutPromise = delay => new Promise(resolve => setTimeout(resolve, delay));
  const average = arr => arr.reduce( ( p, c ) => p + c, 0 ) / arr.length;
  const HighResolutionTimer = ({
    startNow = true,
  } = {}) => {
    let startTime, stopTime;
    if (startNow) start();
    return {
      start: start,
      stop: stop
    };

    function start() {
      startTime = performance.now();
    }

    function stop() {
      stopTime = performance.now();
      return stopTime - startTime;
    }
  };
  const withHighResolutionTimer = async (fn) => {
    const timer = HighResolutionTimer();
    const result = await fn();
    return timer.stop();
  };
  return await start();
})();

I’ve executed it on my in my FF with 88 tabs /14 windows and the result is more less the same for both functions: [ 23.84375, 23.21875 ].
I would say there is a good chance that internally both API are using the same code.

Regarding keeping data in memory VS in storage - I agree with @jscher2000, definitely in memory to avoid disk write operation, especially if will would happen often.
Also, in this case you don’t need persistent storage and mostly the memory footprint is small - even if you wold store the whole result of the API (if I serialize the result as JSON, it’s 0.7MB, which is nothing considering I’m running 88 tabs).

lionelw · December 7, 2019, 12:48pm

Hmm, you might try both and see which is more accurate.
@jscher2000

You know what? Thanks to this bug #2 would be more accurate! Of course if we work around it, then yes both ways would be identical in outcomes.

Oh man, I love performance questions!
@juraj.masiar

So glad you do! But I think you’ve misunderstood the question. For starters, we want to compare multiple tabs.query({ windowId }) (#1) versus a single windows.getAll({ populate: true }) (#2).

And that’s just the initialisation of the two ways to do this task. #1 will continue to do tabs.query for every tab-count-changing event, while #2 would do so only when a window is created.

I’ve come up with the 2 versions of the addon, to make the difference clear. I’m not sure how to compare overall performance, maybe you can help, @juraj.masiar?

Here’s background1.js for #1:

init();
browser.windows.onCreated.addListener(onWindowCreated);
browser.tabs.onCreated.addListener(onTabCreated);
browser.tabs.onRemoved.addListener(onTabRemoved);
browser.tabs.onDetached.addListener(onTabDetached);
browser.tabs.onAttached.addListener(onTabAttached);

async function init() {
    const windows = await browser.windows.getAll();
    windows.forEach(onWindowCreated);
}

function onWindowCreated(windowObj) {
    updateBadge(windowObj.id);
}

function onTabCreated(tab) {
    updateBadge(tab.windowId);
}

function onTabRemoved(tabId, info) {
    if (info.isWindowClosing) return;
    updateBadge(info.windowId, tabId);
}

function onTabDetached(tabId, info) {
    updateBadge(info.oldWindowId);
}

function onTabAttached(tabId, info) {
    updateBadge(info.newWindowId);
}

// TODO: 'Debounce' updateBadge to prevent multiple redundant tabs.query and browserAction.setBadgeText calls at once.
async function updateBadge(windowId, removedTabId) {
    const text = `${await getTabCount(windowId, removedTabId)}`;
    browser.browserAction.setBadgeText({ windowId, text });
}

// tabs.onRemoved fires too early and the count is one too many. https://bugzilla.mozilla.org/show_bug.cgi?id=1396758
// So removedTabId is passed here from onTabRemoved() to signal the need to work around the bug.
async function getTabCount(windowId, removedTabId) {
    const tabs = await browser.tabs.query({ windowId });
    let count = tabs.length;
    if (removedTabId && tabs.find(tab => tab.id == removedTabId)) count--;
    return count;
}

Here’s background2.js for #2:

let tabCounts = {};
init();
browser.windows.onCreated.addListener(onWindowCreated);
browser.windows.onRemoved.addListener(onWindowRemoved);
browser.tabs.onCreated.addListener(onTabCreated);
browser.tabs.onRemoved.addListener(onTabRemoved);
browser.tabs.onDetached.addListener(onTabDetached);
browser.tabs.onAttached.addListener(onTabAttached);

async function init() {
    const windows = await browser.windows.getAll({ populate: true });
    for (const windowObj of windows) {
        const windowId = windowObj.id;
        tabCounts[windowId] = windowObj.tabs.length;
        updateBadge(windowId);
    }
}

async function onWindowCreated(windowObj) {
    const windowId = windowObj.id;
    // Via listeners, windowObj will not be populated. So we get the tabs here.
    tabCounts[windowId] = (await browser.tabs.query({ windowId })).length;
    updateBadge(windowId);
}

function onWindowRemoved(windowId) {
    delete tabCounts[windowId];
}

function onTabCreated(tab) {
    const windowId = tab.windowId;
    if (isWindowBeingCreated(windowId)) return;
    tabCounts[windowId]++;
    updateBadge(windowId);
}

function onTabRemoved(tabId, info) {
    if (info.isWindowClosing) return;
    const windowId = info.windowId;
    tabCounts[windowId]--;
    updateBadge(windowId);
}

function onTabDetached(tabId, info) {
    const windowId = info.oldWindowId;
    tabCounts[windowId]--;
    updateBadge(windowId);
}

function onTabAttached(tabId, info) {
    const windowId = info.newWindowId;
    if (isWindowBeingCreated(windowId)) return;
    tabCounts[windowId]++;
    updateBadge(windowId);
}

function isWindowBeingCreated(windowId) {
    return !(windowId in tabCounts);
}

async function updateBadge(windowId) {
    const text = `${tabCounts[windowId]}`;
    browser.browserAction.setBadgeText({ windowId, text });
}

Here’s the manifest.json, swap "background1.js" with "background2.js" as needed:

{
  "manifest_version": 2,
  "name": "Tab Counter",
  "version": "0.0.1",
  "background": {
    "scripts": ["background1.js"]
  },
  "browser_action": {}
}

Notice that in background2.js, we account for multiple events triggering from one action (e.g. detaching a tab also creates a window) using isWindowBeingCreated. This is necessary to produce correct outcomes.

I can’t quite figure out how to do the same in background1.js – which means multiple redundant tabs.query calls – but outcomes will still be correct. We should, if we want a fair performance comparison…

Edit: I guess we can probably use the same isWindowBeingCreated technique instead of coming up with a ‘debouncer’… I’ll try it out tomorrow. Sorry it’s late… Need sleep~

jscher2000 · December 7, 2019, 8:14pm

Hmm, luckily I wasn’t calling tabs.query on tabs.onRemoved in my own extension or I probably would have been pulling my hair out.

In your extension, do you also need to update the badge in windows.onFocusChanged?

lionelw · December 7, 2019, 11:57pm

Doesn’t seem necessary. If all the relevant events are taken care of, you could tile multiple windows and see accurate changes in any unfocused windows.

lionelw · December 8, 2019, 12:58pm

Updated background1.js:

let openWindowIds = new Set();
init();
browser.windows.onCreated.addListener(onWindowCreated);
browser.windows.onRemoved.addListener(onWindowRemoved);
browser.tabs.onCreated.addListener(onTabCreated);
browser.tabs.onRemoved.addListener(onTabRemoved);
browser.tabs.onDetached.addListener(onTabDetached);
browser.tabs.onAttached.addListener(onTabAttached);

async function init() {
    const windows = await browser.windows.getAll();
    windows.forEach(onWindowCreated);
}

function onWindowCreated(windowObj) {
    const windowId = windowObj.id;
    openWindowIds.add(windowId);
    updateBadge(windowId);
}

function onWindowRemoved(windowId) {
    openWindowIds.delete(windowId);
}

function onTabCreated(tab) {
    const windowId = tab.windowId;
    if (isWindowBeingCreated(windowId)) return;
    updateBadge(windowId);
}

function onTabRemoved(tabId, info) {
    if (info.isWindowClosing) return;
    updateBadge(info.windowId, tabId);
}

function onTabDetached(tabId, info) {
    const windowId = info.oldWindowId;
    updateBadge(windowId);
}

function onTabAttached(tabId, info) {
    const windowId = info.newWindowId;
    if (isWindowBeingCreated(windowId)) return;
    updateBadge(windowId);
}

// To prevent redundant updateBadge calls when an action triggers more than just windows.onCreated.
// Relies on the expectation that windows.onCreated is always triggered last.
function isWindowBeingCreated(windowId) {
    return !openWindowIds.has(windowId);
}

async function updateBadge(windowId, removedTabId) {
    const text = `${await getTabCount(windowId, removedTabId)}`;
    browser.browserAction.setBadgeText({ windowId, text });
}

// tabs.onRemoved fires too early and the count is one too many. https://bugzilla.mozilla.org/show_bug.cgi?id=1396758
// So removedTabId is passed here from onTabRemoved() to signal the need to work around the bug.
async function getTabCount(windowId, removedTabId) {
    const tabs = await browser.tabs.query({ windowId });
    let count = tabs.length;
    if (removedTabId && tabs.find(tab => tab.id == removedTabId)) count--;
    return count;
}

Question still remains is there a big difference in performance between the two solutions, #1 with more API calls and #2 with fewer (which walks hand-in-hand with the question of how expensive the API calls are).

juraj.masiar · December 10, 2019, 8:05pm

Well, after seeing both versions, I would say, screw performance, go with the less complex solution! So the first one .

The difference will be too small anyway… it’s better to stay with easier maintainable code.
It’s all asynchronous anyway, so it shouldn’t really slow down your browser at all because it will run on some idle CPU .