Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that TreeStyleTab does not bloat the session (to the point that it will not be loaded by Firefox). #3388

Open
dahopem opened this issue Sep 21, 2023 · 20 comments

Comments

@dahopem
Copy link

dahopem commented Sep 21, 2023

Abstract

Steps to reproduce

  1. Start Firefox with clean profile.
  2. Install TreeStyleTab.
  3. Create a large session with 1800 tabs (e.g. clustered into 18 windows with 100 tabs each). Ensure that each tab has a favicon whose data size is 90 KB (e.g. a favicon in the verbose SVG format)
  4. Kill the browser process.
  5. Start the browser again as a new process.
  6. Try to restore the session.

Expected result

Expect that the session will be restored successfully.

Actual result

Observe that restoring the session fails.

Environment

  • Version of Firefox: 116.0
  • Version (or revision) of Tree Style Tab: 3.9.17

Analysis

TreeStyleTab will, for each window, add an entry "extension:treestyletab@piro.sakura.ne.jp:cached-tabs" as well as an entry "extension:treestyletab@piro.sakura.ne.jp:cached-sidebar-contents" into the "extData" section of the window's section of the JSON interpretation of the decompressed sessionstore file (e.g. "/sessionstore-backups/previous.jsonlz4" or "/sessionstore-backups/recovery.jsonlz4" within the Firefox profile's directory). These entries have string values of huge length. The huge length stems from each of the string values containing basically the complete tab structure information of the particular window again, and in particular containing a JSON-encoding of data which, in turn, contains one or multiple base64-encodings of the favicon data. The "extension:treestyletab@piro.sakura.ne.jp:cached-tabs" value contains the base64-encoded favicon data one time per tab, the "extension:treestyletab@piro.sakura.ne.jp:cached-sidebar-contents" value contains the base64-encoded favicon data thee times per tab. The resulting session data exceeds the maximum string size of the JavaScript environment of Firefox (currently 1073741822 bytes), triggering a failure at decompressing the sessionstore file.

This is possible due to the following:

Each favicon data is base64-encoded, resulting in a 120 KB string per favicon. Each favicon is not stored 1 time (as intended), but at least 5 times (within the "extension:treestyletab@piro.sakura.ne.jp:cached-tabs" value 1 more time as well as within the "extension:treestyletab@piro.sakura.ne.jp:cached-sidebar-contents" 3 more times), consuming at least 600 KB per tab. For 1800 such tabs, the sessionstore file has a size of at least 1080000000 bytes, which exceeds the maximum string size of the JavaScript environment of Firefox, leading to the unusability of the stored session.

@irvinm
Copy link
Contributor

irvinm commented Oct 1, 2023

This looks to be a continuation of #1907 albeit with a different focus.

piroor added a commit that referenced this issue Oct 4, 2023
@piroor
Copy link
Owner

piroor commented Oct 4, 2023

Here is an experimental branch with a change to compress session data: https://github.com/piroor/treestyletab/tree/compress-session-data
Development build: https://github.com/piroor/treestyletab/actions/runs/6409046697

I've experimentally implement compression/decompression of the cache. There is some demerits: compression and decompression require CPU power and time. On my case it takes 3sec to compress/decompress sidebar cache with 100 or more tabs. Could you try the build with large number of tabs and its effectivity? @dahopem

@piroor
Copy link
Owner

piroor commented Oct 4, 2023

And I think we need to migrate for better performance, from the present raw DOM tree UI with large number DOM nodes to a virtual scrolling UI with less DOM nodes...

piroor added a commit that referenced this issue Oct 4, 2023
@piroor
Copy link
Owner

piroor commented Oct 5, 2023

Worse still, the decompression can require more time while the startup process. On my case, decompression result is returned after 30 seconds when I reloaded TST itself - this means that the initial paint of the sidebar is delayed 30 seconds. Storing cache to the session data was originally introduced to speed up the initialization process, so this slowing down is quite ironic.

@piroor
Copy link
Owner

piroor commented Oct 5, 2023

Hmm, compression result looks surely effective to reduce storage usage. On my case, the data size of the sidebar cache with 173 tabs is reduced from 7.7MB to 2.9MB - the delta is 4.8MB. It will be about 50MB if I use 1800 tabs.

On the other hand, the compression finishes within 1.5 sec on my case. It will take more time with 1800 tabs.

@piroor
Copy link
Owner

piroor commented Oct 5, 2023

I've added an option to activate/deactivate data compression for the session data and merged changes to the trunk.

@dahopem
Copy link
Author

dahopem commented Oct 5, 2023

Note that the compressed data will be compressed again (by the mozilla lz4 compression). I would guess that adding compression may also substantially increase the duration of the periods during which Firefox will be unresponsive due to storing the session (which can be quite significant for large sessions). Adding compression may also increase the energy consumption, which is quite relevant on mobile devices (including notebook computers). Due to Firefox quite aggressively storing the session again and again, this may be quite relevant. Thus, compression seems to be just a workaround, not a true fix.

