[WIP] Provided file system from NACL.

[WIP] Provided file system from NACL.

BUG=None
TEST=None

[sabin.floares, 2015-04-21 16:43:36]

sabin.floares@intel.com changed reviewers:
+ lionel.g.landwerlin@intel.com, robert.bradford@intel.com

[llandwerlin-old, 2015-06-03 11:15:55]

https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
File ppapi/api/dev/ppb_filesystemprovider_dev.idl (right):

https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
ppapi/api/dev/ppb_filesystemprovider_dev.idl:14: enum PP_ProviderError_Dev {
You should probably use the errors codes defined in pp_errors.idl.
And add new ones there is you need to.

https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
ppapi/api/dev/ppb_filesystemprovider_dev.idl:81: char[256] entry_path;
I'm sorry, when we discussed this last time, I understood that the strings in
these structures would be names, not paths. Names are limited (256 characters on
unix systems), but paths can be indefinitely long. Again, I'm sorry I misled you
there.

I guess you could keep the structures mostly as they are and just replace the
char[256] by mem_t (you might be able to keep char[] in PP_EntryMetadata_Dev but
only if this is specified somewhere in the filesystem provider documentation).
Then instead of passing a 

[out] PP_FilesystemRequest request

argument to GetNextRequest() you could pass a

[in] PP_ArrayOutput output

which would be used by the proxy to allocate the right size for structure +
strings. In memory the layout would look something like this :

[ sizeof(PP_FilesystemRequestValue) | size(string1) | string(string2) | ... ]
(for now 2 is the maximum number of strings you have to deal with).

For example in the case of PP_FilesystemRequestCopyEntry this would be the
sizeof PP_FilesystemRequestValue + the size of source_path + size of
target_path.

PP_FilesystemRequestCopyEntry would look like this :

struct PP_FilesystemRequestCopyEntry {
  mem_t source_path;
  mem_t target_path; 
};

Where source_path points at sizeof(PP_FilesystemRequestValue) and target_path
points at sizeof(PP_FilesystemRequestValue) + sizeof(source_path).

https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
ppapi/api/dev/ppb_filesystemprovider_dev.idl:163: struct PP_FilesystemRequest{
Why not having a PP_FilesystemResponse structure as well?
You could remove most of the Send* methods of the interface.

https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
ppapi/api/dev/ppb_filesystemprovider_dev.idl:296: [in, size_is(data_size)] mem_t
data,
It seems you have shared memory for data coming from the browser to the plugin.
But for some reason you don't apply this principle to sending data from the
plugin to the browser. Why not?

https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
ppapi/api/dev/ppb_filesystemprovider_dev.idl:315: int32_t
FreeWriteRequestBuffer(
RecycleRequestBuffer() would be a more appropriate name.

[sabin.floares, 2015-06-03 12:30:09]

Hi Lionel,

Thanks for the comments!

On 2015/06/03 11:15:55, llandwerlin wrote:
>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> File ppapi/api/dev/ppb_filesystemprovider_dev.idl (right):
> 
>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> ppapi/api/dev/ppb_filesystemprovider_dev.idl:14: enum PP_ProviderError_Dev {
> You should probably use the errors codes defined in pp_errors.idl.
> And add new ones there is you need to.

Acknowledged.

>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> ppapi/api/dev/ppb_filesystemprovider_dev.idl:81: char[256] entry_path;
> I'm sorry, when we discussed this last time, I understood that the strings in
> these structures would be names, not paths. Names are limited (256 characters
on
> unix systems), but paths can be indefinitely long. Again, I'm sorry I misled
you
> there.
> 
> I guess you could keep the structures mostly as they are and just replace the
> char[256] by mem_t (you might be able to keep char[] in PP_EntryMetadata_Dev
but
> only if this is specified somewhere in the filesystem provider documentation).
> Then instead of passing a 
> 
> [out] PP_FilesystemRequest request
> 
> argument to GetNextRequest() you could pass a
> 
> [in] PP_ArrayOutput output
> 
> which would be used by the proxy to allocate the right size for structure +
> strings. In memory the layout would look something like this :
> 
> [ sizeof(PP_FilesystemRequestValue) | size(string1) | string(string2) | ... ]
> (for now 2 is the maximum number of strings you have to deal with).
> 
> For example in the case of PP_FilesystemRequestCopyEntry this would be the
> sizeof PP_FilesystemRequestValue + the size of source_path + size of
> target_path.
> 
> PP_FilesystemRequestCopyEntry would look like this :
> 
> struct PP_FilesystemRequestCopyEntry {
>   mem_t source_path;
>   mem_t target_path; 
> };
> 
> Where source_path points at sizeof(PP_FilesystemRequestValue) and target_path
> points at sizeof(PP_FilesystemRequestValue) + sizeof(source_path).

Yes, this is a very nice alternative. At this point I only save write requests
until the user frees the request. I can save all request and use the request id
to free them.
I can keep the current format and pass the address of the std::string buffers
from each request.

>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> ppapi/api/dev/ppb_filesystemprovider_dev.idl:163: struct PP_FilesystemRequest{
> Why not having a PP_FilesystemResponse structure as well?
> You could remove most of the Send* methods of the interface.

Yes, it would be nice to do this, but I didn't find a good way to encapsulate
arrays like "PP_EntryMetadata_Dev[]". I will try to have another angle on this
based on your comments.

>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> ppapi/api/dev/ppb_filesystemprovider_dev.idl:296: [in, size_is(data_size)]
mem_t
> data,
> It seems you have shared memory for data coming from the browser to the
plugin.
> But for some reason you don't apply this principle to sending data from the
> plugin to the browser. Why not?

I actually use shared memory for sending data too. I have a single shared memory
region that is split into 2: Read half and Write half - could be asymmetric.
When I read/write from the resource or host I use offsets into this.
 
>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> ppapi/api/dev/ppb_filesystemprovider_dev.idl:315: int32_t
> FreeWriteRequestBuffer(
> RecycleRequestBuffer() would be a more appropriate name.

Acknowledged.

[llandwerlin-old, 2015-06-03 12:38:42]

On 2015/06/03 12:30:09, sabin.floares wrote:

> >
>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> > ppapi/api/dev/ppb_filesystemprovider_dev.idl:296: [in, size_is(data_size)]
> mem_t
> > data,
> > It seems you have shared memory for data coming from the browser to the
> plugin.
> > But for some reason you don't apply this principle to sending data from the
> > plugin to the browser. Why not?
> 
> I actually use shared memory for sending data too. I have a single shared
memory
> region that is split into 2: Read half and Write half - could be asymmetric.
> When I read/write from the resource or host I use offsets into this.
>  

You have a memcpy here :

https://codereview.chromium.org/1093383002/diff/120001/ppapi/proxy/filesystem...

You could get rid of it if you had a pattern like the VideoEncoder :

https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_vide...

Have a GetBuffer() method on PPB_FilesystemProvider which returns a pointer to a
piece of shared memory & length. The plugins writes directly in it, and then put
the pointer into the response structure before calling SendResponse() with it.

[sabin.floares, 2015-06-03 13:02:57]

On 2015/06/03 12:38:42, llandwerlin wrote:
> On 2015/06/03 12:30:09, sabin.floares wrote:
> 
> > >
> >
>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> > > ppapi/api/dev/ppb_filesystemprovider_dev.idl:296: [in, size_is(data_size)]
> > mem_t
> > > data,
> > > It seems you have shared memory for data coming from the browser to the
> > plugin.
> > > But for some reason you don't apply this principle to sending data from
the
> > > plugin to the browser. Why not?
> > 
> > I actually use shared memory for sending data too. I have a single shared
> memory
> > region that is split into 2: Read half and Write half - could be asymmetric.
> > When I read/write from the resource or host I use offsets into this.
> >  
> 
> You have a memcpy here :
> 
>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/proxy/filesystem...
> 
> You could get rid of it if you had a pattern like the VideoEncoder :
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_vide...
> 
> Have a GetBuffer() method on PPB_FilesystemProvider which returns a pointer to
a
> piece of shared memory & length. The plugins writes directly in it, and then
put
> the pointer into the response structure before calling SendResponse() with it.

I did saw the approach but I have this issue: when copying a directory from the
a browser backend to the plugin a maximum of 5 threads(at the moment) could
produce chunks of 512KB. The writing is serialized(if the response is not
received no further writing is done). So for this situation I need 5*512KB of
shared memory. For the other way around I need the same(plugin->browser).

It seemed a lot at the moment per plugin instance. If this is not an issue I can
drop the current approach with the shared memory access controller and I can use
this. It would greatly improve performance and simplify the design.

[sabin.floares, 2015-06-03 13:03:00]

On 2015/06/03 12:38:42, llandwerlin wrote:
> On 2015/06/03 12:30:09, sabin.floares wrote:
> 
> > >
> >
>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/api/dev/ppb_file...
> > > ppapi/api/dev/ppb_filesystemprovider_dev.idl:296: [in, size_is(data_size)]
> > mem_t
> > > data,
> > > It seems you have shared memory for data coming from the browser to the
> > plugin.
> > > But for some reason you don't apply this principle to sending data from
the
> > > plugin to the browser. Why not?
> > 
> > I actually use shared memory for sending data too. I have a single shared
> memory
> > region that is split into 2: Read half and Write half - could be asymmetric.
> > When I read/write from the resource or host I use offsets into this.
> >  
> 
> You have a memcpy here :
> 
>
https://codereview.chromium.org/1093383002/diff/120001/ppapi/proxy/filesystem...
> 
> You could get rid of it if you had a pattern like the VideoEncoder :
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/api/ppb_vide...
> 
> Have a GetBuffer() method on PPB_FilesystemProvider which returns a pointer to
a
> piece of shared memory & length. The plugins writes directly in it, and then
put
> the pointer into the response structure before calling SendResponse() with it.

I did saw the approach but I have this issue: when copying a directory from the
a browser backend to the plugin a maximum of 5 threads(at the moment) could
produce chunks of 512KB. The writing is serialized(if the response is not
received no further writing is done). So for this situation I need 5*512KB of
shared memory. For the other way around I need the same(plugin->browser).

It seemed a lot at the moment per plugin instance. If this is not an issue I can
drop the current approach with the shared memory access controller and I can use
this. It would greatly improve performance and simplify the design.

[llandwerlin-old, 2015-07-17 16:40:49]

lionel.g.landwerlin@intel.com changed reviewers:
- lionel.g.landwerlin@intel.com

[sabin.floares, 2016-10-06 12:27:30]

Description was changed from

==========
[WIP] Provided file system from NACL.

BUG=None
TEST=None
==========

to

==========
[WIP] Provided file system from NACL.

BUG=None
TEST=None
==========