Optimized webstorage performance

Optimized webstorage performance
This patch is to improve WebStorage access perfomance.
Chromium WebStorage getItem & setItem perfomance is not so good because of its multiporcess arch.
Currently, WebStorage data transfer between browser process and renderer process by Channel, which is implemented by pipe on Win platfrom, unix domain socket on POSIX platfrom. 
One issue is, when data size is big, both pipe and socket IPC performs worse than shared memory mechnasim.
in this change, big size WebStorage data is transfered by shared memory, it will improve the setItem()/getItem() performance.
According to the perforamnce test case "WebStoragePerfTest.html" I submit in http://code.google.com/p/chromium/issues/detail?id=127528,
By this patch
On platform Ubuntu11.04/4G Memory/ Core i7, setItem() peformance improved 40%, getItem improved 10%.
On platform Win7/4G Memory/Core i5 M560, setItem() peformance improved 30%, getItem improved 15%.

BUG=127528
TEST=launch chromium and load WebStoragePerfTest.html, result will be shown after javascript finished.

[michaeln, 2012-05-10 18:42:59]

Hi pan,

Thanx for the bug report and the patch. We know about this problem (i've marked
the bug you filed as a dup of that problem) and are nearing completion of some
work to resolve it in a different way (see bug for details). The particular
methods and ipc messages that your CL pokes at are actually slated to be deleted
upon completion of that work. Given that, I think this particular CL is not
needed.

https://chromiumcodereview.appspot.com/10389067/diff/1/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_message_filter.cc (right):

