Closed Bug 1371255 Opened 7 years ago Closed 5 years ago

Very bad memory efficiency of browser.storage.local

Categories

(WebExtensions :: Storage, defect, P1)

defect

Tracking

(firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 verified)

VERIFIED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- verified

People

(Reporter: jwkbugzilla, Assigned: rpl)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(4 files)

I just had a remote debugging session with a user who complains about Adblock Plus 2.9.1 being completely broken for them. Turns out, this user misconfigured Adblock Plus by adding way too many filters. However, while Adblock Plus 2.8.2 (using direct file access) wasn't dealing too badly with a single file containing 34 MB of data, Adblock Plus 2.9.1 (using browser.extension.storage) errors out or crashes with an "out of memory" error when saving data. This being a 32-bit Firefox build (Firefox 52 ESR and Firefox 55 Nightly on Windows) on a machine having plenty of swap memory, my conclusion is that there is a memory spike going beyond 3 GB. At the same time, as long as the data merely stays in memory the memory usage stays below 400 MB (that's the total memory usage of the Firefox process, not merely Adblock Plus).

Note on the data structure: Adblock Plus uses browser.extension.storage as a replacement for plain files, so the data is a plain array containing all the lines that would have been written to the file.
Flags: needinfo?(amckay)
Attached image storage-local-set.PNG
Crap comment 1 wasn't helpful was it, sorry about that. 

I'd be curious to see how this is now that bug 1370752 has landed. That's significantly improved the time spent on storage.local.set. Telemetry is at https://mzl.la/2tglhcl, but I've attached an image of it this work.
Flags: needinfo?(amckay)
A user of uBlock Origin reported having a similar undue memory usage issue in relation with browser.storage.local[1] Below I reproduced relevant excerpts:

> This happens only on 32builds of Firefox even with default filter, just when the total
> size of filters or storage reaches around 30-35 mb, uBo or Firefox reverts storage size
> to 1kb [...] RAM/Disk/CPU usage goes as high as 700-1100mb during this
> process(have 4gb as page file too) so memory limitation should not be a problem but it
> looks that way? no other 32 bit apps have memory issues if page file is high enough but
> here looks like Firefox/uBo api is the problem ? That's why no issues in 64bit version &
> this issue should not be happening!
> ...
> Updating filters in background hanks the UI and you can see that in ram/disk/usage under
> task manager, it goes as high as 1.8gb(increased page file to25gb just to test and no
> other apps have have memory issues but here it is) &
> this is with the default lists + ignore cookies list.

In a previous comment[2], the user reported having error conditions thrown at the browser console

> Just a PSA to stop pushing beta update 1.13.11rc0.webext to users as it might break
> their settings
> 
> [test setup=32bit lin/win10 Asus X552EA-SX009D Laptop (APU Dual Core- 2GB ram) with FF57]
> 
> the webext in beta 1.13.11rc0 on AMO
> breaks filters/settings if user has a custom list added
> 
> ERRORS:
> 
>     Failed to serialize browser.storage key
>     "cache/https://www.kiboke-studio.hr/i-dont-care-about-cookies/abp/":
>     TypeError: > JSON.serialize is not a function ExtensionStorage.jsm:60
> 
>     Failed to serialize browser.storage key
>     "cache/disconnect-malware":
>     TypeError: JSON.serialize is not a function > ExtensionStorage.jsm:60

[1] https://github.com/gorhill/uBlock/issues/2684#issuecomment-325197542
[2] https://github.com/gorhill/uBlock/issues/2684#issuecomment-325178595
I see the issue of comment 3 in Firefox 55.0.3 x86.

I see the "out of memory" reason on https://hg.mozilla.org/releases/mozilla-release/annotate/710711a481286f27b81df2e4b082d48829510633/toolkit/components/extensions/ExtensionStorage.jsm#l132 in Browser Console.


