Closed
Bug 1361974
Opened 8 years ago
Closed 8 years ago
Add API to check usage of a given storage
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: ttaubert, Assigned: ttaubert)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
In SessionStore, we're currently "estimating" storage size by first collecting it and then summing up the string length of the DOMSessionStorage entries. That's bad and we should add a proper API to check usage before we collect anything.
Updated•8 years ago
|
Blocks: ss-feature
Assignee | ||
Comment 1•8 years ago
|
||
I hope this isn't too stupid. At first I added it to the StorageManager, but then one could basically pass the sessionStorage to the localStorage manager and still get correct results, which seemed confusing. So instead I just added it to nsIDOMWindowUtils.
Comment 2•8 years ago
|
||
Comment on attachment 8864471 [details] [diff] [review]
0001-Bug-1361974-Add-API-to-check-usage-of-a-given-storag.patch
Review of attachment 8864471 [details] [diff] [review]:
-----------------------------------------------------------------
Yep, why not.
::: dom/storage/StorageCache.cpp
@@ +542,5 @@
>
> +uint64_t
> +StorageCache::GetOriginQuotaUsage(const Storage* aStorage) const
> +{
> + return mData[GetDataSetIndex(aStorage)].mOriginQuotaUsage;
mOriginQuotaUsage is signed
Attachment #8864471 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #2)
> > +uint64_t
> > +StorageCache::GetOriginQuotaUsage(const Storage* aStorage) const
> > +{
> > + return mData[GetDataSetIndex(aStorage)].mOriginQuotaUsage;
>
> mOriginQuotaUsage is signed
Indeed, fixed. Thanks!
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd0b66f2ab5
Add nsIDOMWindowUtils API to check usage of a given storage r=mayhemer
Comment 5•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 6•8 years ago
|
||
This bug may have contributed to a sudden change in the Telemetry probe FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS[1] which seems to have occurred in Nightly 20170506[2][3].
It seems as though we're recording many many more storage size estimates.
Is this an improvement? A regression?
Is this intentional? Is this expected?
Is this probe measuring something useful?
[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1646/alerts/?from=2017-05-06&to=2017-05-06
[2]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0b255199db9d6a6f189b89b7906f99155bde3726&tochange=37a5b7f6f101df2eb292b1b6baaf1540c9920e20
[3]: https://mzl.la/2rnszt9
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Chris H-C :chutten from comment #6)
> This bug may have contributed to a sudden change in the Telemetry probe
> FX_SESSION_RESTORE_DOM_STORAGE_SIZE_ESTIMATE_CHARS[1] which seems to have
> occurred in Nightly 20170506[2][3].
>
> It seems as though we're recording many many more storage size estimates.
Yeah... Probably dozens while just doing a Google search. Is the amount of data/pings we collect problematic?
> Is this an improvement? A regression?
It's kind of an improvement, we get a lot more data points now :)
> Is this intentional? Is this expected?
Definitely expected but I didn't make that change to get more data points.
> Is this probe measuring something useful?
Great question. It does tell us that people store a lot of data in DOMSessionStorage, but we knew that before already. I wouldn't mind if the probe went away, there's no real value in seeing these data points change over time. Its value was to realize we have to do something about that. WDYT?
Flags: needinfo?(ttaubert)
Comment 8•8 years ago
|
||
Well, if it's still useful we can keep it. The number of samples it stores doesn't have an adverse effect on storage or bandwidth (which is why we store these things as sparse histograms :) ).
While we're thinking about it, could we bring the definition up-to-date? It needs an alert_emails field containing email addresses of people who know what it means when this histogram changes suddenly (not sure why it isn't alerting session-restore-telemetry-alerts@ like the other session restore probes). It also needs a bug_numbers field containing bugs that affect its behaviour or provide more information (like this one!)
If we're unsure about its usefulness, we can give it five versions to prove it (or prove otherwise) and set an expires_in_version of 61.
Sound good to you?
Flags: needinfo?(ttaubert)
Assignee | ||
Comment 9•7 years ago
|
||
SGTM. I'll attach a patch to bug 1362058 that changed the probe.
Flags: needinfo?(ttaubert)
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•