https://chromiumcodereview.appspot.com/10389067/diff/1/content/browser/dom_st...
content/browser/dom_storage/dom_storage_message_filter.cc:144: void
DOMStorageMessageFilter::OnGetItem(int connection_id,
fyi: this method/syncipc is soon to be deleted from the project

https://chromiumcodereview.appspot.com/10389067/diff/1/content/browser/dom_st...
content/browser/dom_storage/dom_storage_message_filter.cc:181: void
DOMStorageMessageFilter::OnSetItem(
ditto this method/syncipc is soon to be deleted

https://chromiumcodereview.appspot.com/10389067/diff/1/content/renderer/rende...
File content/renderer/renderer_webstoragearea_impl.cc (right):

https://chromiumcodereview.appspot.com/10389067/diff/1/content/renderer/rende...
content/renderer/renderer_webstoragearea_impl.cc:119: if
(!shared_buf.CreateAndMapAnonymous(value_len)) {
See ChildThread::AllocateSharedMemory. According to that, on posix, shared
memory buffers cannot be created from within a sandboxed renderer. Unless a
buffer is pre-allocated for us by the main process and we have a handle to that
pre-allocated buffer, this allocation would involve another sync ipc.

[michaeln, 2012-05-10 20:24:19]

some ideas about where shared mem usage might be of help going forward...

http://codereview.chromium.org/10389067/diff/1/content/browser/dom_storage/do...
File content/browser/dom_storage/dom_storage_message_filter.cc (right):

http://codereview.chromium.org/10389067/diff/1/content/browser/dom_storage/do...
content/browser/dom_storage/dom_storage_message_filter.cc:123: void
DOMStorageMessageFilter::OnLoadStorageArea(int connection_id,
This syncipc method might be a good candidate for switching to shared memory.
Instead of serializing the entire |map| on the ipc channel, serialize it into a
shared memory buffer and return a handle to it on the ipc channel.

http://codereview.chromium.org/10389067/diff/1/content/browser/dom_storage/do...
content/browser/dom_storage/dom_storage_message_filter.cc:298: void
DOMStorageMessageFilter::SendDomStorageEvent(
This is another method that might be a good candidate to switch over. These
events are broadcast to all renderers. If the meaty parts (key, new_val,
old_val) were xferred to all renderers by passing a handle to the same shared
mem buffer, that would be pretty slick.

[Pan, 2012-05-11 08:45:41]

Hi michaeln,

Thanks for your quickly reply, :)
I have seen the ASYNC code in dom_storage_message_filter.cc, I believe that
would give a lot of help.
However if all the sync methods are removed, can we support W3C WebStorage spec
as before? ASYNC method have a different programing model in JS level, right?

On 2012/05/10 18:42:59, michaeln wrote:
> Hi pan,
> 
> Thanx for the bug report and the patch. We know about this problem (i've
marked
> the bug you filed as a dup of that problem) and are nearing completion of some
> work to resolve it in a different way (see bug for details). The particular
> methods and ipc messages that your CL pokes at are actually slated to be
deleted
> upon completion of that work. Given that, I think this particular CL is not
> needed.
> 
>
https://chromiumcodereview.appspot.com/10389067/diff/1/content/browser/dom_st...
> File content/browser/dom_storage/dom_storage_message_filter.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10389067/diff/1/content/browser/dom_st...
> content/browser/dom_storage/dom_storage_message_filter.cc:144: void
> DOMStorageMessageFilter::OnGetItem(int connection_id,
> fyi: this method/syncipc is soon to be deleted from the project
> 
>
https://chromiumcodereview.appspot.com/10389067/diff/1/content/browser/dom_st...
> content/browser/dom_storage/dom_storage_message_filter.cc:181: void
> DOMStorageMessageFilter::OnSetItem(
> ditto this method/syncipc is soon to be deleted
> 
>
https://chromiumcodereview.appspot.com/10389067/diff/1/content/renderer/rende...
> File content/renderer/renderer_webstoragearea_impl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10389067/diff/1/content/renderer/rende...
> content/renderer/renderer_webstoragearea_impl.cc:119: if
> (!shared_buf.CreateAndMapAnonymous(value_len)) {
> See ChildThread::AllocateSharedMemory. According to that, on posix, shared
> memory buffers cannot be created from within a sandboxed renderer. Unless a
> buffer is pre-allocated for us by the main process and we have a handle to
that
> pre-allocated buffer, this allocation would involve another sync ipc.

[michaeln, 2012-05-11 15:10:08]

> I have seen the ASYNC code in dom_storage_message_filter.cc, I believe that
> would give a lot of help.
> However if all the sync methods are removed, can we support W3C WebStorage
spec
> as before? ASYNC method have a different programing model in JS level, right?

Sure we can. The JS interface isn't being changed, just the interaction between
renderers and the main browser process. The solution to the "wicked slow"
problem we're
pursuing is client-side caching. JS get/set operations are handled by directly
reading/writing that cache. The cache for an area is primed syncly on first
access
(this is the purpose of the LoadStorageArea ipc), and then changes to that area
are
propagated asyncly across process boundaries (SetItemAsync, RemoveItemAsync,
ClearAsync,
StorageEvent).

This inprogress CL isn't ready yet but you can get the idea...
http://codereview.chromium.org/10383123/

[Pan, 2012-05-14 06:50:30]

http://codereview.chromium.org/10389067/diff/1/content/renderer/renderer_webs...
File content/renderer/renderer_webstoragearea_impl.cc (right):

http://codereview.chromium.org/10389067/diff/1/content/renderer/renderer_webs...
content/renderer/renderer_webstoragearea_impl.cc:119: if
(!shared_buf.CreateAndMapAnonymous(value_len)) {
Thanks :)
Yes, renderer cannot create a shared memory buffers in seccompsandbox, however,
in default sandbox, it can work at least till now. Shall we make sure that pass
seccompsandbox?
As a workaround in POSIX, I just submit a change that ask shm from browser
process firstly, this sync ipc cost less compare with a bulk data sync ipc
through channel.

On 2012/05/10 18:42:59, michaeln wrote:
> See ChildThread::AllocateSharedMemory. According to that, on posix, shared
> memory buffers cannot be created from within a sandboxed renderer. Unless a
> buffer is pre-allocated for us by the main process and we have a handle to
that
> pre-allocated buffer, this allocation would involve another sync ipc.

[Pan, 2012-05-14 07:04:24]

On 2012/05/11 15:10:08, michaeln wrote:
> > I have seen the ASYNC code in dom_storage_message_filter.cc, I believe that
> > would give a lot of help.
> > However if all the sync methods are removed, can we support W3C WebStorage
> spec
> > as before? ASYNC method have a different programing model in JS level,
right?
> 
> Sure we can. The JS interface isn't being changed, just the interaction
between
> renderers and the main browser process. The solution to the "wicked slow"
> problem we're
> pursuing is client-side caching. JS get/set operations are handled by directly
> reading/writing that cache. The cache for an area is primed syncly on first
> access
> (this is the purpose of the LoadStorageArea ipc), and then changes to that
area
> are
> propagated asyncly across process boundaries (SetItemAsync, RemoveItemAsync,
> ClearAsync,
> StorageEvent).
> 
> This inprogress CL isn't ready yet but you can get the idea...
> http://codereview.chromium.org/10383123/

Thanks for your explain,
I think primed syncly for cache area first and then async ipc will improve
performance greatly for sessionStorage .
however for localStorage API, I feel it will have problem when two renderer
processes in the same origin have competition to update the same localStorage
key, in that scenario, browser process is the only one who should know the real
localstorage holder, how do you think of that?

[michaeln, 2012-05-14 18:14:45]

I don't see concurrent writes as a problem. You can see how that's handled in
the CL i pointed you at earlier, the purveyor of the 'truth' is the main browser
process.

Also, fyi, sessionStorage can span process boundaries too. That it can hop from
process to process is fairly obvious, but also less obviously, a session can be
active in two different processes at the same time with pre-rendered content. So
in most respects sessionStorage is really just like localStorage.

> Thanks for your explain,
> I think primed syncly for cache area first and then async ipc will improve
> performance greatly for sessionStorage .
> however for localStorage API, I feel it will have problem when two renderer
> processes in the same origin have competition to update the same localStorage
> key, in that scenario, browser process is the only one who should know the
real
> localstorage holder, how do you think of that?

[michaeln, 2012-05-14 20:16:12]

I think the discussion here has shifted and is more relevant to the code which
is just about up for review in http://codereview.chromium.org/10383123/.

> I don't see concurrent writes as a problem. You can see how that's handled in
> the CL i pointed you at earlier, the purveyor of the 'truth' is the main
browser
> process.
> 
> Also, fyi, sessionStorage can span process boundaries too. That it can hop
from
> process to process is fairly obvious, but also less obviously, a session can
be
> active in two different processes at the same time with pre-rendered content.
So
> in most respects sessionStorage is really just like localStorage.
> 
> > Thanks for your explain,
> > I think primed syncly for cache area first and then async ipc will improve
> > performance greatly for sessionStorage .
> > however for localStorage API, I feel it will have problem when two renderer
> > processes in the same origin have competition to update the same
localStorage
> > key, in that scenario, browser process is the only one who should know the
> real
> > localstorage holder, how do you think of that?

[Pan, 2012-05-15 05:03:19]

On 2012/05/14 20:16:12, michaeln wrote:
> I think the discussion here has shifted and is more relevant to the code which
> is just about up for review in http://codereview.chromium.org/10383123/.
> 
> > I don't see concurrent writes as a problem. You can see how that's handled
in
> > the CL i pointed you at earlier, the purveyor of the 'truth' is the main
> browser
> > process.
> > 
> > Also, fyi, sessionStorage can span process boundaries too. That it can hop
> from
> > process to process is fairly obvious, but also less obviously, a session can
> be
> > active in two different processes at the same time with pre-rendered
content.
> So
> > in most respects sessionStorage is really just like localStorage.
> > 
> > > Thanks for your explain,
> > > I think primed syncly for cache area first and then async ipc will improve
> > > performance greatly for sessionStorage .
> > > however for localStorage API, I feel it will have problem when two
renderer
> > > processes in the same origin have competition to update the same
> localStorage
> > > key, in that scenario, browser process is the only one who should know the
> > real
> > > localStorage holder, how do you think of that?

Thanks for your explain :)
I think we can keep this issue 's context and shift the discussion to
http://codereview.chromium.org/10383123/.

[michaeln, 2012-05-29 23:04:46]

hi again pan, haven't heard from you in a while. i've run the test you provided
against m20 (sync and no caching) and m21 (async with caching).

**** m20 results (sync w/o caching)

Write Test:
string size	rounds	total size	total time (ms)	average speed (KB/s)
4096KB	384	1572864KB	18911	83171.91
Read Test:
string size	rounds	total size	total time (ms)	average speed (KB/s)
4096KB	384	1572864KB	11399	137982.63

**** m21 results (async with caching)

Write Test:
string size	rounds	total size	total time (ms)	average speed (KB/s)
4096KB	384	1572864KB	6397	245875.25
Read Test:
string size	rounds	total size	total time (ms)	average speed (KB/s)
4096KB	384	1572864KB	4904	320730.83

i think we can close this particular CL

[Pan, 2012-05-30 00:24:33]

Hi Michael, your work is very impressive, this CL should be closed.
Thanks. :)

-----Original Message-----
From: michaeln@chromium.org [mailto:michaeln@chromium.org] 
Sent: Wednesday, May 30, 2012 7:05 AM
To: Deng, Pan; darin@chromium.org; jorlow@chromium.org; abarth@chromium.org
Cc: chromium-reviews@chromium.org; joi+watch-content@chromium.org;
darin-cc@chromium.org; jam@chromium.org
Subject: Re: Optimized webstorage performance (issue 10389067)

hi again pan, haven't heard from you in a while. i've run the test you provided
against m20 (sync and no caching) and m21 (async with caching).

**** m20 results (sync w/o caching)

Write Test:
string size	rounds	total size	total time (ms)	average speed (KB/s)
4096KB	384	1572864KB	18911	83171.91
Read Test:
string size	rounds	total size	total time (ms)	average speed (KB/s)
4096KB	384	1572864KB	11399	137982.63

**** m21 results (async with caching)

Write Test:
string size	rounds	total size	total time (ms)	average speed (KB/s)
4096KB	384	1572864KB	6397	245875.25
Read Test:
string size	rounds	total size	total time (ms)	average speed (KB/s)
4096KB	384	1572864KB	4904	320730.83

i think we can close this particular CL

https://chromiumcodereview.appspot.com/10389067/