Is it possible to deactivate the cache entirely? If it is just a cache, then losing the cached data should not have an effect on the correctness of the behavior of TST, it should just have an effect on the session load duration. This way, the energy consumption and storing duration would probably be substantially reduced (because there is just less data to be processed), which may more than offset the increase of the session load duration due to not having such a cache.

@piroor
Copy link
Owner

piroor commented Oct 5, 2023

Note that the compressed data will be compressed again (by the mozilla lz4 compression). I would guess that adding compression may also substantially increase the duration of the periods during which Firefox will be unresponsive due to storing the session (which can be quite significant for large sessions). Adding compression may also increase the energy consumption, which is quite relevant on mobile devices (including notebook computers). Due to Firefox quite aggressively storing the session again and again, this may be quite relevant. Thus, compression seems to be just a workaround, not a true fix.

Ah... you are right, I totally misunderstood the point. Thanks.

You can deactivate storing sidebar contents cache: TST options => Advanced => Optimize tree restoration with cache.

@valadaptive
Copy link

valadaptive commented Nov 22, 2023

There seems to be another big problem with the tree cache: automatic session backups take a really long time.

Firefox schedules session backups to occur when the browser is "idle", which happens roughly every 20 seconds. Every time this happens, the browser spends almost 2 seconds frozen because writing a single really big string into JSON is quite slow. Here's a profile where you can see the slowdown that occurs. This results in frequent 2-second lagspikes.

How feasible would it be to remove the cache entirely, or at least provide a warning in the settings that it could slow things down with large numbers of tabs? It seems misleading to have an option that claims to "optimize" a user's experience but ends up causing their browser to hang every 20 seconds.

What are the main bottlenecks that make restoring the cache faster than not doing so?

piroor added a commit that referenced this issue Nov 22, 2023
@piroor
Copy link
Owner

piroor commented Nov 22, 2023

I've introduced an experimental change 23a25a1: now TST stores "cache" to the on-memory storage (not persisted across browser sessions). This means that the size of the session storage persisted to the disk is reduced and the initialization performance at the browser startup becomes worse. After more dogfooding I'll decide that the option is enabled by default or not.

@yorickvP
Copy link

yorickvP commented Jan 4, 2024

I was wondering where the regular lag was coming from. My session file was 80MB uncompressed with ~800 tabs open. Firefox was lagging 0.5s every 20s.
I disabled "Optimize tree restoration with cache" and now my firefox is way faster. It also starts noticeably faster. I'm not sure if the cache feature is useful at all around these tab counts.

@valadaptive
Copy link

I've since disabled the cache as well, but:

I've introduced an experimental change 23a25a1: now TST stores "cache" to the on-memory storage (not persisted across browser sessions).

I think the main bottleneck is in serializing the session to JSON, not writing it to disk. The huge lag spikes are thus not fixed by keeping the cache in memory.

@valadaptive
Copy link

I've started experiencing huge 2-second lag spikes from writing JSON again, even with the cache disabled. Has the session cache code been modified?

@piroor
Copy link
Owner

piroor commented Jan 24, 2024

See also: #3434

@piroor
Copy link
Owner

piroor commented Jan 24, 2024

To @dahopem and other people:
A change to solve this issue introduced at TST 3.9.20 caused a performance regression on some environments, and I've created three different development versions. They use different approach for each, and some may re-introduce the performance issue reported here. Could you cooperate to try development versions and find out most effective and safest choice?

@valadaptive
Copy link

Hi,

I've taken a quick look at #3434, and it looks like it doesn't apply here.

I'm experiencing lag spikes with "Optimize tree restoration with cache" completely disabled-- there is no cache.

@github-actions github-actions bot added the stale no reaction got for a long term label Feb 5, 2024
Copy link

github-actions bot commented Feb 5, 2024

This issue has been labeled as "stale" due to no response by the reporter within 1 month (and 7 days after last commented by someone). And it will be closed automatically 14 days later if not responded.

@piroor piroor removed the stale no reaction got for a long term label Feb 5, 2024
@github-actions github-actions bot added the stale no reaction got for a long term label Feb 6, 2024
Copy link

github-actions bot commented Feb 6, 2024

This issue has been labeled as "stale" due to no response by the reporter within 1 month (and 7 days after last commented by someone). And it will be closed automatically 14 days later if not responded.

@piroor piroor removed help wanted stale no reaction got for a long term labels Feb 7, 2024
@piroor
Copy link
Owner

piroor commented Mar 25, 2024

FYI: TST 4.0.x is now available and it never saves such large data to the session.

@wsmwk
Copy link

wsmwk commented Oct 30, 2024

@dahopem ? #3388 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants