Closed Bug 968272 Opened 6 years ago Closed 6 years ago

use less conservative temporary storage policy in QuotaManager

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [games][qa-])

Attachments

(1 file, 2 obsolete files)

The QuotaManager currently uses this calculation to determine the maximum amount of temporary storage:
  http://mxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManager.cpp#854
Furthermore, a given group (roughly, eTLD+1) can use at most 20% of this.  With the current coefficients, even if you have 4GB free, a single group gets only 75MB.  For large asm.js codes (esp. debug/unminified), this can be too small for even a single cache entry to fit, effectively disabling caching.

I'll not suggest which individual coefficients to tweak but just that a desirable endstate, from my perspective, is that, as long as you have more than a 1GB of available disk space, a single group should be able to store at least 200MB.
I agree that the limits we have right now are too low.

Here's one proposal:

totalTempStorage = cap(freeDiskSpace * .30, 0, 50GB)

x = cap(totalTempStorage * .20, 10MB, 300MB)
maxGroupStorage = cap(x, 0, totalTempStorage)

In other words, we'd allow temporary storage to use 30% of free disk space, but at most 50GB.

Each "group" (i.e. eTLD+1) is allowed to use 20% of that, but always at least 10MB and at most 300MB. Though the 10MB minimum limit still wouldn't allow it to grow beyond the 30% of free disk space.

This means that a website can write 300MB of crap to your HD. But only if you have 5GB of free storage.
That sounds good enough to me.

Just to make sure: by freeDiskSpace, you mean what GetTemporaryStorageLimit calls availableKB (which is GetDiskSpaceAvailable()+aCurrentUsage), not GetDiskSpaceAvailable(), right?  (Otherwise, the act of consuming temporary storage would decrease totalTempStorage.)
Keywords: dev-doc-needed
Well, sounds great!  Any takers?

Also: there is a FFOS game we'll be demoing at MWC that generates a 34MB cache file.  With 1GB free, that cache file gets rejected with the current quota heuristics (so we're setting dom.quotaManager.temporaryStorage.fixedLimit at the moment).  The proposal in comment 1 would give us 61MB which should be enough, so could we also use the same policy on mobile?
Summary: use less conservative temporary storage policy in QuotaManager on desktop → use less conservative temporary storage policy in QuotaManager
Assignee: nobody → Jan.Varga
see also bug 965532
See Also: → 965532
I think this is changes is needed. If you take a look at the bug 965532 I have filed, I suggested that the mechanism will work as it's working in chrome (see description in bug).
As a web developer, It would be really helpful not to have a limit of 300mb per domain on temp storage. Since the storage is temp anyway, if my data is too old it will be deleted anyway by other web apps so I can't really "pollute" the user computer.

Also, I think it will be useful to have consistency with chrome development so that it will not be too **** cross browser implementations.

