Break large blobs into multiple ipcs during creation.

A few improvements to Blob handling.
* Break large blobs into multiple ipcs during creation.
* Use shared memory blocks for the xfer.
* Rename some methods and IPCs for readability.
* Cap memory usage in the browser process at 1G.

BUG=97221
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105950

[michaeln, 2011-09-22 20:40:10]

still have to look at the blob_storage_controller_unittest

[michaeln, 2011-09-22 21:53:15]

fixed up the unittests

[michaeln, 2011-09-22 22:59:25]

http://codereview.chromium.org/7974011/diff/5019/webkit/blob/blob_storage_con...
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/7974011/diff/5019/webkit/blob/blob_storage_con...
webkit/blob/blob_storage_controller.cc:68:
target_blob_data->AppendData(item.data());
Since there's no spilling over to disk if things get very large yet we can run
into OOM errors here and crash the browser process. It might be nice to have
some defense against that until we do spill over to disk when appropiate. Wdyt?

[michaeln, 2011-09-23 21:41:33]

> Since there's no spilling over to disk if things get very large yet we can run
> into OOM errors here and crash the browser process. It might be nice to have
> some defense against that until we do spill over to disk when appropiate.
Wdyt?

I've added a 1G limit to blob storage controller memory usage.

[jianli, 2011-09-23 23:00:42]

http://codereview.chromium.org/7974011/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/blob_message_filter.h (right):

http://codereview.chromium.org/7974011/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/blob_message_filter.h:31: void
OnRegisterUnfinalizedBlobUrl(const GURL& url);
It would be more intuitive to name the message as
BlobHostMsg_StartBuildingBlob.

http://codereview.chromium.org/7974011/diff/15001/content/common/webblobregis...
File content/common/webblobregistry_impl.cc (right):

http://codereview.chromium.org/7974011/diff/15001/content/common/webblobregis...
content/common/webblobregistry_impl.cc:37: const size_t kMaxDataChunkSize = 5 *
1024 * 1024;
Why do we need to enforce this size constraint?

