Make PStorage actors from children with the same principal share a parent DOMStorage object

NEW
Unassigned

Status

()

Core
DOM
5 years ago
5 years ago

People

(Reporter: jdm, Unassigned)

Tracking

18 Branch
x86_64
Linux
Points:
---

Firefox Tracking Flags

(blocking-b2g:-, b2g18+)

Details

(Whiteboard: [MemShrink:P2] c=)

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Currently we create a brand new DOMStorageImpl for every StorageParent instance, causing us to re-cache everything on the first access. We should look into obtaining the relevant principal and using nsIDOMStorageManager to get a proper shared DOMStorage object in the parent instead.
Whiteboard: [MemShrink]
(Reporter)

Updated

5 years ago
Version: unspecified → 18 Branch
blocking-b2g: --- → tef?
(Reporter)

Comment 1

5 years ago
I've got code that I'm testing, but this appears harder than originally expected. We seem to be missing the code that ensures one storage object per origin.
Assignee: nobody → josh
(Reporter)

Comment 2

5 years ago
Honza, the comment in nsIDOMStorageManager for getLocalStorageForPrincipal says "Returns instance of localStorage object for aURI's origin. This method ensures there is always only a single instance for a single origin.", which we assumed meant we could get a storage object that wouldn't require re-caching all of the keys and values on subsequent reloads. However, looking at the code, getLocalStorageForPrincipal has always created a brand new storage object since it was first introduced, and there does not appear to be any kind of origin -> storage object cache anywhere. Am I misunderstanding the purpose of the method?
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 3

5 years ago
Created attachment 741867 [details] [diff] [review]
Really ugly, hacky WIP that doesn't give the expected results.

As described.
(In reply to Josh Matthews [:jdm] from comment #2)
> Honza, the comment in nsIDOMStorageManager for getLocalStorageForPrincipal
> says "Returns instance of localStorage object for aURI's origin. This method
> ensures there is always only a single instance for a single origin.", which
> we assumed meant we could get a storage object that wouldn't require
> re-caching all of the keys and values on subsequent reloads. However,
> looking at the code, getLocalStorageForPrincipal has always created a brand
> new storage object since it was first introduced, and there does not appear
> to be any kind of origin -> storage object cache anywhere. Am I
> misunderstanding the purpose of the method?

Not sure I follow the question.  In this old code nsDOMStorage caches all its entries inside it self.  There is always new nsDOMStorage object for each windows.localStorage.  We also preload the data to an inmemory sqlite table.

This has changed in the new code - there is only one common cache for all objects.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #4)
> In this old code nsDOMStorage caches all
> its entries inside it self.

In it's DOMStorageImpl to be exact.
If you really want to go the way of saring DOMStorageImpl, you may want to keep a hash table of them by principal code base URI.  The patch should be just used to prove the concept.
(Reporter)

Comment 7

5 years ago
Given the amount of effort required to create a new per-origin cache and get the details of when to delete it correct, I don't think this should be the focus of our tef-blocking efforts. We should get as much bang for our buck from bug 864941 instead.

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink] [tef-triage]
Leaving this nominated for now as we discuss bug 850175 and whether that will continue to block.
Whiteboard: [MemShrink] [tef-triage] → [MemShrink:P3] [tef-triage]
Whiteboard: [MemShrink:P3] [tef-triage] → [MemShrink:P2] [tef-triage]
Circling back to this, bug 850175 is taking it's course based on comment 7 this bug actually sounds like too much risk to take so late in v1.0.1 and our resources are better spent elsewhere.  Tracking and flagging for scrum backlog.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Whiteboard: [MemShrink:P2] [tef-triage] → [MemShrink:P2] c=
(Reporter)

Updated

5 years ago
Assignee: josh → nobody
You need to log in before you can comment on or make changes to this bug.