What do you guys think?
Any news in this bug?
Any chance this is close?  If you're too busy, I could take a stab at this.
Flags: needinfo?(Jan.Varga)
(In reply to Luke Wagner [:luke] (on partial PTO) from comment #9)
> Any chance this is close?  If you're too busy, I could take a stab at this.

Ok, feel free to write a patch, I'll review it.
Flags: needinfo?(Jan.Varga)
Excellent!  First, a request for a bit of guidance: should all the smart limits remain?  Also, which tests do should be updated?
Hi, since there's no exact spec for this, it would be extremely helpful for developers to have a concise logic with Chrome's quota management.
i.e: For each app give 10% out of user's free disk space.
For more info: https://developers.google.com/chrome/whitepapers/storage#temporary
What we'd have with comment 1 is fairly similar to the situation with Chrome, except (1) FF would have lower/upper bounds on global/group limits, (2) Chrome hands out 50% of free disk space instead of 30%.

Jonas: could we use numbers closer to Chrome?  The particular limit that makes me worried people will unnecessarily hit on FF is the 300MB group upper bound.
Flags: needinfo?(jonas)
Attached patch tweak-temporary-quota (obsolete) — Splinter Review
This issue has become a priority with a to-be-released asm.js partner demo, so I'd like to move this along somewhat quickly if we can (and hopefully even uplift to aurora).

This patch:
 - mostly implements comment 1 verbatim except instead of 30%, I used 40% because otherwise this patch would actually *regress* the quota available for temporary storage in the [0, 500MB] range.  Sound ok Jonas?
 - all the smartLimits except for smartLimit.chunk didn't seem to make sense with the new, simpler scheme, so I removed 'em all; there don't appear to be any uses in mozilla or tests
 - temporaryStorage.smartLimit.chunk was renamed to temporaryStorage.chunkSize since "smartLimit" no longer made sense (and "chunkSize" seems to be a better name)
 - the unit of the remaining fixedLimit/chunkSize prefs was changed from KB to bytes since I think it's simpler and what everyone wants to deal in anyways
 - to avoid per-platform variance, I removed the ANDROID #ifdef b/c it seemed like an unnecessary source of platform-specific variance.
Assignee: Jan.Varga → luke
Status: NEW → ASSIGNED
Attachment #8425112 - Flags: review?(Jan.Varga)
Attachment #8425112 - Flags: feedback?(jonas)
Attached patch tweak-temporary-quota (obsolete) — Splinter Review
(forgot to qref)
Attachment #8425112 - Attachment is obsolete: true
Attachment #8425112 - Flags: review?(Jan.Varga)
Attachment #8425112 - Flags: feedback?(jonas)
Attachment #8425116 - Flags: review?(Jan.Varga)
Attachment #8425116 - Flags: feedback?(jonas)
I agree with Shacahr. I think it would very helpful for developers that the numbers will be the same to Chrome (and also Opera).

The upper bound of 300m is not enough for so many web apps. and is not needed. The "****" that a website can write to a computer is not persistent anyway (right?) so it will be overridden when some other website (group) will write to the pull (I assume that this is the case, right?)

If this is the case, I suggest, like Shachar, that we'll go with the Chrome method, each domain can have 20% out of 50% free disk space.

Luke, what do you think?
I'm for it; it's easy to change the patch that is up for review once Jonas and Jan agree.  I mostly just want to move this along and even the attached patch is a big improvement to the current situation.
Great.
Jan, Jonas, Any objections to try and be consistent with the Chrome temporary storage numbers?
I don't think we should worry about aligning with Chrome. Their boundaries are bound to change and so are ours. Additionally, the amount of data used when using the various APIs won't be the same between browsers anyway. I.e. Chrome's IndexedDB implementation won't use the same amount of storage as Firefox's IndexedDB implementation even when the exact same data is written.

The proper solution here is to implement the quota API which allows pages to see what the limit is, and how much data they are using so far.


Luke, so is your current proposal:

availableDiskSpace = freeDiskSpace + currentlyUsedByTempStorage

totalTempStorage = cap(availableDiskSpace * .40, 0, 50GB)

x = cap(totalTempStorage * .20, 10MB, 300MB)
maxGroupStorage = cap(x, 0, totalTempStorage)

If so that sounds ok with me.

I'm happy to try to think of another limit than the 300MB. But I don't think simply picking something like 400MB is going to materially change things. We would still have to worry about some apps breaking through that limit.

Removing the maxGroupStorage limit doesn't seem like a solution either since it just will mean that all data will become more volatile since visiting a website can suddenly nuke all the data from all other websites.


Again, the quota API seems like a better solution by allowing a website to request more data and enabling the UA to prompt the user when that happens. Simply using temporary storage will always suffer from at least:
* Random websites that the user doesn't care about will be able to write hundreds of MB of data to the
  user's system.
* For websites that the user does care about the data will still be very volatile and can be deleted when
  the user visits some other website. Even if the local data is just a cache, re-downloading hundreds of MB
  of data will be annoying to the user.


We have to find a solution for allowing websites to use more resources than "normal" websites. In FirefoxOS we have already solved this through allowing websites to be installed as apps. We're going to simplify this flow even more so that there is essentially no difference between "installing" a website and bookmarking it.

Once a website is "installed"/bookmarked we remove all limits on how much data it can store.

I'd love to find a similar solution on desktop too. This way we could allow websites to do things like update in the background even when the user is not on the site. That's probably something you want to do anyway if you have huge resources that need to get updated every now and then.
Flags: needinfo?(jonas)
I wonder if there's no way we can loose the hard limit of 300MB per group, and just rely on percentage of free disk space.
To reach the limit of 300MB per group the user only needs to have ~3GB free disk space, which nowadays is really low.
If a user has 50GB free space why won't he be able to benefit those web apps that use it?
Another thing, What happens if an app wants to store 100MB but the cap will actually be lower?
Will it be able to transition to the 'persistent' idb storage after a prompt to the user?
If not, can the app know the cap in advance?
(In reply to Jonas Sicking (:sicking) from comment #19)
> Luke, so is your current proposal:
[...]
> If so that sounds ok with me.

Yep!

> I'm happy to try to think of another limit than the 300MB. But I don't think
> simply picking something like 400MB is going to materially change things. We
> would still have to worry about some apps breaking through that limit.

For the initial target of games, I expect normal bandwidth limitations would naturally keep apps under 1GB of assets, so I'd be happy with that :)

> Removing the maxGroupStorage limit doesn't seem like a solution either since
> it just will mean that all data will become more volatile since visiting a
> website can suddenly nuke all the data from all other websites.

Agreed.

> Again, the quota API seems like a better solution by allowing a website to
> request more data and enabling the UA to prompt the user when that happens.

Definitely.  It'd be nice though if we had default limits such that, any casually-playable game fits without a prompt (assuming sufficient disk space) since any "prompty" friction can turn away significant fractions of users.

> We have to find a solution for allowing websites to use more resources than
> "normal" websites. In FirefoxOS we have already solved this through allowing
> websites to be installed as apps. We're going to simplify this flow even
> more so that there is essentially no difference between "installing" a
> website and bookmarking it.

Yeah, I'm really excited about this direction.  On the caching/volatility you mentioned above, once you bookmark an app, it'd be cool if, on bookmarking, we effectively "mv storage/temporary/$(APP) storage/persistent/$(APP)", upgrading any existing asm.js and IndexedDB entries to persistent storage.  (This would really increase the need for some UI for visualizing storage usage, as people may be quite liberal with bookmarking.)  This is a good reason to allow a lot of temporary storage by default (at the expense of volatility) since, with bookmark-not-install, we expect many persistent storages to start out temporary.
> > Again, the quota API seems like a better solution by allowing a website to
> > request more data and enabling the UA to prompt the user when that happens.
> 
> Definitely.  It'd be nice though if we had default limits such that, any
> casually-playable game fits without a prompt (assuming sufficient disk
> space) since any "prompty" friction can turn away significant fractions of
> users.

Sure. Is there any data on how large "casually playable" games are?

> > We have to find a solution for allowing websites to use more resources than
> > "normal" websites. In FirefoxOS we have already solved this through allowing
> > websites to be installed as apps. We're going to simplify this flow even
> > more so that there is essentially no difference between "installing" a
> > website and bookmarking it.
> 
> Yeah, I'm really excited about this direction.  On the caching/volatility
> you mentioned above, once you bookmark an app, it'd be cool if, on
> bookmarking, we effectively "mv storage/temporary/$(APP)
> storage/persistent/$(APP)", upgrading any existing asm.js and IndexedDB
> entries to persistent storage.

My thinking about this has actually been that we need to have three storage areas: Temporary, Persistent and Default.

Default is what you'd get if you don't specify any policy at all, and presumably where we could store asm.js entries. For normal websites we'd count "default" just like temporary. However for installed/trusted/bookmarked pages we'd treat it like "permanent". This way no moving needs to happen and the page can always use the same name to access the data. Only the policy for it changes.
(In reply to Jonas Sicking (:sicking) from comment #23)
> Sure. Is there any data on how large "casually playable" games are?

Lemme see if I can get something from Unity.

> My thinking about this has actually been that we need to have three storage
> areas: Temporary, Persistent and Default.

Seems like that could work, too.  Don't forget we also discussed a different storage class whose management/eviction was more suitable for (temporary) cache files :) (Not just asm.js too, I hear cached WebGL shaders could be coming.)
D'oh, I forgot that only asm.js cache files go into temporary storage by default and IDB still goes into persistent-with-prompt-at-50MB storage by default.  Since asm.js cache entries are well under 300MB, I'm happy to keep the 300MB limit for now.  I assume by the time things switch to the "default" storage policy by default the Quota API would also be in so gamedevs can better deal with large assets.
Comment on attachment 8425116 [details] [diff] [review]
tweak-temporary-quota

comment 19 sounds like f+ from Jonas on the heuristics in this patch.
Attachment #8425116 - Flags: feedback?(jonas)
(In reply to Luke Wagner [:luke] from comment #25)
> D'oh, I forgot that only asm.js cache files go into temporary storage by
> default and IDB still goes into persistent-with-prompt-at-50MB storage by
> default.

That should not be the case. I think IDB goes into temporary by default currently.
(In reply to Jonas Sicking (:sicking) from comment #27)
Definitely into persistent (with a prompt once you cross 50MB).
If so that's a bug that we should fix ASAP. So let's not make any assumptions here based on that behavior.
IIUC, and Jan can explain better here, this has been a goal for a while.  But it's not like you can just flip the switch; until we have an API for a site to request persistent or just more storage, switching to temporary would simply break games (e.g., monstermadness.com) that exceed the temporary storage limit.
I still think that a better limit calculation should be set and that we should not have the max_group" set to 300mb (even if the user has 300GB of free space).

Maybe setting the number higher from 300 to a higher number (like 2GB ) will help, like that, a website can't take too much space on the hard drive (even if a user has 300GB of free disk space) and on the other hand for large games it will no be a problem.

We still have the mechanism that removes group data when other group is writing so we are safe here and if it is crucial for an app to be persistent it should ask for a persistent storage and that handles it.

In any case, the most important feature that is missing and is needed for both pers/temp is quota API. I have an app that asks for persistent approval, I don't know how much free space the user has and thus can completely finish the space on the user's hard drive.

I think that until an API will be provided we should have a prefixed API for knowing how much available disk space there is (for persistent) - what do you think?
Guy: at the moment, IDB doesn't even use temporary storage so this 300mb max_group setting is irrelevant (I forgot this earlier in the bug).  While the general comments on the quota API make sense, they are somewhat out of scope for this bug, which is focused on the short-term goal of bumping temporary storage for asm.js caching.
Luke: I thought that the purpose of this dialog is to make indexedDB support temporary storage. In which case my opinion (and actually need) is that a hard coded upper bound is not good since we can always find apps that will need more. Therefore I think that a relative method (relative to the free space) is a better method. The hard coded for max_group is too low and I think that it is better to make it bigger while compromising it's persistent - i.e other apps can delete that data. 

An app that really rely on the persistence of the data will likely prompt for more persistent anyway.

Regardless, the quota API is needed - I understand that this is out of scope - but this is extremely important and for me, makes indexedDB to be unusable (reliable or not) sense I don't even know how much space the user has on its computer - so the 300mb (less or more) question is irrelevant. We should talk about this in another bug.

What do you think about increasing the 300mb limit? what do you think about making it relative to the free space the user has (like in Chrome)?
Again, while these are all important points, I think they belong in another bug as IDB doesn't use temporary storage atm and, for the reasons given above, cannot be trivially switched to use it in the near-term.  I can't find any existing bugs for implementing http://www.w3.org/TR/quota-api/ or switching IDB to use temporary storage; perhaps janv/bent could point us to them.
Ah, I see, so there is currently a non-standard "storage" option to IDBFactory.open() that lets you pick "temporary"; just the default is persistent (set by http://hg.mozilla.org/mozilla-central/file/b9e1856deef1/dom/quota/QuotaManager.cpp#l2195).  That means that the group limit max *is* quite relevant for devs atm since it allows them to avoid the 50mb persistent storage prompt.  So, Jonas, what do you say to bumping this cap something much bigger than 300mb?  I didn't get any hard data back about casual game asset use other than "some games use a bunch; we really want that quota API".
Flags: needinfo?(jonas)
Thanks Luke.

That's also why I thought it is really relevant to talk about the quota manager in the regard. Without knowing how much space I have to write I can't really use it. At the moment, with the help of Jan, I overcome this by simply "pre-allocating" the entire size I want to use and see if I get an error or not.

What is your thought about "much bigger than 300mb"?
TBH, I really don't see any reason for the additional max-group-limit cap.  Yes, it will allow some random site to store 10gb if you have 125gb free but it seems to me like that's no big deal; it's not evicting other temporary storage and if the user starts using up the free space for other purposes, we'll start evicting the 10gb.
I agree Luke, max-group-limit is, meh, in my opinion as well
After some discussion with Jan and Jonas, this new version of the patch:
 - ups the % of free space available to temporary storage to 50%
 - removes the total-temporary-limit cap
 - ups the group-limit cap to 2GB
Sound good to everyone?
Attachment #8427332 - Flags: review?(Jan.Varga)
Flags: needinfo?(jonas)
Attachment #8425116 - Attachment is obsolete: true
Attachment #8425116 - Flags: review?(Jan.Varga)
Comment on attachment 8427332 [details] [diff] [review]
tweak-temporary-quota

Review of attachment 8427332 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.
If all parties agree, let's land this.

::: dom/quota/QuotaManager.cpp
@@ +1868,5 @@
>                                      &mTemporaryStorageLimit);
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
>  
> +    mTemporaryStorageInitialized = true;

Just curious, why is this needed ?
Attachment #8427332 - Flags: review?(Jan.Varga) → review+
(In reply to Jan Varga [:janv] from comment #41)
> Just curious, why is this needed ?

It's for the mTemporaryStorageInitialized assertion I added in GetGroupLimit.
(In reply to Luke Wagner [:luke] from comment #42)
> (In reply to Jan Varga [:janv] from comment #41)
> > Just curious, why is this needed ?
> 
> It's for the mTemporaryStorageInitialized assertion I added in GetGroupLimit.

ok, I somehow overlooked that.
Looks good to me.

I still think it should be capped proportionally to the free disk space and not by hard coded number - However, I think that 2GB is something that most apps should be OK with and if not they should ask for permissions for persistent storage.

What's the schedule for this?

Is there a bug open for requestQuota API?
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b762a84f3bb

(In reply to guy paskar from comment #44)
> What's the schedule for this?

Jan thought we could uplift to Beta (FF30, released Jun 10th), and it definitely seems like we could uplift to Aurora.  I'll nominate after this gets into Nightly.
Sounds good. I would be glad to see it uplifted to beta. My app is ready to test it in large scale production environment and give feedback as soon as possible.
I was wondering about a certain issue:
Let's say I"m starting to write data in a temporary db.
When I run out of space, will I get an error back, or there will be prompt automatically?
Will the db's be shared between the temporary and persistent?
(In reply to Shachar from comment #47)
> I was wondering about a certain issue:
> Let's say I"m starting to write data in a temporary db.
> When I run out of space, will I get an error back, or there will be prompt
> automatically?

You'll get an error.

> Will the db's be shared between the temporary and persistent?

Not sure what you mean here, but I suspect it's out of scope of this bug.
The third "default" storage type and quota API will provide better control, etc. once implemented.
(In reply to Jan Varga [:janv] from comment #48)
> 
> You'll get an error.
> 
Great!

> > Will the db's be shared between the temporary and persistent?
> 
> Not sure what you mean here, but I suspect it's out of scope of this bug.
> The third "default" storage type and quota API will provide better control,
> etc. once implemented.
I'm not sure this is applicable here, but for example in chrome filesystem, when you open a temporary filesystem and a persistent filesystem the data is not shared between them. meaning data written in temporary filesystem isn't visible in persisten filesystem and vice-versa. Is this the case here with IDB?
Yes, they are completely separate.
https://hg.mozilla.org/mozilla-central/rev/5b762a84f3bb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment on attachment 8427332 [details] [diff] [review]
tweak-temporary-quota

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not fixing a regression
User impact if declined: IndexedDB has abnormally low quota for temporary storage; this will cause asm.js caching to fail for soon-to-be-released partner asm.js apps
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low; increases quota
Attachment #8427332 - Flags: approval-mozilla-beta?
Attachment #8427332 - Flags: approval-mozilla-aurora?
Comment on attachment 8427332 [details] [diff] [review]
tweak-temporary-quota

I don't see this blocking b2g and we're very late in the beta 30 cycle so only approving this for Aurora and it can ride from there.
Attachment #8427332 - Flags: approval-mozilla-beta?
Attachment #8427332 - Flags: approval-mozilla-beta-
Attachment #8427332 - Flags: approval-mozilla-aurora?
Attachment #8427332 - Flags: approval-mozilla-aurora+
Whiteboard: [games] → [games][qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.