If we do this, I think it would be better to abort the request rather than
truncating the data silently.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:31: struct Item {
Why do we need to change from class to struct?

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:57: void SetToData(const char* data, uint64 length) {
Does std::string data support 64-bit?

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:145: int64 GetMemoryUsage() {
Please add const modifier.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:149: if (iter->type() == TYPE_DATA)
Do we also need to count the sub-blob?

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:53: BlobMap::iterator found =
unfinalized_blob_map_.find(url.spec());
Can we introduce a helper function like GetUnfinalizedBlobDataFromUrl, similar
to what GetBlobDataFromUrl does? DCHECK(!BlobUrlHasRef(url)) logic could be also
moved into the helper function.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:59: memory_usage_ -=
target_blob_data->GetMemoryUsage();
Can we avoid re-enumerating all the items to count the memory usage?

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.h:58: int64 memory_usage_;
Please add comment for this variable.

[michaeln, 2011-09-24 01:15:24]

no snapshot yet, i have to look at memory_usage_ getting out of sync when clones
are made.

http://codereview.chromium.org/7974011/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/blob_message_filter.h (right):

http://codereview.chromium.org/7974011/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/blob_message_filter.h:31: void
OnRegisterUnfinalizedBlobUrl(const GURL& url);
On 2011/09/23 23:00:42, jianli wrote:
> It would be more intuitive to name the message as
> BlobHostMsg_StartBuildingBlob.

That would make sense so long as we change the terminology throughout to be
consistent. How's this...

StartBuildingBlob  // replaces Register (along with the next two)
AppendBlobDataItem
FinishBuildingBlob
CloneBlob  // replaces RegisterFromOther
RemoveBlob // replaces Unregister

http://codereview.chromium.org/7974011/diff/15001/content/common/webblobregis...
File content/common/webblobregistry_impl.cc (right):

http://codereview.chromium.org/7974011/diff/15001/content/common/webblobregis...
content/common/webblobregistry_impl.cc:37: const size_t kMaxDataChunkSize = 5 *
1024 * 1024;
On 2011/09/23 23:00:42, jianli wrote:
> Why do we need to enforce this size constraint?
> 
> If we do this, I think it would be better to abort the request rather than
> truncating the data silently.

It's not truncated? There are a series of BlobHostMsg_AppendBlobDataItem
messages which send all of the data. See the while() loop.

IPC messages have a 'max' size constraint, we need to fit underneath that size
constraint. 5M is well beneath that limit, it could be larger. Kind of arbitrary
what value to use here (so long as it's under the max). I choose 5M so the size
of contiguous heap allocated blocks don't have to be too large.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:31: struct Item {
On 2011/09/23 23:00:42, jianli wrote:
> Why do we need to change from class to struct?

To use the handy IPC_STRUCT macro stuff instead of handcoding it.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:57: void SetToData(const char* data, uint64 length) {
On 2011/09/23 23:00:42, jianli wrote:
> Does std::string data support 64-bit?

no, i'll change to uint32

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:145: int64 GetMemoryUsage() {
On 2011/09/23 23:00:42, jianli wrote:
> Please add const modifier.

will do

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:149: if (iter->type() == TYPE_DATA)
On 2011/09/23 23:00:42, jianli wrote:
> Do we also need to count the sub-blob?

No, they're 'canonicalized' into FILE or DATA items in the storage controller.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:53: BlobMap::iterator found =
unfinalized_blob_map_.find(url.spec());
On 2011/09/23 23:00:42, jianli wrote:
> Can we introduce a helper function like GetUnfinalizedBlobDataFromUrl, similar
> to what GetBlobDataFromUrl does? DCHECK(!BlobUrlHasRef(url)) logic could be
also
> moved into the helper function.

In most cases, we actually want the iter more than the blobData ptr, so i didn't
see that helper as being helpful.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:59: memory_usage_ -=
target_blob_data->GetMemoryUsage();
On 2011/09/23 23:00:42, jianli wrote:
> Can we avoid re-enumerating all the items to count the memory usage?

The value could be cached in BlobData, but i don't think it's worth it.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:124: blob_map_[url.spec()] = blob_data;
Bah... this throws off the memory_usage_ tracking. I'll take a closer look at
that.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.h:58: int64 memory_usage_;
On 2011/09/23 23:00:42, jianli wrote:
> Please add comment for this variable.

I thought memory_usage_ was mnemonic enough. What should the comment say?

[michaeln, 2011-09-24 02:30:56]

here's a new snapshot

[jianli, 2011-09-27 01:12:18]

By the way, have you run the blob related webkit layout tests to verify?

http://codereview.chromium.org/7974011/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/blob_message_filter.h (right):

http://codereview.chromium.org/7974011/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/blob_message_filter.h:31: void
OnRegisterUnfinalizedBlobUrl(const GURL& url);
On 2011/09/24 01:15:24, michaeln wrote:
> On 2011/09/23 23:00:42, jianli wrote:
> > It would be more intuitive to name the message as
> > BlobHostMsg_StartBuildingBlob.
> 
> That would make sense so long as we change the terminology throughout to be
> consistent. How's this...
> 
> StartBuildingBlob  // replaces Register (along with the next two)
> AppendBlobDataItem
> FinishBuildingBlob
> CloneBlob  // replaces RegisterFromOther
> RemoveBlob // replaces Unregister

Sounds good to me.

http://codereview.chromium.org/7974011/diff/15001/content/common/webblobregis...
File content/common/webblobregistry_impl.cc (right):

http://codereview.chromium.org/7974011/diff/15001/content/common/webblobregis...
content/common/webblobregistry_impl.cc:37: const size_t kMaxDataChunkSize = 5 *
1024 * 1024;
On 2011/09/24 01:15:24, michaeln wrote:
> On 2011/09/23 23:00:42, jianli wrote:
> > Why do we need to enforce this size constraint?
> > 
> > If we do this, I think it would be better to abort the request rather than
> > truncating the data silently.
> 
> It's not truncated? There are a series of BlobHostMsg_AppendBlobDataItem
> messages which send all of the data. See the while() loop.
> 
> IPC messages have a 'max' size constraint, we need to fit underneath that size
> constraint. 5M is well beneath that limit, it could be larger. Kind of
arbitrary
> what value to use here (so long as it's under the max). I choose 5M so the
size
> of contiguous heap allocated blocks don't have to be too large.

Got it. Could you please add comment for this so that other people without the
background can understand this logic easily?

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:31: struct Item {
On 2011/09/24 01:15:24, michaeln wrote:
> On 2011/09/23 23:00:42, jianli wrote:
> > Why do we need to change from class to struct?
> 
> To use the handy IPC_STRUCT macro stuff instead of handcoding it.

Then we'd better remove the underscore suffix from all member variables and get
rid of all accessors.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.h:58: int64 memory_usage_;
On 2011/09/24 01:15:24, michaeln wrote:
> On 2011/09/23 23:00:42, jianli wrote:
> > Please add comment for this variable.
> 
> I thought memory_usage_ was mnemonic enough. What should the comment say?

Better to comment about what kind of stuffs are counted into the memory usage
since we only count the binary data.

http://codereview.chromium.org/7974011/diff/21005/webkit/blob/blob_data.h
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/7974011/diff/21005/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:57: void SetToData(const char* data, int length) {
Why not using uint32?

[michaeln, 2011-09-27 23:27:31]

http://codereview.chromium.org/7974011/diff/15001/content/browser/renderer_ho...
File content/browser/renderer_host/blob_message_filter.h (right):

http://codereview.chromium.org/7974011/diff/15001/content/browser/renderer_ho...
content/browser/renderer_host/blob_message_filter.h:31: void
OnRegisterUnfinalizedBlobUrl(const GURL& url);
On 2011/09/27 01:12:18, jianli wrote:
> On 2011/09/24 01:15:24, michaeln wrote:
> > On 2011/09/23 23:00:42, jianli wrote:
> > > It would be more intuitive to name the message as
> > > BlobHostMsg_StartBuildingBlob.
> > 
> > That would make sense so long as we change the terminology throughout to be
> > consistent. How's this...
> > 
> > StartBuildingBlob  // replaces Register (along with the next two)
> > AppendBlobDataItem
> > FinishBuildingBlob
> > CloneBlob  // replaces RegisterFromOther
> > RemoveBlob // replaces Unregister
> 
> Sounds good to me.

Done.

http://codereview.chromium.org/7974011/diff/15001/content/common/webblobregis...
File content/common/webblobregistry_impl.cc (right):

http://codereview.chromium.org/7974011/diff/15001/content/common/webblobregis...
content/common/webblobregistry_impl.cc:37: const size_t kMaxDataChunkSize = 5 *
1024 * 1024;
On 2011/09/27 01:12:18, jianli wrote:
> On 2011/09/24 01:15:24, michaeln wrote:
> > On 2011/09/23 23:00:42, jianli wrote:
> > > Why do we need to enforce this size constraint?
> > > 
> > > If we do this, I think it would be better to abort the request rather than
> > > truncating the data silently.
> > 
> > It's not truncated? There are a series of BlobHostMsg_AppendBlobDataItem
> > messages which send all of the data. See the while() loop.
> > 
> > IPC messages have a 'max' size constraint, we need to fit underneath that
size
> > constraint. 5M is well beneath that limit, it could be larger. Kind of
> arbitrary
> > what value to use here (so long as it's under the max). I choose 5M so the
> size
> > of contiguous heap allocated blocks don't have to be too large.
> 
> Got it. Could you please add comment for this so that other people without the
> background can understand this logic easily?

Done.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:31: struct Item {
On 2011/09/27 01:12:18, jianli wrote:
> On 2011/09/24 01:15:24, michaeln wrote:
> > On 2011/09/23 23:00:42, jianli wrote:
> > > Why do we need to change from class to struct?
> > 
> > To use the handy IPC_STRUCT macro stuff instead of handcoding it.
> 
> Then we'd better remove the underscore suffix from all member variables and
get
> rid of all accessors.

Done. This was a ripply change, had to update many accessor callsites.

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/7974011/diff/15001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.h:58: int64 memory_usage_;
On 2011/09/27 01:12:18, jianli wrote:
> On 2011/09/24 01:15:24, michaeln wrote:
> > On 2011/09/23 23:00:42, jianli wrote:
> > > Please add comment for this variable.
> > 
> > I thought memory_usage_ was mnemonic enough. What should the comment say?
> 
> Better to comment about what kind of stuffs are counted into the memory usage
> since we only count the binary data.

Done.

http://codereview.chromium.org/7974011/diff/21005/webkit/blob/blob_data.h
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/7974011/diff/21005/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:57: void SetToData(const char* data, int length) {
On 2011/09/27 01:12:18, jianli wrote:
> Why not using uint32?

Done, switched to using size_t which is what std::string expects.

[michaeln, 2011-09-28 00:39:45]

> By the way, have you run the blob related webkit layout tests to verify?

Not yet, they're pretty scattered about in multiple locations in the LayoutTests
directory. I'll do so locally and start some layout_bots too.

[michaeln, 2011-09-28 00:45:24]

On 2011/09/28 00:39:45, michaeln wrote:
> > By the way, have you run the blob related webkit layout tests to verify?
> 
> Not yet, they're pretty scattered about in multiple locations in the
LayoutTests
> directory. I'll do so locally and start some layout_bots too.

Yup... the tests in http/tests/local/blob and fast/file all pass. There are
other blob related things elsewhere too i think. I'll get some trybots going.

[jianli, 2011-09-29 01:13:33]

Looks quite good. Some more issues, but nothing big.

http://codereview.chromium.org/7974011/diff/34001/content/browser/renderer_ho...
File content/browser/renderer_host/blob_message_filter.cc (right):

http://codereview.chromium.org/7974011/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/blob_message_filter.cc:53:
blob_storage_context_->controller()->RegisterUnfinalizedBlobUrl(url);
Can we also update BlobStorageContext to use the new naming scheme for
consistency?

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_data.h
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:42: data = data_str;
Nit: probably no need to rename parameter from data to data_str. We can do sth
like:
  this.data = data;

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:32: static const int64 kMaxMemoryUsage =
1073741824;  // 1G
How about sth like: 1024 x 1024 x 1024 to make it more readable?

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:143: if
(DecrementBlobDataUsage(found->second))
Just want to check whether it is possible to rely on the reference count of
BlobData to decide if it is the right time to decrease the memory usage count.

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.h:62: // Returns true if no longer is use.
in use?

http://codereview.chromium.org/7974011/diff/34001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/7974011/diff/34001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_write_unittest.cc:137: void
RegisterBlobUrl(const GURL& url, const BlobData* data) {
This helper function seems to be same as one we introduce in
blob_storage_controller_unittest.cc. Can we share the same code somehow?

[michaeln, 2011-09-29 18:59:37]

http://codereview.chromium.org/7974011/diff/34001/content/browser/renderer_ho...
File content/browser/renderer_host/blob_message_filter.cc (right):

http://codereview.chromium.org/7974011/diff/34001/content/browser/renderer_ho...
content/browser/renderer_host/blob_message_filter.cc:53:
blob_storage_context_->controller()->RegisterUnfinalizedBlobUrl(url);
On 2011/09/29 01:13:33, jianli wrote:
> Can we also update BlobStorageContext to use the new naming scheme for
> consistency?

Done.

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_data.h
File webkit/blob/blob_data.h (right):

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_data.h#new...
webkit/blob/blob_data.h:42: data = data_str;
On 2011/09/29 01:13:33, jianli wrote:
> Nit: probably no need to rename parameter from data to data_str. We can do sth
> like:
>   this.data = data;

Done (duh... thnx)

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:32: static const int64 kMaxMemoryUsage =
1073741824;  // 1G
On 2011/09/29 01:13:33, jianli wrote:
> How about sth like: 1024 x 1024 x 1024 to make it more readable?

Done.

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:143: if
(DecrementBlobDataUsage(found->second))
On 2011/09/29 01:13:33, jianli wrote:
> Just want to check whether it is possible to rely on the reference count of
> BlobData to decide if it is the right time to decrease the memory usage count.

It's an option, but that would mean more tightly coupling the controller and the
data classes (to observe destruction or keep backptrs to the controller). And
also blobdata objects may outlive the controller since BlobUrlRequestJobs take a
reference to them until the request is done. I went with the more explicit book
keeping scheme for its simplicity.

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/7974011/diff/34001/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.h:62: // Returns true if no longer is use.
On 2011/09/29 01:13:33, jianli wrote:
> in use?

Done.

http://codereview.chromium.org/7974011/diff/34001/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_write_unittest.cc (right):

http://codereview.chromium.org/7974011/diff/34001/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_write_unittest.cc:137: void
RegisterBlobUrl(const GURL& url, const BlobData* data) {
On 2011/09/29 01:13:33, jianli wrote:
> This helper function seems to be same as one we introduce in
> blob_storage_controller_unittest.cc. Can we share the same code somehow?

Done, shared as controller->AddFinishedBlob(url, data)

[michaeln, 2011-09-29 19:36:20]

http://codereview.chromium.org/7974011/diff/40001/webkit/tools/test_shell/tes...
File webkit/tools/test_shell/test_shell_webblobregistry_impl.cc (right):

http://codereview.chromium.org/7974011/diff/40001/webkit/tools/test_shell/tes...
webkit/tools/test_shell/test_shell_webblobregistry_impl.cc:47:
g_io_thread->PostTask(FROM_HERE, NewRunnableMethod(
also switched to using base::Bind in this file

[michaeln, 2011-09-29 19:45:01]

http://codereview.chromium.org/7974011/diff/44004/webkit/blob/blob_url_reques...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/7974011/diff/44004/webkit/blob/blob_url_reques...
webkit/blob/blob_url_request_job.cc:338: // stream_.Seek() blocks the IO thread,
see http://crbug.com/75548.
might be nice to do the seek on the filethread prior to coming back to the io
thread...

FileUtilProxy::OpenAndSeek(path, open_flags, seek_offset)

[jianli, 2011-09-30 00:59:33]

LGTM w/ a few nits.

http://codereview.chromium.org/7974011/diff/44004/content/common/webkit_param...
File content/common/webkit_param_traits.h (left):

http://codereview.chromium.org/7974011/diff/44004/content/common/webkit_param...
content/common/webkit_param_traits.h:24: #include "webkit/blob/blob_data.h"
This include is not longer needed.

http://codereview.chromium.org/7974011/diff/44004/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.cc (right):

http://codereview.chromium.org/7974011/diff/44004/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.cc:285: DCHECK(found !=
blob_data_usage_count_.end());
More sanity check like:
  DCHECK(found->second > 0);

http://codereview.chromium.org/7974011/diff/44004/webkit/blob/blob_storage_co...
File webkit/blob/blob_storage_controller.h (right):

http://codereview.chromium.org/7974011/diff/44004/webkit/blob/blob_storage_co...
webkit/blob/blob_storage_controller.h:37: void AddFinishedBlob(const GURL& url,
const BlobData* blob_data);
Probably BuildBlob sounds better? This seems to be used for testing purpose.
Perhaps we should add the comment to warn this.

[jam, 2011-09-30 01:14:00]

IPC was never meant to send MBs of data. On some platforms, it's slow to do so
and it would introduce delays in the IPC system that can affect other tabs.

When we have large data, we use shared memory. Why can't this be done here?

[michaeln, 2011-09-30 21:56:29]

> When we have large data, we use shared memory. Why can't this be done here?

Thnx for making that really good point. I'll look into doing just that.

[michaeln, 2011-10-03 20:41:22]

On 2011/09/30 21:56:29, michaeln wrote:
> > When we have large data, we use shared memory. Why can't this be done here?
> 
> Thnx for making that really good point. I'll look into doing just that.

Oh... looks like sharedmem allocation has to happen in the browser process, if
so, that's unfortunate.

void RenderMessageFilter::OnAllocateSharedMemoryBuffer(
    uint32 buffer_size,
    base::SharedMemoryHandle* handle) {
  base::SharedMemory shared_buf;
  if (!shared_buf.CreateAndMapAnonymous(buffer_size)) {
    *handle = base::SharedMemory::NULLHandle();
    NOTREACHED() << "Cannot map shared memory buffer";
    return;
  }
  shared_buf.GiveToProcess(peer_handle(), handle);
}

[jam, 2011-10-03 20:42:43]

On Mon, Oct 3, 2011 at 1:41 PM, <michaeln@chromium.org> wrote:

> On 2011/09/30 21:56:29, michaeln wrote:
>
>> > When we have large data, we use shared memory. Why can't this be done
>> here?
>>
>
>  Thnx for making that really good point. I'll look into doing just that.
>>
>
> Oh... looks like sharedmem allocation has to happen in the browser process,
> if
> so, that's unfortunate.
>

Why is that unfortunate? In a few cases where we need this from the
renderer, we send an IPC to allocate the memory (i.e. what this IPC does).

>
> void RenderMessageFilter::**OnAllocateSharedMemoryBuffer(
>    uint32 buffer_size,
>    base::SharedMemoryHandle* handle) {
>  base::SharedMemory shared_buf;
>  if (!shared_buf.**CreateAndMapAnonymous(buffer_**size)) {
>    *handle = base::SharedMemory::**NULLHandle();
>    NOTREACHED() << "Cannot map shared memory buffer";
>    return;
>  }
>  shared_buf.GiveToProcess(peer_**handle(), handle);
> }
>
>
>
http://codereview.chromium.**org/7974011/<http://codereview.chromium.org/7974...
>

[michaeln, 2011-10-03 20:43:19]

Oh... but not on windows...

  // Allocate a shared memory buffer to hold the bitmap bits.
#if defined(OS_POSIX)
  // On POSIX, we need to ask the browser to create the shared memory for us,
  // since this is blocked by the sandbox.
  base::SharedMemoryHandle shared_mem_handle;
  ViewHostMsg_AllocateSharedMemoryBuffer *msg =
      new ViewHostMsg_AllocateSharedMemoryBuffer(buf_size,
                                                 &shared_mem_handle);
  if (RenderThread::current()->Send(msg)) {
    if (base::SharedMemory::IsHandleValid(shared_mem_handle)) {
      shared_buf_ = new base::SharedMemory(shared_mem_handle, false);
      if (!shared_buf_ || !shared_buf_->Map(buf_size)) {
        NOTREACHED() << "Map failed";
        return;
      }
    } else {
      NOTREACHED() << "Browser failed to allocate shared memory";
      return;
    }
  } else {
    NOTREACHED() << "Browser allocation request message failed";
    return;
  }
#else  // !OS_POSIX
  shared_buf_ = new base::SharedMemory;
  if (!shared_buf_->CreateAndMapAnonymous(buf_size)) {
    NOTREACHED();
    return;
  }
#endif

[jam, 2011-10-03 22:30:41]

On Mon, Oct 3, 2011 at 1:43 PM, <michaeln@chromium.org> wrote:

> Oh... but not on windows...
>

yep, on windows no ipc is needed. there should be a few examples


>
>  // Allocate a shared memory buffer to hold the bitmap bits.
> #if defined(OS_POSIX)
>  // On POSIX, we need to ask the browser to create the shared memory for
> us,
>  // since this is blocked by the sandbox.
>  base::SharedMemoryHandle shared_mem_handle;
>  ViewHostMsg_**AllocateSharedMemoryBuffer *msg =
>      new ViewHostMsg_**AllocateSharedMemoryBuffer(**buf_size,
>                                                 &shared_mem_handle);
>  if (RenderThread::current()->**Send(msg)) {
>    if (base::SharedMemory::**IsHandleValid(shared_mem_**handle)) {
>      shared_buf_ = new base::SharedMemory(shared_mem_**handle, false);
>      if (!shared_buf_ || !shared_buf_->Map(buf_size)) {
>        NOTREACHED() << "Map failed";
>        return;
>      }
>    } else {
>      NOTREACHED() << "Browser failed to allocate shared memory";
>      return;
>    }
>  } else {
>    NOTREACHED() << "Browser allocation request message failed";
>    return;
>  }
> #else  // !OS_POSIX
>  shared_buf_ = new base::SharedMemory;
>  if (!shared_buf_->**CreateAndMapAnonymous(buf_**size)) {
>    NOTREACHED();
>    return;
>  }
> #endif
>
>
http://codereview.chromium.**org/7974011/<http://codereview.chromium.org/7974...
>

[michaeln, 2011-10-11 01:08:13]

> Shared memory... I'll look into doing just that.

Ok, additional changes to xfer larger blocks of BlobBuilder data via shared
memory.

[jam, 2011-10-11 01:56:51]

http://codereview.chromium.org/7974011/diff/59019/content/common/webblob_mess...
File content/common/webblob_messages.h (right):

http://codereview.chromium.org/7974011/diff/59019/content/common/webblob_mess...
content/common/webblob_messages.h:40:
IPC_SYNC_MESSAGE_CONTROL1_1(BlobHostMsg_SyncAllocateSharedMemory,
no need to duplicate this. you can reuse the existing message in view_messages.h
(ViewHostMsg_AllocateSharedMemoryBuffer)

http://codereview.chromium.org/7974011/diff/59019/content/common/webblobregis...
File content/common/webblobregistry_impl.cc (right):

http://codereview.chromium.org/7974011/diff/59019/content/common/webblobregis...
content/common/webblobregistry_impl.cc:45: const size_t kMaxSharedMemorySize =
10 * 1024 * 1024;
now that you're using shared memory, why are you still looping for 10MB at a
time instead of just sending it all at once?

[jam, 2011-10-11 02:02:18]

http://codereview.chromium.org/7974011/diff/59019/content/common/webblob_mess...
File content/common/webblob_messages.h (right):

http://codereview.chromium.org/7974011/diff/59019/content/common/webblob_mess...
content/common/webblob_messages.h:40:
IPC_SYNC_MESSAGE_CONTROL1_1(BlobHostMsg_SyncAllocateSharedMemory,
On 2011/10/11 01:56:51, John Abd-El-Malek wrote:
> no need to duplicate this. you can reuse the existing message in
view_messages.h
> (ViewHostMsg_AllocateSharedMemoryBuffer)

just to be clear, we should reuse not just the message but also the dispatching
function. so if this blob code is used in both renderer+worker, the impl of the
function can move to ChildProcessHost from RenderMessageFilter, and it can be a
ChildHostMsg instead

[michaeln, 2011-10-11 02:05:26]

> function can move to ChildProcessHost from RenderMessageFilter, and
> it can be a ChildHostMsg instead

thnx, it wasn't clear to me where to put it such that it would be shared

[michaeln, 2011-10-11 02:15:29]

http://codereview.chromium.org/7974011/diff/59019/content/common/webblobregis...
File content/common/webblobregistry_impl.cc (right):

http://codereview.chromium.org/7974011/diff/59019/content/common/webblobregis...
content/common/webblobregistry_impl.cc:45: const size_t kMaxSharedMemorySize =
10 * 1024 * 1024;
On 2011/10/11 01:56:51, John Abd-El-Malek wrote:
> now that you're using shared memory, why are you still looping for 10MB at a
> time instead of just sending it all at once?

Right now, mostly to avoid very large contiguous blocks. But eventually, the
brower-side should spill over to disk as the blob grows, feeding it
incrementally should help make that go more in parallel.

Maybe 10M is too small, but some upper bound would be good i think.

WebCore::BlobBuilder doesn't call out to chrome until it's ready push the whole
thing over. I intend to make that code more incremental too, calling out to
chrome prior to the whole thing being done.

[michaeln, 2011-10-11 02:41:05]

> > function can move to ChildProcessHost from RenderMessageFilter, and
> > it can be a ChildHostMsg instead
> 
> thnx, it wasn't clear to me where to put it such that it would be shared

Hold on... putting this in ChildProcessHost doesn't work for renderers since
RenderProcessHost does not derive from it.

// [Browser]RenderProcessHost is the main exception that doesn't derive from
// this class. That project lives on the UI thread.
class CONTENT_EXPORT BrowserChildProcessHost :
    public ChildProcessHost,
    public ChildProcessInfo,
    public ChildProcessLauncher::Client,
    public base::WaitableEventWatcher::Delegate

class CONTENT_EXPORT RenderProcessHost : public IPC::Channel::Sender,
                                         public IPC::Channel::Listener


I guess i'll just keep making this CL bigger by adding a new
SharedMemoryMsgFilter class and add that filter to all child process hosts
including the render process host. Originally, the CL was a small change to
break up a large ipc into a series of small ipcs. It then grew to renaming a
bunch of things and now to refactoring how/where shared mem utlility messages
are handled.

[michaeln, 2011-10-11 02:59:29]

Before i add files and update gypi's and such... wdyt?

Here's the code to be shared on the browser-side
 
void SharedMemoryMessageFilter::OnAllocateSharedMemory(
    size_t buffer_size,
    base::SharedMemoryHandle* handle) {
#if defined(OS_WIN)
  NOTREACHED();
#else
  base::SharedMemory shared_buf;
  if (!shared_buf.CreateAndMapAnonymous(buffer_size)) {
    *handle = base::SharedMemory::NULLHandle();
    NOTREACHED() << "Cannot map shared memory buffer";
    return;
  }
  shared_buf.GiveToProcess(peer_handle(), handle);
#endif
}

Here's the code to be shared on the client-side.

base::SharedMemory* ChildThread::AllocateSharedMemory(
    size_t buf_size) {
  scoped_ptr<base::SharedMemory> shared_buf;
#if defined(OS_WIN)
  shared_buf.reset(new base::SharedMemory);
  if (!shared_buf->CreateAndMapAnonymous(buf_size)) {
    NOTREACHED();
    return NULL;
  }
#else
  // On POSIX, we need to ask the browser to create the shared memory for us,
  // since this is blocked by the sandbox.
  base::SharedMemoryHandle shared_mem_handle;
  if (this->Send(new SharedMemoryMsg_SyncAllocateSharedMemory(
                            buf_size, &shared_mem_handle))) {
    if (base::SharedMemory::IsHandleValid(shared_mem_handle)) {
      shared_buf.reset(new base::SharedMemory(shared_mem_handle, false));
      if (!shared_buf->Map(buf_size)) {
        NOTREACHED() << "Map failed";
        return NULL;
      }
    } else {
      NOTREACHED() << "Browser failed to allocate shared memory";
      return NULL;
    }
  } else {
    NOTREACHED() << "Browser allocation request message failed";
    return NULL;
  }
#endif
  return shared_buf.release();
}

[jam, 2011-10-11 15:48:52]

On 2011/10/11 02:15:29, michaeln wrote:
>
http://codereview.chromium.org/7974011/diff/59019/content/common/webblobregis...
> File content/common/webblobregistry_impl.cc (right):
> 
>
http://codereview.chromium.org/7974011/diff/59019/content/common/webblobregis...
> content/common/webblobregistry_impl.cc:45: const size_t kMaxSharedMemorySize =
> 10 * 1024 * 1024;
> On 2011/10/11 01:56:51, John Abd-El-Malek wrote:
> > now that you're using shared memory, why are you still looping for 10MB at a
> > time instead of just sending it all at once?
> 
> Right now, mostly to avoid very large contiguous blocks. But eventually, the
> brower-side should spill over to disk as the blob grows, feeding it
> incrementally should help make that go more in parallel.
> 
> Maybe 10M is too small, but some upper bound would be good i think.
> 
> WebCore::BlobBuilder doesn't call out to chrome until it's ready push the
whole
> thing over. I intend to make that code more incremental too, calling out to
> chrome prior to the whole thing being done.

I'm not familiar with the spec. Is there no limit to how big blobs can be?

What about in the browser side, does it get written to disk in chunks or is it
ever in memory all at once (either at writing or at reading)?

[jam, 2011-10-11 15:50:31]

On 2011/10/11 02:41:05, michaeln wrote:
> > > function can move to ChildProcessHost from RenderMessageFilter, and
> > > it can be a ChildHostMsg instead
> > 
> > thnx, it wasn't clear to me where to put it such that it would be shared
> 
> Hold on... putting this in ChildProcessHost doesn't work for renderers since
> RenderProcessHost does not derive from it.
> 
> // [Browser]RenderProcessHost is the main exception that doesn't derive from
> // this class. That project lives on the UI thread.
> class CONTENT_EXPORT BrowserChildProcessHost :
>     public ChildProcessHost,
>     public ChildProcessInfo,
>     public ChildProcessLauncher::Client,
>     public base::WaitableEventWatcher::Delegate
> 
> class CONTENT_EXPORT RenderProcessHost : public IPC::Channel::Sender,
>                                          public IPC::Channel::Listener
> 
> 
> I guess i'll just keep making this CL bigger by adding a new
> SharedMemoryMsgFilter class and add that filter to all child process hosts
> including the render process host. Originally, the CL was a small change to
> break up a large ipc into a series of small ipcs. It then grew to renaming a
> bunch of things and now to refactoring how/where shared mem utlility messages
> are handled.

I had a reminder to correct myself in the morning, but you beat me to it :) For
small code like this (i.e. few lines) you can just copy the implementation from
RMF to ChildProcessHost.

[jam, 2011-10-11 16:06:48]

On 2011/10/11 02:41:05, michaeln wrote:
> > > function can move to ChildProcessHost from RenderMessageFilter, and
> > > it can be a ChildHostMsg instead
> > 
> > thnx, it wasn't clear to me where to put it such that it would be shared
> 
> Hold on... putting this in ChildProcessHost doesn't work for renderers since
> RenderProcessHost does not derive from it.
> 
> // [Browser]RenderProcessHost is the main exception that doesn't derive from
> // this class. That project lives on the UI thread.
> class CONTENT_EXPORT BrowserChildProcessHost :
>     public ChildProcessHost,
>     public ChildProcessInfo,
>     public ChildProcessLauncher::Client,
>     public base::WaitableEventWatcher::Delegate
> 
> class CONTENT_EXPORT RenderProcessHost : public IPC::Channel::Sender,
>                                          public IPC::Channel::Listener
> 
> 
> I guess i'll just keep making this CL bigger by adding a new
> SharedMemoryMsgFilter class and add that filter to all child process hosts
> including the render process host. Originally, the CL was a small change to
> break up a large ipc into a series of small ipcs. It then grew to renaming a
> bunch of things and now to refactoring how/where shared mem utlility messages
> are handled.

It's up to you if you want to break it into smaller pieces. Those will be easier
to review and land as usual.

[michaeln, 2011-10-11 18:29:08]

> I'm not familiar with the spec. Is there no limit to how big blobs can be?

No. This abstraction is supposed to support large amounts of data.

> What about in the browser side, does it get written to disk in chunks or is it
> ever in memory all at once (either at writing or at reading)?

Our impl isn't done yet.

Memory backed data items don't spill over to disk at all yet, for now, this CL
puts in a hard limit on how much memory backed blob data will be allowed. For
reading, the existing infrastructure will incrementally read from disk (for
file-backed blobs). For writing, if/when that gets done, it should also be
spooled incrementally... chunking blob-builder activity is part of that.

[michaeln, 2011-10-12 01:46:35]

> It's up to you if you want to break it into smaller pieces. Those will be
easier
> to review and land as usual.

This CL is blocked on another to share the shared memory allocation msg
sending/handling.
http://codereview.chromium.org/8229039/

[michaeln, 2011-10-15 01:17:32]

> This CL is blocked on another to share the shared memory allocation msg
> sending/handling.
> http://codereview.chromium.org/8229039/

That CL is committed now, ptal.

[jam, 2011-10-15 01:53:15]

lgtm with nits (I only concentrated on the IPC parts), thanks for converting
this over to shared memory

http://codereview.chromium.org/7974011/diff/74001/content/common/webblob_mess...
File content/common/webblob_messages.h (right):

http://codereview.chromium.org/7974011/diff/74001/content/common/webblob_mess...
content/common/webblob_messages.h:39:
IPC_SYNC_MESSAGE_CONTROL3_0(BlobHostMsg_SyncAppendSharedMemory,
nit: we don't put "Sync" in sync IPCs by convention

http://codereview.chromium.org/7974011/diff/74001/content/common/webblobregis...
File content/common/webblobregistry_impl.cc (right):

http://codereview.chromium.org/7974011/diff/74001/content/common/webblobregis...
content/common/webblobregistry_impl.cc:39: const size_t kLargeSizeThreshold =
250 * 1024;
nit: constants usually go at the top of the file. it's also convention to
specify what units the constant is in, i.e. kLargeSizeThresholdKB

http://codereview.chromium.org/7974011/diff/74001/content/common/webblobregis...
content/common/webblobregistry_impl.cc:46: const size_t kMaxSharedMemorySize =
10 * 1024 * 1024;
ditto

[jam, 2011-10-18 18:26:18]

On 2011/10/15 01:53:15, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/7974011/diff/74001/content/common/webblob_mess...
> content/common/webblob_messages.h:39:
> IPC_SYNC_MESSAGE_CONTROL3_0(BlobHostMsg_SyncAppendSharedMemory,
> nit: we don't put "Sync" in sync IPCs by convention

hi, looks like this wasn't done. can you please do it in a followup?

[michaeln, 2011-10-18 21:15:27]

Since the msg doesn't have a return value, it's not apparent at the callsite
that it blocks, so i thought it was good to keep 'Sync' in the name to make it
clear when looking at a callsite.

> hi, looks like this wasn't done. can you please do it in a followup?