It looks like this issue has been fixed by Bug 1370752, I see the Firefox 56 releases memory from time to time rather than sustaining growth. But Bug 1370752 actually landed on Firefox 56, may request a uplift?
Depends on: 1370752
Keywords: dataloss, perf
(In reply to YF (Yang) from comment #4)
> I see the issue of comment 3 in Firefox 55.0.3 x86.
> 
> I see the "out of memory" reason on
> https://hg.mozilla.org/releases/mozilla-release/annotate/
> 710711a481286f27b81df2e4b082d48829510633/toolkit/components/extensions/
> ExtensionStorage.jsm#l132 in Browser Console.
> 
> 
> It looks like this issue has been fixed by Bug 1370752, I see the Firefox 56
> releases memory from time to time rather than sustaining growth. But Bug
> 1370752 actually landed on Firefox 56, may request a uplift?

Comment 3 bug by R.Hill and OOM bug in comment  Description by Wladimir Palant

I just reproduced both bugs in Linux x32 with Firefox 57(20170827) & windows 10 (RS2)
So IDK if it's fixed in "Firefox 56" and regressed in Firefox 57 or the problem is different altogether.
This seems like a major regression to me in terms of perf when compared to old Sq-lite DB storage which was used in "Legacy"addons .

Like mentioned in comment 3
1: can see the HIGH resource usage when compared to non-web-extensions which should not be and should be a high priority.
2: New storage system does not use append or writing in chunks to disk like legacy versions or has the HIGH performance that it had.
3: The performance should be substantially better when compared to old storage, Cpu, memory and disk usage should be far less compared to them

NI: Cris for not fixed by Bug 1370752 & Andy for further info about this.

YF (Yang) It's still happening in Today's build so IDK if the bug you linked fixes this or the performance levels are even any where near the old one. Can you please recheck?
Flags: needinfo?(yfdyh000)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(amckay)
Just tested with chrome 62 x86 and it does not have any issues even with 100mb of data.

Firefox 52 with old storage also has no issues whatsoever .

Now did some more testing with all three on a laptop with 4gig of memory and 64 bit os,

Chrome started and loaded the filters very efficiently with CPU~10%, while updating filters the ram,cpu & disk usage was very low too

Firefox 52 started and loaded fairly quickly, while updating and writing of filters(110mbish just to test) the disk usage and ram were pretty low with cpu usage around ~20%,

Now when comparing these with Firefox 57 somethings were *very worrying*

During reading of filters chrome and FF52 were very easy on resources while

FF57 during reading from storage for about a good few seconds with 100% disk usage and ram jumping from 300mb to 800mb them coming back down to 349mb.
Chrome ram usage was 310mb during the same, FF52 was 269 mb and both did not show high Disk or ram usage,

In FF57 while updating the Disk was being used 100% unlike chrome & FF52 which were ~5%.
Ram usage also jumped during writing of filters upto 1600mb, chrome and FF52 were around 300-350mb mark,

FF57 *DATA-LOSS* occurred but not in the other two.

With 64 bit FF57 the results were same and performed very badly as above except the data loss did not occur.

The New STORAGE system is severely inefficient and with Firefox improving things recently in all over the place,
This is one area where it has *regressed severely*.

The old storage was far better and was performing nearly close to chrome in some cases and beating it in others.

Why is storage.js being used instead of SQlite.db/SQlit3.db ? Aren't the latter more performative in terms of speed and compression  than the former?

Are there any automated benchmarks to compare them to see how badly they have regressed?
I've run two profiling sessions with uBlock Origin ("uBO") and Firefox 55.0.3.

The first profiling session was compare startup time of legacy version versus webext version. Results are in attachment in comment 7. The scenario used focus more on the use of browser.storage.local.get.

Observations: the time spent in uBO's own code is pretty much the same in both profiles (see "onAssetRead"). However there is a significant chunk of extra work done in the webext version which is not present in the legacy version. Most of that time -- 621 ms -- is spent in resource://gre/modules/ExtensionStorage.jsm, line 121 and line 108. I believe it's related to the use of TextEncoder, which purpose is to encode/decode in-memory the data being written/read through browser.storage.local.


The sedon profiling session was to compare a scenario in which uBO had to write to the storage using browser.storage.local.set. Results are in attachment in comment 8.

Observations: Again, as in the first scenario, the code in resource://gre/modules/ExtensionStorage.jsm seems to be the main culprit, the extra overhead is ~1.2 second this time compared to the legacy version, and again it appears that TextEncoder is the chokepoint. Using a TextEncoder, which encode/decode in-memory the data to be saved is the most likely explanation to the memory spikes observed by users and large amount of data is read/written through the browser.storage.local API.

I did not profile with Firefox 57, in which I can see the use of TextEncoder/TextDecoder has been removed apparently.

The issue here is concerning for uBlock Origin, since there is a webext (embedded) version (1.13.10) on the verge of approval on AMO, which means the reported issue here is likely to start to affect a lot of users once it becomes available on AMO.
Typos in comment 9:

- "Firefox 55.0.3" => "Firefox 55.0.2".
- "The sedon profiling" => "The second profiling".
If you want resource efficiency, use IndexedDB. storage.local is not designed for storing huge amounts of data.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(amckay)
(In reply to YF (Yang) from comment #12)
> (In reply to hulmelo from comment #5)
> > YF (Yang) It's still happening in Today's build so IDK if the bug you linked
> > fixes this or the performance levels are even any where near the old one.
> > Can you please recheck?
> 
> Not sure what you mean, just report the "out of memory" error in Fx55 and
> Fx56 has fixed it.

Just reproduced it on x86 linux builds,
So not completely fixed.

If you can test x86 builds again to see if you can verify.
(In reply to Kris Maglione [:kmag] from comment #11)
> If you want resource efficiency, use IndexedDB. storage.local is not
> designed for storing huge amounts of data.

If we really want to say that, then this needs to be documented. This is going to be surprising for a lot of developer coming from Chrome. I am probably going to look into what can be optimized here around next week or so.
Flags: needinfo?(evilpies)
(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #14)
> (In reply to Kris Maglione [:kmag] from comment #11)
> > If you want resource efficiency, use IndexedDB. storage.local is not
> > designed for storing huge amounts of data.
> 
> If we really want to say that, then this needs to be documented. This is
> going to be surprising for a lot of developer coming from Chrome. I am
> probably going to look into what can be optimized here around next week or
> so.

Chrome already documents this:

https://developer.chrome.com/extensions/storage#limits

so anyone implementing for Chrome first should already be wary of using these APIs in this way.
> https://developer.chrome.com/extensions/storage#limits

I read the "chrome.storage is not a big truck" as a limit on throughput, not on size (or then what exactly is the point of "unlimitedStorage" permission?) Quoting that Chrome doc just seems a convenient way to shift the blame on extension developers for not having been clever enough to foresee the issue here.

I do now have a fix with indexedDB -- seems to work fine, but I have to say now I am skittish about this, we might just be told sooner than later that extension developers should have foresee that indexedDB is not really a good solution after all.[1]

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1313401#c8
https://github.com/gorhill/uBlock/issues/2925

 uBO is not allowed to use indexedDB at all in private mode


Why is this force made?

Why cannot users use their addons/extensions as they want?
Isn't this against what Mozilla stands for?

Why is SQlite not allowed to be used if it performs better?
and data to be stored in single file like ubo.sqlite instead of being forced to
save extension data in various places when using index db
\browser-data-extension\name of extension\ settings
and
\storage\default\extension-name\.DB+millions other files.....

This issue should be higher up in line to be addressed!
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
(In reply to hulmelo from comment #17)
> This issue should be higher up in line to be addressed!

See comment 14. 

Given the "unlimitedStorage" thing Firefox will probably need to handle this better if we're expecting people to port stuff over from Chrome.

>Why is SQlite not allowed to be used if it performs better?

Because it exposes internal implementation specifics that will break if we update, and avoiding this is exactly the point of WebExtensions and Web APIs in general.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #18)
> (In reply to hulmelo from comment #17)
> > This issue should be higher up in line to be addressed!
> 
> See comment 14. 
> 
> Given the "unlimitedStorage" thing Firefox will probably need to handle this
> better if we're expecting people to port stuff over from Chrome.
> 
> >Why is SQlite not allowed to be used if it performs better?
> 
> Because it exposes internal implementation specifics that will break if we
> update, and avoiding this is exactly the point of WebExtensions and Web APIs
> in general.

But look at the issues it's causing especially x86 or 32 builds

 just see this

https://github.com/gorhill/uBlock/issues/2934

the above link has an extension using "index db"
the recommended method to use

look at the performance profile provided there. 

better install 32 bit build & use this

https://github.com/gorhill/uBlock/releases/download/1.14.0/uBlock0.webext.xpi

load multiple pages and see for yourself , sure you will see the issues.

please just try once.

Is someone looking to find the problem? Just to see what the issue might be?
FF 32bit has a large user base & if these problem occur on it , how will the users react?
Do you think they will care what is causing the issue or just close and switch to chrome?
Description also states 32 builds,
Clearly something wrong in x86 builds or storage sub system
Priority: -- → P1
https://bugzilla.mozilla.org/show_bug.cgi?id=1371255#c19

Can successfully reproduce the issue on windows10 with Firefox Nightly 57 with 32bit builds.

Tried to reproduce once the same issue in Linux 32 bit but did not succeed.

Using the .xpi can see during at startup with it enabled for a good 30-60 seconds the cpu usage is very high,
page loading with it seems to never end , opened 10 pages which did not finish loading even after 3 minutes.
in linux did not face the issue in the solo try but will try to check again.

Now this is semi related to this: BUT also Happens in Linux 32bit

older version of the addon used ubocl0.sqlite file to store the DB,
now since it switched to IndexDB api it saves in storage folder but the files are not kept in one file but scattered,
which is not ideal, it should just be like in old extensions one db file for all things , not multiple.
It will create a huge mess and more chances of error in future.

Earlier that ublock0.sqlite was 15mb,
in storage.js? It grew to 33mb,
When using INDEXdb it's right now at 94mb!!

**from 15mb to 94mb?**

did a clean install to re-verify = same
why such a huge difference?
Note not counting the size of other files in the storage folder just the big.db

Secondly it looses the updates, just turn off auto update do a manual update of filters, restart FF a few time see in settings filters updates are lost.

Thirdly the the ABP issue is *reproducible* in 32 bit version of Firefox Nightly 57.

Please feel free to ask anything if you have problems reproducing it.
Sorry forgot to add

The IndexDB version sometime looses the update & Doesn't update too unless you re-install it or delete

storage\default\The folder of addon

& local-storage version cannot go above 40mb in both ABP+UBO+32bit Firefox,
to test just add many filters in ublock or ABP & update,
see the size of storage.js grow and reset somewhere b/w 33mb-40mb and data lost.

Tested on windows 10 redstone 2 32 bit with 32bit Firefox
Ubuntu 32 bit with 32 bit Firefox
Alright got time to test 32bitlinux builds,

STR:

1: add ubo
2: add many or all filters.
3: disable auto update of filters.
4: purge & update.
5: restart & go into dashboard & then filters


You can see many filters are now out of date again!
Try updating them, they never update although spinners keeps spinning.

Something is fundamentally wrong with 
IndexDB & Browser storage api's on 32 bit builds.

This should be a critical bug.
https://github.com/gorhill/uBlock/issues/2684

according to the dev it's aFirefox issue too
@ hulmelo & Firefox_Ninja - please avoid using multiple accounts to give yourself more credibility, thanks ;)
Severity: normal → major
Keywords: footprint, topperf
OS: Unspecified → All
Hardware: Unspecified → All
(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #25)
> @ hulmelo & Firefox_Ninja - please avoid using multiple accounts to give
> yourself more credibility, thanks ;)

Ty for setting the severity & l=the lined bug.

and delusional you are for thinking that.
Any news ?

For the time being switched to

https://downloads.adblockplus.org/devbuilds/adblockplusfirefox/adblockplusfirefox-2.99.0.1832beta.xpi

It's using storage.js and really close to hitting the 30mb size error but performs better that indexDB which with same filters is 80mb uBo is using and slowing startup pageloading.
Hi guys, new to this so sorry in advance.
I am having the issues mentioned here but got a workaround for now,
in ublock disable cosmetic filtering and ignore generic filters seems to Speed up the page loading a bit.
But CPU and Disk usage will be high still.
Hopefully this gets fixed soon.
Feels like 32bit has regressed badly recently in terms of performance and webextensions,
can some one guide on how to do a benchmark between builds to find when it regressed?
Hi, this issue is causing a really really bad memory problem for us on New XKit as compared to the Chrome implementation of the same webextension. We're working on mitigating some of the worst effects of this on our end, but this is causing memory usage for a simple page (http://merrimonster.tumblr.com/) to balloon to multiple gigabytes compared to less then a dozen megabytes for the same page on chrome.

Reproduction Steps:

 1. Download XKit 7.8.0 (pure webextension) from https://github.com/new-xkit/XKit/releases/tag/v7.8.0
 2. visit https://tumblr.com to let xkit go through the initial setup process (you don't need to have a tumblr account)
 3. visit http://merrimonster.tumblr.com/ and scroll through a couple pages.

Doing this in Firefox leads to extremely high memory usage compared to the same implementation on Chrome (with a complex config, I hit 13 gigabytes vs ~300MB on chrome. less complex configs show similar ratios: 400 megabytes on FF vs a couple of megabytes on chrome, for example). After GC this comes down to a more reasonable size, but even GCing 13 gigabytes of memory is a gargantuan task. This is just the memory usage for the single content process.

We're going to release an hotfix today (7.8.1) that mitigates this by ignoring some iframes for content script injection, but this is a compromise for us functionality-wise and doesn't solve the root inefficiencies we're having with browser.storage.local.

Since this is currently a P1 bug, can it either get someone assigned to look into it or downgraded to a P2?
Flags: needinfo?(amckay)
Flags: needinfo?(evilpies)
Here are some measurements of what nightpool is talking about. The memory reports are collected before and after visiting https://findoworld.tumblr.com/post/165889203818 with New XKit 7.8.0 enabled. This page has around 14 iframes into which Firefox injects the extension's content scripts.

The brief summary of the about:memory diff is that there's a 1GB increase of which 38% is `dom/structured-clone-holder`, 26% is `js-non-window`, and 24% is `window-objects`.

I'm available on IRC as hobinjk if you want more information.
It would be interesting to see what that memory report looked like after GC.

But part of the problem seems to be that XKit creates tons of copies of its JSON-stringified settings objects for each document that its content script is loaded into. I don't really understand why it needs to JSON stringify at all, and I'm not clear on why so many copies are made, but it seems excessive.

And several of those strings look like databases... not even mini databases, just databases... that would be better stored in indexedDB.
>But part of the problem seems to be that XKit creates tons of copies of its JSON-stringified settings objects for each document that its content script is loaded into. I don't really understand why it needs to JSON stringify at all, and I'm not clear on why so many copies are made, but it seems excessive.

I mean, the root cause is that we're dealing with a large legacy code base written well, well before storage.local was a thing Firefox extensions could use. Before WebExtensions, we stored browser settings in preferences. We can't change the API we provide to XKit scripts, so when the Chrome version was written it was written to accommodate the limitations of the preferences API. This has been the way XKit has worked since ... 2012? 2013? As to why many copies are made, it's because XKit is **only** a content script. So each document is a fully independent instance. 

While there are great reasons for us to migrate to other types of storage systems, it would be a large, large investment of effort for a 50,000 line project that hasn't seen much significant structural change since 2014.

Again, I'm more interested in figuring out why this code works fine on Chrome but consumes memory usage exponentially under Firefox. The 150 MB of string allocation seems to be a very small part of that.

I've attached a chrome allocation timeline for a XKit page load with many small frames. (Actually, 2 page loads in the same timeline). I have a very large storage that I use mostly to test bugs like this, approximately 6MB. An average storage is closer to 2 or 1 MB. As you can see, with my 6MB storage, Chrome made about 300MB of allocations within the period of the page load. I don't have measurements of this for obvious reasons, but while loading the same page Firefox reached 13GB of memory usage. As far as I can tell, the memory profiler stops working around 1.5GB, so I was unable to diagnose the issue further.
Timeline wouldn't upload as an attachment, so here it is: http://nightpool.club/chrome-blog-load.heaptimeline
Moving the backend of storage.local to indexedDB is being tracked in bug 1406181. We'll prototype that to see what the memory usage is on these use cases is.
Flags: needinfo?(amckay)
Assignee: nobody → lgreco
I wonder whether an immediate fix on user side is possible, like e.g. downgrading to previous version of AdBlock or FF? I am suffering from the current behavior a lot since FF ones in a while (30 min) goes in "Not responding" mode for 1 Minute making a lot of memory activity as seen from consumed memory vs. time in Task Manager. The same 1-min-hang-up happens every time after I've edited AdBlockFilters.
You can install our development build from https://addons.mozilla.org/addon/adblock-plus/#beta-channel - it should perform considerably better, but the user interface is currently much less advanced.
Hi Wladimir,

Thanks! I have installed a development build. The problem still persists, but I have impression that it is not that severe anymore -- the plots in Task Manager look less wild.

About UI -- right. I will write directly to you as this is off topic here.
Component: WebExtensions: General → WebExtensions: Storage
See Also: → 1406181
Depends on: 1277612
Product: Toolkit → WebExtensions
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Affects Firefox for Android]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

[Tracking Requested - why for this release]:

This should be fixed. If this is still an issue with 66 or greater, please reopen.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Verified using Firefox 66.0.2 (64-bit and 32-bit) and 57.0.4 (32-bit) running on Windows 10 x64 bit, the issue is no longer reproducible and appears fixed on 66 but I did manage to reproduce it on the earlier version. I've also compared memory and CPU usage using the same scenarios and extensions with Google Chrome, results showing similar or in some cases better performance on Firefox.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.