872808 - Merge localstorage changes from b2g18 to mc (bug 850175)

Status: RESOLVED WONTFIX

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

Description

[Gregor Wagner [:gwagner]]

Currently the fix for the Twitter crash from bug 850175 is only on b2g18. We should also apply those changes on trunk.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: Quota patch

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: Broadcast patch

Comment 3

[Mounir Lamouri (:mounir)]

Don't we have a general quota handling coming including localStorage? Isn't that patch breaking the idea of that feature by having a specific quota for localStorage?

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 750136 [details] [diff] [review]
Broadcast patch

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

This is non sense for the new implementation.  What is the reason to block the events when they are not going through IPC?

If we want to save memory here, we must find a way to not copy the string data, which I was in believe is not happening.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 750136 [details] [diff] [review]
Broadcast patch

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

::: dom/src/storage/DOMStorage.cpp
@@ +52,5 @@
>  , mIsPrivate(aIsPrivate)
>  , mIsSessionOnly(false)
>  {
>    mCache->Preload();
> +  mBroadcastLimit = Preferences::GetUint(kBroadcastLimit) * 1024;

This shouldn't be a member on DOMStorage. Make it a static global variable in this file. And only initialize it once, perhaps by initializing it to -1 and only reading the pref if the global is not -1.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Honza: Is it really worth only blocking IPC events? The downside is that on platforms where we actually have use this limitation, it means that the limitation will apply inconsistently.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #6)
> Honza: Is it really worth only blocking IPC events? The downside is that on
> platforms where we actually have use this limitation, it means that the
> limitation will apply inconsistently.

The limitation is a hack that is going to be fixed when b2g will base on code with 600307 landed.  That is my opinion.  What you say sounds like when we do a hack (and in this case not really good one!) to optimize something we will have to live with it for ever?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Honza: The patch is is for mozilla-central where bug 600307 obviously has already landed.

I don't think that bug 600307 helps us that much with memory copies. Many of the copies are required in order to do the IO and doing the IPC. I.e. reading a 4MB value stored in an entry is going to at least require 4MB copy that SQLite creates during reading. Then 4MB as the value is read into a ns(C)String. Then 4MB in the IPC code as we create an IPC package. Then 4MB in the child process as we create an ns(C)String. Since this happens in a rapid succession all of these values can end up being in memory at the same time.

And in our case twitter was changing a 4MB value to another 4MB value, which meant that there's a few more 4MB chunks since we were dealing with two separate 4MB values that were copied over IPC. And the values were being copied from one child process to another by going through the parent process.

We can improve the situation a little bit if we changed localStorage to do the IO in the child process, since then there would be fewer copies due to IPC as the data is read.

But the change notification still result in multiple IPC copies as data is being sent from one child process to multiple other ones.

So on low-memory devices like the ones that we're shipping FirefoxOS on, the notifications are generating a lot of copies that's no fault of the localStorage code, or even possible for the localStorage code to get rid of. So the only way to reduce the memory overhead is to remove the notification.

So I don't really think of it as a hack, but rather disabling features that we simply can't make work in the constrained environment that we have.

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #8)
> Honza: The patch is is for mozilla-central where bug 600307 obviously has
> already landed.
> 
> I don't think that bug 600307 helps us that much with memory copies. Many of
> the copies are required in order to do the IO and doing the IPC. I.e.
> reading a 4MB value stored in an entry is going to at least require 4MB copy
> that SQLite creates during reading. Then 4MB as the value is read into a
> ns(C)String. Then 4MB in the IPC code as we create an IPC package. Then 4MB
> in the child process as we create an ns(C)String. Since this happens in a
> rapid succession all of these values can end up being in memory at the same
> time.
> 
> And in our case twitter was changing a 4MB value to another 4MB value, which
> meant that there's a few more 4MB chunks since we were dealing with two
> separate 4MB values that were copied over IPC. And the values were being
> copied from one child process to another by going through the parent process.
> 
> We can improve the situation a little bit if we changed localStorage to do
> the IO in the child process, since then there would be fewer copies due to
> IPC as the data is read.

Is there a bug for this?  Can more then one process access the same origin data?

> 
> But the change notification still result in multiple IPC copies as data is
> being sent from one child process to multiple other ones.

The current code is not doing anything like this.  If you change data in one child process then a different child process where localStorage for the same origin exists doesn't get any notification.  I had information that's not needed and since we are limiting the notifications anyway, I think we can live with it.

> 
> So on low-memory devices like the ones that we're shipping FirefoxOS on, the
> notifications are generating a lot of copies that's no fault of the
> localStorage code, or even possible for the localStorage code to get rid of.
> So the only way to reduce the memory overhead is to remove the notification.
> 
> So I don't really think of it as a hack, but rather disabling features that
> we simply can't make work in the constrained environment that we have.

Let you know that we cache all localstorage data for an origin in child processes.  The cache lives as long as the window.  After a window closes localstorage cache is being held for 20 seconds to prevent loading it again (i.e. prevent blocking).

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Honza Bambas (:mayhemer) from comment #9)
> (In reply to Jonas Sicking (:sicking) from comment #8)
> > We can improve the situation a little bit if we changed localStorage to do
> > the IO in the child process, since then there would be fewer copies due to
> > IPC as the data is read.
> 
> Is there a bug for this?  Can more then one process access the same origin
> data?

Yes, in some circumstances multiple processes can access the same origin data. Though most of the time that is not the case. And we can know the cases when that can happen and when it can't.

I filed bug 896885

> > But the change notification still result in multiple IPC copies as data is
> > being sent from one child process to multiple other ones.
> 
> The current code is not doing anything like this.  If you change data in one
> child process then a different child process where localStorage for the same
> origin exists doesn't get any notification.  I had information that's not
> needed and since we are limiting the notifications anyway, I think we can
> live with it.

Yeah, I think we can live with that for now. Though it does mean that things don't work quite correctly when opened by the browser app in B2G right now.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Mass closing as we are no longer working on b2g/firefox os.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Mass closing as we are no longer working on b2g/firefox os.