Issue 12817009: Add Query() support to FileRef

teravest

7 years, 9 months ago (2013-03-18 20:27:23 UTC) #1

dmichael (off chromium)

https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h File ppapi/cpp/file_ref.h (right): https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newcode157 ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const CompletionCallback& callback); Please use "CompletionCallbackWithOutput<PP_FileInfo>" ...

7 years, 9 months ago (2013-03-18 23:16:16 UTC) #2

Yusuke Sato

Thanks for the work! drive-by non-codereview comment: I tested the new API with my PPAPI ...

7 years, 9 months ago (2013-03-19 00:01:12 UTC) #3

teravest1

On Mon, Mar 18, 2013 at 6:01 PM, <yusukes@chromium.org> wrote: > Thanks for the work! ...

7 years, 9 months ago (2013-03-19 14:42:19 UTC) #4

raymes1

> I'll take a look at sending the request directly to the browser. It > ...

7 years, 9 months ago (2013-03-19 16:30:32 UTC) #5

> I'll take a look at sending the request directly to the browser. It
> seems like this should be possible, but I don't know how ugly the code
> will look yet.

This will probably require some ugly hacks until we move file ref to
the new proxy model. (just something to keep in mind - if it's a very
urgent performance requirement please go ahead otherwise let's
consider doing the refactor first).

On Tue, Mar 19, 2013 at 7:41 AM, Justin TerAvest <teravest@google.com> wrote:
>
> On Mon, Mar 18, 2013 at 6:01 PM,  <yusukes@chromium.org> wrote:
> > Thanks for the work! drive-by non-codereview comment:
> >
> > I tested the new API with my PPAPI plugin, and found that
> > pp::FileRef::Query()
> > was more than 2 times slower than pp::FileIO::Query(). On my Z620,
> > FileRef::Query is about 3.8ms/call and FileIO::Query is about 1.5ms/call.
> >
> > chrome://tracing shows that pp::FileRef::Query() is implemented in
> > Chrome_FileThread (in the browser process) while pp::FileIO::Query() is in
> > Renderer::FILE. I know almost nothing about PPAPI internals, but would it
be
> > possible to do it in a renderer (or to send the IPC directly from the
plugin
> > to
> > the browser) so that we can eliminate the additional renderer-browser IPC
> > cost?
>
> I'll take a look at sending the request directly to the browser. It
> seems like this should be possible, but I don't know how ugly the code
> will look yet.
>
> >
> >
> >
> > On 2013/03/18 23:16:16, dmichael wrote:
> >>
> >> https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h
> >> File ppapi/cpp/file_ref.h (right):
> >
> >
> >
> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newco...
> >>
> >> ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const
> >> CompletionCallback& callback);
> >> Please use "CompletionCallbackWithOutput<PP_FileInfo>" instead
> >
> >
> >
> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages.h
> >>
> >> File ppapi/proxy/ppapi_messages.h (right):
> >
> >
> >
> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages...
> >>
> >> ppapi/proxy/ppapi_messages.h:813: PP_FileInfo /* info */)
> >> It doesn't look like you're using this message
> >
> >
> >
> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
> >
> >
> >
> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> ppapi/proxy/ppb_file_ref_proxy.cc:10: #include "base/debug/stack_trace.h"
> >> ^^ leftover from debugging
> >
> >
> >
> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
> >> PP_FileInfo*> PendingFileInfoMap;
> >> It looks like you're using |int| instead of |unsigned int| for the
> >> callback id
> >> elsewhere.
> >
> >
> >
> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> ppapi/proxy/ppb_file_ref_proxy.cc:183: PP_FileInfo *target_info =
> >
> > found->second;
> >>
> >> nit: '*' should be with PP_FileInfo
> >
> >
> >
> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
> >> sizeof(tmp_info));
> >> Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
> >> shouldn't
> >> need to special case this here
> >
> >
> >
> >
> > https://codereview.chromium.org/12817009/

teravest1

On Tue, Mar 19, 2013 at 10:30 AM, Raymes Khoury <raymes@google.com> wrote: >> I'll take ...

7 years, 9 months ago (2013-03-19 16:35:18 UTC) #6

On Tue, Mar 19, 2013 at 10:30 AM, Raymes Khoury <raymes@google.com> wrote:
>> I'll take a look at sending the request directly to the browser. It
>> seems like this should be possible, but I don't know how ugly the code
>> will look yet.
>
> This will probably require some ugly hacks until we move file ref to
> the new proxy model. (just something to keep in mind - if it's a very
> urgent performance requirement please go ahead otherwise let's
> consider doing the refactor first).

I talked with Dave a little about this earlier this morning.
In addition to refactoring to the new proxy model, there have been
some discussions about changing how FileIO works, potentially sending
file desciptors to the plugin process so it can read or write
directly. It looks like the discussion is at
http://code.google.com/p/chromium/issues/detail?id=194304

I think it makes sense to get this resolved before implementing
FileRef::Query. I'm going to do some reading up today to make sure I
understand what direction we're headed.

>
> On Tue, Mar 19, 2013 at 7:41 AM, Justin TerAvest <teravest@google.com> wrote:
>>
>> On Mon, Mar 18, 2013 at 6:01 PM,  <yusukes@chromium.org> wrote:
>> > Thanks for the work! drive-by non-codereview comment:
>> >
>> > I tested the new API with my PPAPI plugin, and found that
>> > pp::FileRef::Query()
>> > was more than 2 times slower than pp::FileIO::Query(). On my Z620,
>> > FileRef::Query is about 3.8ms/call and FileIO::Query is about 1.5ms/call.
>> >
>> > chrome://tracing shows that pp::FileRef::Query() is implemented in
>> > Chrome_FileThread (in the browser process) while pp::FileIO::Query() is in
>> > Renderer::FILE. I know almost nothing about PPAPI internals, but would it
be
>> > possible to do it in a renderer (or to send the IPC directly from the
plugin
>> > to
>> > the browser) so that we can eliminate the additional renderer-browser IPC
>> > cost?
>>
>> I'll take a look at sending the request directly to the browser. It
>> seems like this should be possible, but I don't know how ugly the code
>> will look yet.
>>
>> >
>> >
>> >
>> > On 2013/03/18 23:16:16, dmichael wrote:
>> >>
>> >> https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h
>> >> File ppapi/cpp/file_ref.h (right):
>> >
>> >
>> >
>> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newco...
>> >>
>> >> ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const
>> >> CompletionCallback& callback);
>> >> Please use "CompletionCallbackWithOutput<PP_FileInfo>" instead
>> >
>> >
>> >
>> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages.h
>> >>
>> >> File ppapi/proxy/ppapi_messages.h (right):
>> >
>> >
>> >
>> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages...
>> >>
>> >> ppapi/proxy/ppapi_messages.h:813: PP_FileInfo /* info */)
>> >> It doesn't look like you're using this message
>> >
>> >
>> >
>> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >>
>> >> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>> >
>> >
>> >
>> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >>
>> >> ppapi/proxy/ppb_file_ref_proxy.cc:10: #include "base/debug/stack_trace.h"
>> >> ^^ leftover from debugging
>> >
>> >
>> >
>> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >>
>> >> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
>> >> PP_FileInfo*> PendingFileInfoMap;
>> >> It looks like you're using |int| instead of |unsigned int| for the
>> >> callback id
>> >> elsewhere.
>> >
>> >
>> >
>> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >>
>> >> ppapi/proxy/ppb_file_ref_proxy.cc:183: PP_FileInfo *target_info =
>> >
>> > found->second;
>> >>
>> >> nit: '*' should be with PP_FileInfo
>> >
>> >
>> >
>> >
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >>
>> >> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
>> >> sizeof(tmp_info));
>> >> Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
>> >> shouldn't
>> >> need to special case this here
>> >
>> >
>> >
>> >
>> > https://codereview.chromium.org/12817009/

yzshen

Hi, Justin. I have a question: It looks like this change will directly go to ...

7 years, 9 months ago (2013-03-19 16:41:19 UTC) #7

Hi, Justin.

I have a question: It looks like this change will directly go to stable. I
thought we usually put new interfaces in dev first. Have we considered
it thoroughly before doing so?

Thanks!


On Tue, Mar 19, 2013 at 9:34 AM, Justin TerAvest <teravest@google.com>wrote:

> On Tue, Mar 19, 2013 at 10:30 AM, Raymes Khoury <raymes@google.com> wrote:
> >> I'll take a look at sending the request directly to the browser. It
> >> seems like this should be possible, but I don't know how ugly the code
> >> will look yet.
> >
> > This will probably require some ugly hacks until we move file ref to
> > the new proxy model. (just something to keep in mind - if it's a very
> > urgent performance requirement please go ahead otherwise let's
> > consider doing the refactor first).
>
> I talked with Dave a little about this earlier this morning.
> In addition to refactoring to the new proxy model, there have been
> some discussions about changing how FileIO works, potentially sending
> file desciptors to the plugin process so it can read or write
> directly. It looks like the discussion is at
> http://code.google.com/p/chromium/issues/detail?id=194304
>
> I think it makes sense to get this resolved before implementing
> FileRef::Query. I'm going to do some reading up today to make sure I
> understand what direction we're headed.
>
> >
> > On Tue, Mar 19, 2013 at 7:41 AM, Justin TerAvest <teravest@google.com>
> wrote:
> >>
> >> On Mon, Mar 18, 2013 at 6:01 PM,  <yusukes@chromium.org> wrote:
> >> > Thanks for the work! drive-by non-codereview comment:
> >> >
> >> > I tested the new API with my PPAPI plugin, and found that
> >> > pp::FileRef::Query()
> >> > was more than 2 times slower than pp::FileIO::Query(). On my Z620,
> >> > FileRef::Query is about 3.8ms/call and FileIO::Query is about
> 1.5ms/call.
> >> >
> >> > chrome://tracing shows that pp::FileRef::Query() is implemented in
> >> > Chrome_FileThread (in the browser process) while pp::FileIO::Query()
> is in
> >> > Renderer::FILE. I know almost nothing about PPAPI internals, but
> would it be
> >> > possible to do it in a renderer (or to send the IPC directly from the
> plugin
> >> > to
> >> > the browser) so that we can eliminate the additional renderer-browser
> IPC
> >> > cost?
> >>
> >> I'll take a look at sending the request directly to the browser. It
> >> seems like this should be possible, but I don't know how ugly the code
> >> will look yet.
> >>
> >> >
> >> >
> >> >
> >> > On 2013/03/18 23:16:16, dmichael wrote:
> >> >>
> >> >>
> https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h
> >> >> File ppapi/cpp/file_ref.h (right):
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newco...
> >> >>
> >> >> ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const
> >> >> CompletionCallback& callback);
> >> >> Please use "CompletionCallbackWithOutput<PP_FileInfo>" instead
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages.h
> >> >>
> >> >> File ppapi/proxy/ppapi_messages.h (right):
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages...
> >> >>
> >> >> ppapi/proxy/ppapi_messages.h:813: PP_FileInfo /* info */)
> >> >> It doesn't look like you're using this message
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:10: #include
> "base/debug/stack_trace.h"
> >> >> ^^ leftover from debugging
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
> >> >> PP_FileInfo*> PendingFileInfoMap;
> >> >> It looks like you're using |int| instead of |unsigned int| for the
> >> >> callback id
> >> >> elsewhere.
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:183: PP_FileInfo *target_info =
> >> >
> >> > found->second;
> >> >>
> >> >> nit: '*' should be with PP_FileInfo
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
> >> >> sizeof(tmp_info));
> >> >> Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
> >> >> shouldn't
> >> >> need to special case this here
> >> >
> >> >
> >> >
> >> >
> >> > https://codereview.chromium.org/12817009/
>



-- 
Best regards,
Yuzhu Shen.

dmichael (off chromium)

On Tue, Mar 19, 2013 at 10:41 AM, Yuzhu Shen <yzshen@google.com> wrote: > Hi, Justin. ...

7 years, 9 months ago (2013-03-19 16:51:53 UTC) #8

On Tue, Mar 19, 2013 at 10:41 AM, Yuzhu Shen <yzshen@google.com> wrote:

> Hi, Justin.
>
> I have a question: It looks like this change will directly go to stable. I
> thought we usually put new interfaces in dev first. Have we considered
> it thoroughly before doing so?
>
This isn't a new interface, just a new function. We don't really have a
good process for adding a function in "Dev" to a stable interface. I guess
we could have a parallel Dev interface that works with the same
PP_Resource? But we don't normally do that.

For justification, see discussion on the bug:
https://code.google.com/p/chromium/issues/detail?id=170721
and in this pepper-team thread (internal only, sorry):
https://groups.google.com/a/google.com/d/msg/pepper-team/vbTKXZJ0zec/JxEu_VqO...

We want it for two reasons:
1) So you can query a Directory, which is currently not possible AFAIK
2) To be able to query faster, since you don't have to create a FileIO to
do it.

It sounds like this doesn't really solve 2 yet. So we should probably work
towards refactoring this interface and doing the FileIO implementation
changes we talked about before adding this API.



>
> Thanks!
>
>
> On Tue, Mar 19, 2013 at 9:34 AM, Justin TerAvest <teravest@google.com>wrote:
>
>> On Tue, Mar 19, 2013 at 10:30 AM, Raymes Khoury <raymes@google.com>
>> wrote:
>> >> I'll take a look at sending the request directly to the browser. It
>> >> seems like this should be possible, but I don't know how ugly the code
>> >> will look yet.
>> >
>> > This will probably require some ugly hacks until we move file ref to
>> > the new proxy model. (just something to keep in mind - if it's a very
>> > urgent performance requirement please go ahead otherwise let's
>> > consider doing the refactor first).
>>
>> I talked with Dave a little about this earlier this morning.
>> In addition to refactoring to the new proxy model, there have been
>> some discussions about changing how FileIO works, potentially sending
>> file desciptors to the plugin process so it can read or write
>> directly. It looks like the discussion is at
>> http://code.google.com/p/chromium/issues/detail?id=194304
>>
>> I think it makes sense to get this resolved before implementing
>> FileRef::Query. I'm going to do some reading up today to make sure I
>> understand what direction we're headed.
>>
>> >
>> > On Tue, Mar 19, 2013 at 7:41 AM, Justin TerAvest <teravest@google.com>
>> wrote:
>> >>
>> >> On Mon, Mar 18, 2013 at 6:01 PM,  <yusukes@chromium.org> wrote:
>> >> > Thanks for the work! drive-by non-codereview comment:
>> >> >
>> >> > I tested the new API with my PPAPI plugin, and found that
>> >> > pp::FileRef::Query()
>> >> > was more than 2 times slower than pp::FileIO::Query(). On my Z620,
>> >> > FileRef::Query is about 3.8ms/call and FileIO::Query is about
>> 1.5ms/call.
>> >> >
>> >> > chrome://tracing shows that pp::FileRef::Query() is implemented in
>> >> > Chrome_FileThread (in the browser process) while pp::FileIO::Query()
>> is in
>> >> > Renderer::FILE. I know almost nothing about PPAPI internals, but
>> would it be
>> >> > possible to do it in a renderer (or to send the IPC directly from
>> the plugin
>> >> > to
>> >> > the browser) so that we can eliminate the additional
>> renderer-browser IPC
>> >> > cost?
>> >>
>> >> I'll take a look at sending the request directly to the browser. It
>> >> seems like this should be possible, but I don't know how ugly the code
>> >> will look yet.
>> >>
>> >> >
>> >> >
>> >> >
>> >> > On 2013/03/18 23:16:16, dmichael wrote:
>> >> >>
>> >> >>
>> https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h
>> >> >> File ppapi/cpp/file_ref.h (right):
>> >> >
>> >> >
>> >> >
>> >> >
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newco...
>> >> >>
>> >> >> ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const
>> >> >> CompletionCallback& callback);
>> >> >> Please use "CompletionCallbackWithOutput<PP_FileInfo>" instead
>> >> >
>> >> >
>> >> >
>> >> >
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages.h
>> >> >>
>> >> >> File ppapi/proxy/ppapi_messages.h (right):
>> >> >
>> >> >
>> >> >
>> >> >
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages...
>> >> >>
>> >> >> ppapi/proxy/ppapi_messages.h:813: PP_FileInfo /* info */)
>> >> >> It doesn't look like you're using this message
>> >> >
>> >> >
>> >> >
>> >> >
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >> >>
>> >> >> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>> >> >
>> >> >
>> >> >
>> >> >
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >> >>
>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:10: #include
>> "base/debug/stack_trace.h"
>> >> >> ^^ leftover from debugging
>> >> >
>> >> >
>> >> >
>> >> >
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >> >>
>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
>> >> >> PP_FileInfo*> PendingFileInfoMap;
>> >> >> It looks like you're using |int| instead of |unsigned int| for the
>> >> >> callback id
>> >> >> elsewhere.
>> >> >
>> >> >
>> >> >
>> >> >
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >> >>
>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:183: PP_FileInfo *target_info =
>> >> >
>> >> > found->second;
>> >> >>
>> >> >> nit: '*' should be with PP_FileInfo
>> >> >
>> >> >
>> >> >
>> >> >
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> >> >>
>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
>> >> >> sizeof(tmp_info));
>> >> >> Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
>> >> >> shouldn't
>> >> >> need to special case this here
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > https://codereview.chromium.org/12817009/
>>
>
>
>
> --
> Best regards,
> Yuzhu Shen.
>

yzshen

On Tue, Mar 19, 2013 at 9:51 AM, David Michael <dmichael@chromium.org>wrote: > > > > ...

7 years, 9 months ago (2013-03-19 17:06:31 UTC) #9

On Tue, Mar 19, 2013 at 9:51 AM, David Michael <dmichael@chromium.org>wrote:

>
>
>
> On Tue, Mar 19, 2013 at 10:41 AM, Yuzhu Shen <yzshen@google.com> wrote:
>
>> Hi, Justin.
>>
>> I have a question: It looks like this change will directly go to stable.
>> I thought we usually put new interfaces in dev first. Have we considered
>> it thoroughly before doing so?
>>
> This isn't a new interface, just a new function. We don't really have a
> good process for adding a function in "Dev" to a stable interface. I guess
> we could have a parallel Dev interface that works with the same
> PP_Resource? But we don't normally do that.
>
Yes. I think it is easy to have a parallel PPB_FileRef_Dev v1.1 that works
with the same PP_Resource (if we want to).


>
> For justification, see discussion on the bug:
> https://code.google.com/p/chromium/issues/detail?id=170721
> and in this pepper-team thread (internal only, sorry):
>
>
https://groups.google.com/a/google.com/d/msg/pepper-team/vbTKXZJ0zec/JxEu_VqO...
>
> We want it for two reasons:
> 1) So you can query a Directory, which is currently not possible AFAIK
> 2) To be able to query faster, since you don't have to create a FileIO to
> do it.
>
> It sounds like this doesn't really solve 2 yet. So we should probably work
> towards refactoring this interface and doing the FileIO implementation
> changes we talked about before adding this API.
>
> Thanks for explaining!
I am fine with this change if it has been considered carefully.

I just want to make sure people have thought about it. Not all changes to
stable interfaces are 'stable' automatically. We should put them in dev if
they need some time to stabilize.

>
>
>>
>> Thanks!
>>
>>
>> On Tue, Mar 19, 2013 at 9:34 AM, Justin TerAvest <teravest@google.com>wrote:
>>
>>> On Tue, Mar 19, 2013 at 10:30 AM, Raymes Khoury <raymes@google.com>
>>> wrote:
>>> >> I'll take a look at sending the request directly to the browser. It
>>> >> seems like this should be possible, but I don't know how ugly the code
>>> >> will look yet.
>>> >
>>> > This will probably require some ugly hacks until we move file ref to
>>> > the new proxy model. (just something to keep in mind - if it's a very
>>> > urgent performance requirement please go ahead otherwise let's
>>> > consider doing the refactor first).
>>>
>>> I talked with Dave a little about this earlier this morning.
>>> In addition to refactoring to the new proxy model, there have been
>>> some discussions about changing how FileIO works, potentially sending
>>> file desciptors to the plugin process so it can read or write
>>> directly. It looks like the discussion is at
>>> http://code.google.com/p/chromium/issues/detail?id=194304
>>>
>>> I think it makes sense to get this resolved before implementing
>>> FileRef::Query. I'm going to do some reading up today to make sure I
>>> understand what direction we're headed.
>>>
>>> >
>>> > On Tue, Mar 19, 2013 at 7:41 AM, Justin TerAvest <teravest@google.com>
>>> wrote:
>>> >>
>>> >> On Mon, Mar 18, 2013 at 6:01 PM,  <yusukes@chromium.org> wrote:
>>> >> > Thanks for the work! drive-by non-codereview comment:
>>> >> >
>>> >> > I tested the new API with my PPAPI plugin, and found that
>>> >> > pp::FileRef::Query()
>>> >> > was more than 2 times slower than pp::FileIO::Query(). On my Z620,
>>> >> > FileRef::Query is about 3.8ms/call and FileIO::Query is about
>>> 1.5ms/call.
>>> >> >
>>> >> > chrome://tracing shows that pp::FileRef::Query() is implemented in
>>> >> > Chrome_FileThread (in the browser process) while
>>> pp::FileIO::Query() is in
>>> >> > Renderer::FILE. I know almost nothing about PPAPI internals, but
>>> would it be
>>> >> > possible to do it in a renderer (or to send the IPC directly from
>>> the plugin
>>> >> > to
>>> >> > the browser) so that we can eliminate the additional
>>> renderer-browser IPC
>>> >> > cost?
>>> >>
>>> >> I'll take a look at sending the request directly to the browser. It
>>> >> seems like this should be possible, but I don't know how ugly the code
>>> >> will look yet.
>>> >>
>>> >> >
>>> >> >
>>> >> >
>>> >> > On 2013/03/18 23:16:16, dmichael wrote:
>>> >> >>
>>> >> >>
>>> https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h
>>> >> >> File ppapi/cpp/file_ref.h (right):
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newco...
>>> >> >>
>>> >> >> ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const
>>> >> >> CompletionCallback& callback);
>>> >> >> Please use "CompletionCallbackWithOutput<PP_FileInfo>" instead
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages.h
>>> >> >>
>>> >> >> File ppapi/proxy/ppapi_messages.h (right):
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages...
>>> >> >>
>>> >> >> ppapi/proxy/ppapi_messages.h:813: PP_FileInfo /* info */)
>>> >> >> It doesn't look like you're using this message
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>> >> >>
>>> >> >> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>> >> >>
>>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:10: #include
>>> "base/debug/stack_trace.h"
>>> >> >> ^^ leftover from debugging
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>> >> >>
>>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned
>>> int,
>>> >> >> PP_FileInfo*> PendingFileInfoMap;
>>> >> >> It looks like you're using |int| instead of |unsigned int| for the
>>> >> >> callback id
>>> >> >> elsewhere.
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>> >> >>
>>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:183: PP_FileInfo *target_info =
>>> >> >
>>> >> > found->second;
>>> >> >>
>>> >> >> nit: '*' should be with PP_FileInfo
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>> >> >>
>>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
>>> >> >> sizeof(tmp_info));
>>> >> >> Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
>>> >> >> shouldn't
>>> >> >> need to special case this here
>>> >> >
>>> >> >
>>> >> >
>>> >> >
>>> >> > https://codereview.chromium.org/12817009/
>>>
>>
>>
>>
>> --
>> Best regards,
>> Yuzhu Shen.
>>
>
>


-- 
Best regards,
Yuzhu Shen.

Yusuke Sato

On 2013/03/19 16:35:18, teravest_google.com wrote: > On Tue, Mar 19, 2013 at 10:30 AM, Raymes ...

7 years, 9 months ago (2013-03-19 21:44:06 UTC) #10

On 2013/03/19 16:35:18, teravest_google.com wrote:
> On Tue, Mar 19, 2013 at 10:30 AM, Raymes Khoury <mailto:raymes@google.com>
wrote:
> >> I'll take a look at sending the request directly to the browser. It
> >> seems like this should be possible, but I don't know how ugly the code
> >> will look yet.
> >
> > This will probably require some ugly hacks until we move file ref to
> > the new proxy model. (just something to keep in mind - if it's a very
> > urgent performance requirement please go ahead otherwise let's
> > consider doing the refactor first).

I think our (internal) project, which is performance sensitive, would become the
first user of the new API.

> I talked with Dave a little about this earlier this morning.
> In addition to refactoring to the new proxy model, there have been
> some discussions about changing how FileIO works, potentially sending
> file desciptors to the plugin process so it can read or write
> directly. It looks like the discussion is at
> http://code.google.com/p/chromium/issues/detail?id=194304
> 
> I think it makes sense to get this resolved before implementing
> FileRef::Query. I'm going to do some reading up today to make sure I
> understand what direction we're headed.

It looks like improving pp::FileRef::Query performance more is not a trivial
task... Then, if submitting this CL as-is wouldn't be difficult, and if the IPC
refactoring discussed at crbug.com/194304 would take time, say >2 weeks, what
about taking the following steps? This would be the best for our project.

1. Submit this change first to introduce pp::FileRef::Query()
2. Then refactor the IPC code to make FileIo::Open, FileRef::Delete,
FileRef::Rename, FileRef::MakeDirectory, and FileRef::Query faster.

We're working on a library which is similar to nacl_mount, and need an API to
implement POSIX's access() and stat() in an efficient manner. Currently we have
to call FileIO::Open and FileIO::Query to implement them and calling the two
APIs takes ~8ms (6-7ms for FileIO::Open, 1-2ms for FileIO::Query). 8ms/call is
not fast enough for us and the performance issue is actually blocking us right
now. With your current pp::FileRef::Query implementation, the performance
doubles (less than 4ms/call) and it would unblock us.

Thanks!

> 
> >
> > On Tue, Mar 19, 2013 at 7:41 AM, Justin TerAvest
<mailto:teravest@google.com> wrote:
> >>
> >> On Mon, Mar 18, 2013 at 6:01 PM,  <mailto:yusukes@chromium.org> wrote:
> >> > Thanks for the work! drive-by non-codereview comment:
> >> >
> >> > I tested the new API with my PPAPI plugin, and found that
> >> > pp::FileRef::Query()
> >> > was more than 2 times slower than pp::FileIO::Query(). On my Z620,
> >> > FileRef::Query is about 3.8ms/call and FileIO::Query is about 1.5ms/call.
> >> >
> >> > chrome://tracing shows that pp::FileRef::Query() is implemented in
> >> > Chrome_FileThread (in the browser process) while pp::FileIO::Query() is
in
> >> > Renderer::FILE. I know almost nothing about PPAPI internals, but would it
> be
> >> > possible to do it in a renderer (or to send the IPC directly from the
> plugin
> >> > to
> >> > the browser) so that we can eliminate the additional renderer-browser IPC
> >> > cost?
> >>
> >> I'll take a look at sending the request directly to the browser. It
> >> seems like this should be possible, but I don't know how ugly the code
> >> will look yet.
> >>
> >> >
> >> >
> >> >
> >> > On 2013/03/18 23:16:16, dmichael wrote:
> >> >>
> >> >> https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h
> >> >> File ppapi/cpp/file_ref.h (right):
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newco...
> >> >>
> >> >> ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const
> >> >> CompletionCallback& callback);
> >> >> Please use "CompletionCallbackWithOutput<PP_FileInfo>" instead
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages.h
> >> >>
> >> >> File ppapi/proxy/ppapi_messages.h (right):
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages...
> >> >>
> >> >> ppapi/proxy/ppapi_messages.h:813: PP_FileInfo /* info */)
> >> >> It doesn't look like you're using this message
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:10: #include
"base/debug/stack_trace.h"
> >> >> ^^ leftover from debugging
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
> >> >> PP_FileInfo*> PendingFileInfoMap;
> >> >> It looks like you're using |int| instead of |unsigned int| for the
> >> >> callback id
> >> >> elsewhere.
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:183: PP_FileInfo *target_info =
> >> >
> >> > found->second;
> >> >>
> >> >> nit: '*' should be with PP_FileInfo
> >> >
> >> >
> >> >
> >> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >> >>
> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
> >> >> sizeof(tmp_info));
> >> >> Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
> >> >> shouldn't
> >> >> need to special case this here
> >> >
> >> >
> >> >
> >> >
> >> > https://codereview.chromium.org/12817009/

teravest

Yusuke, Your plan sounds reasonable to me. I think the matter of using file desciptors ...

7 years, 9 months ago (2013-03-20 17:35:46 UTC) #11

Yusuke,
  Your plan sounds reasonable to me.

I think the matter of using file desciptors in the plugin will take a
while to be resolved, so here's what I'll to do:
  1. Refactor FileRef to use the "new proxy" model. This will take a
little while, as the FileRef code is a little complicated, and has a
bunch of juggling through FileRef_Shared.
  2. Implement FileRef::Query() in terms of a plugin->browser message.
  3. Possibly move over other FileRef methods to make them faster, if
it's helpful.

I don't have an estimate for how long this will take (yet), because I
don't know how much work (1) will take.

I can submit this change as is, though I don't know if it's useful at
all with the current performance.

On Tue, Mar 19, 2013 at 3:44 PM,  <yusukes@chromium.org> wrote:
> On 2013/03/19 16:35:18, teravest_google.com wrote:
>>
>> On Tue, Mar 19, 2013 at 10:30 AM, Raymes Khoury <mailto:raymes@google.com>
>
> wrote:
>>
>> >> I'll take a look at sending the request directly to the browser. It
>> >> seems like this should be possible, but I don't know how ugly the code
>> >> will look yet.
>> >
>> > This will probably require some ugly hacks until we move file ref to
>> > the new proxy model. (just something to keep in mind - if it's a very
>> > urgent performance requirement please go ahead otherwise let's
>> > consider doing the refactor first).
>
>
> I think our (internal) project, which is performance sensitive, would become
> the
> first user of the new API.
>
>
>> I talked with Dave a little about this earlier this morning.
>> In addition to refactoring to the new proxy model, there have been
>> some discussions about changing how FileIO works, potentially sending
>> file desciptors to the plugin process so it can read or write
>> directly. It looks like the discussion is at
>> http://code.google.com/p/chromium/issues/detail?id=194304
>
>
>> I think it makes sense to get this resolved before implementing
>> FileRef::Query. I'm going to do some reading up today to make sure I
>> understand what direction we're headed.
>
>
> It looks like improving pp::FileRef::Query performance more is not a trivial
> task... Then, if submitting this CL as-is wouldn't be difficult, and if the
> IPC
> refactoring discussed at crbug.com/194304 would take time, say >2 weeks,
> what
> about taking the following steps? This would be the best for our project.
>
> 1. Submit this change first to introduce pp::FileRef::Query()
> 2. Then refactor the IPC code to make FileIo::Open, FileRef::Delete,
> FileRef::Rename, FileRef::MakeDirectory, and FileRef::Query faster.
>
> We're working on a library which is similar to nacl_mount, and need an API
> to
> implement POSIX's access() and stat() in an efficient manner. Currently we
> have
> to call FileIO::Open and FileIO::Query to implement them and calling the two
> APIs takes ~8ms (6-7ms for FileIO::Open, 1-2ms for FileIO::Query). 8ms/call
> is
> not fast enough for us and the performance issue is actually blocking us
> right
> now. With your current pp::FileRef::Query implementation, the performance
> doubles (less than 4ms/call) and it would unblock us.
>
> Thanks!
>
>
>> >
>>
>> > On Tue, Mar 19, 2013 at 7:41 AM, Justin TerAvest
>
> <mailto:teravest@google.com> wrote:
>
>> >>
>> >> On Mon, Mar 18, 2013 at 6:01 PM,  <mailto:yusukes@chromium.org> wrote:
>> >> > Thanks for the work! drive-by non-codereview comment:
>> >> >
>> >> > I tested the new API with my PPAPI plugin, and found that
>> >> > pp::FileRef::Query()
>> >> > was more than 2 times slower than pp::FileIO::Query(). On my Z620,
>> >> > FileRef::Query is about 3.8ms/call and FileIO::Query is about
>> >> > 1.5ms/call.
>> >> >
>> >> > chrome://tracing shows that pp::FileRef::Query() is implemented in
>> >> > Chrome_FileThread (in the browser process) while pp::FileIO::Query()
>> >> > is
>
> in
>>
>> >> > Renderer::FILE. I know almost nothing about PPAPI internals, but
>> >> > would it
>> be
>> >> > possible to do it in a renderer (or to send the IPC directly from the
>> plugin
>> >> > to
>> >> > the browser) so that we can eliminate the additional renderer-browser
>> >> > IPC
>> >> > cost?
>> >>
>> >> I'll take a look at sending the request directly to the browser. It
>> >> seems like this should be possible, but I don't know how ugly the code
>> >> will look yet.
>> >>
>> >> >
>> >> >
>> >> >
>> >> > On 2013/03/18 23:16:16, dmichael wrote:
>> >> >>
>> >> >>
>> >> >>
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h
>> >> >> File ppapi/cpp/file_ref.h (right):
>> >> >
>> >> >
>> >> >
>> >> >
>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newco...
>>
>> >> >>
>> >> >> ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const
>> >> >> CompletionCallback& callback);
>> >> >> Please use "CompletionCallbackWithOutput<PP_FileInfo>" instead
>> >> >
>> >> >
>> >> >
>> >> >
>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages.h
>>
>> >> >>
>> >> >> File ppapi/proxy/ppapi_messages.h (right):
>> >> >
>> >> >
>> >> >
>> >> >
>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages...
>>
>> >> >>
>> >> >> ppapi/proxy/ppapi_messages.h:813: PP_FileInfo /* info */)
>> >> >> It doesn't look like you're using this message
>> >> >
>> >> >
>> >> >
>> >> >
>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>
>> >> >>
>> >> >> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>> >> >
>> >> >
>> >> >
>> >> >
>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>
>> >> >>
>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:10: #include
>
> "base/debug/stack_trace.h"
>>
>> >> >> ^^ leftover from debugging
>> >> >
>> >> >
>> >> >
>> >> >
>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>
>> >> >>
>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
>> >> >> PP_FileInfo*> PendingFileInfoMap;
>> >> >> It looks like you're using |int| instead of |unsigned int| for the
>> >> >> callback id
>> >> >> elsewhere.
>> >> >
>> >> >
>> >> >
>> >> >
>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>
>> >> >>
>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:183: PP_FileInfo *target_info =
>> >> >
>> >> > found->second;
>> >> >>
>> >> >> nit: '*' should be with PP_FileInfo
>> >> >
>> >> >
>> >> >
>> >> >
>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>
>> >> >>
>> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
>> >> >> sizeof(tmp_info));
>> >> >> Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
>> >> >> shouldn't
>> >> >> need to special case this here
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > https://codereview.chromium.org/12817009/
>
>
>
> https://codereview.chromium.org/12817009/

Yusuke Sato

On 2013/03/20 17:35:46, teravest wrote: > Yusuke, > Your plan sounds reasonable to me. > ...

7 years, 9 months ago (2013-03-20 17:48:24 UTC) #12

On 2013/03/20 17:35:46, teravest wrote:
> Yusuke,
>   Your plan sounds reasonable to me.
> 
> I think the matter of using file desciptors in the plugin will take a
> while to be resolved, so here's what I'll to do:
>   1. Refactor FileRef to use the "new proxy" model. This will take a
> little while, as the FileRef code is a little complicated, and has a
> bunch of juggling through FileRef_Shared.
>   2. Implement FileRef::Query() in terms of a plugin->browser message.
>   3. Possibly move over other FileRef methods to make them faster, if
> it's helpful.
> 
> I don't have an estimate for how long this will take (yet), because I
> don't know how much work (1) will take.
> 
> I can submit this change as is, though I don't know if it's useful at
> all with the current performance.

The current performance (<4 ms/call on Z620) is still very useful at least for
us.

-Yusuke


> 
> On Tue, Mar 19, 2013 at 3:44 PM,  <mailto:yusukes@chromium.org> wrote:
> > On 2013/03/19 16:35:18, http://teravest_google.com wrote:
> >>
> >> On Tue, Mar 19, 2013 at 10:30 AM, Raymes Khoury <mailto:raymes@google.com>
> >
> > wrote:
> >>
> >> >> I'll take a look at sending the request directly to the browser. It
> >> >> seems like this should be possible, but I don't know how ugly the code
> >> >> will look yet.
> >> >
> >> > This will probably require some ugly hacks until we move file ref to
> >> > the new proxy model. (just something to keep in mind - if it's a very
> >> > urgent performance requirement please go ahead otherwise let's
> >> > consider doing the refactor first).
> >
> >
> > I think our (internal) project, which is performance sensitive, would become
> > the
> > first user of the new API.
> >
> >
> >> I talked with Dave a little about this earlier this morning.
> >> In addition to refactoring to the new proxy model, there have been
> >> some discussions about changing how FileIO works, potentially sending
> >> file desciptors to the plugin process so it can read or write
> >> directly. It looks like the discussion is at
> >> http://code.google.com/p/chromium/issues/detail?id=194304
> >
> >
> >> I think it makes sense to get this resolved before implementing
> >> FileRef::Query. I'm going to do some reading up today to make sure I
> >> understand what direction we're headed.
> >
> >
> > It looks like improving pp::FileRef::Query performance more is not a trivial
> > task... Then, if submitting this CL as-is wouldn't be difficult, and if the
> > IPC
> > refactoring discussed at crbug.com/194304 would take time, say >2 weeks,
> > what
> > about taking the following steps? This would be the best for our project.
> >
> > 1. Submit this change first to introduce pp::FileRef::Query()
> > 2. Then refactor the IPC code to make FileIo::Open, FileRef::Delete,
> > FileRef::Rename, FileRef::MakeDirectory, and FileRef::Query faster.
> >
> > We're working on a library which is similar to nacl_mount, and need an API
> > to
> > implement POSIX's access() and stat() in an efficient manner. Currently we
> > have
> > to call FileIO::Open and FileIO::Query to implement them and calling the two
> > APIs takes ~8ms (6-7ms for FileIO::Open, 1-2ms for FileIO::Query). 8ms/call
> > is
> > not fast enough for us and the performance issue is actually blocking us
> > right
> > now. With your current pp::FileRef::Query implementation, the performance
> > doubles (less than 4ms/call) and it would unblock us.
> >
> > Thanks!
> >
> >
> >> >
> >>
> >> > On Tue, Mar 19, 2013 at 7:41 AM, Justin TerAvest
> >
> > <mailto:teravest@google.com> wrote:
> >
> >> >>
> >> >> On Mon, Mar 18, 2013 at 6:01 PM,  <mailto:yusukes@chromium.org> wrote:
> >> >> > Thanks for the work! drive-by non-codereview comment:
> >> >> >
> >> >> > I tested the new API with my PPAPI plugin, and found that
> >> >> > pp::FileRef::Query()
> >> >> > was more than 2 times slower than pp::FileIO::Query(). On my Z620,
> >> >> > FileRef::Query is about 3.8ms/call and FileIO::Query is about
> >> >> > 1.5ms/call.
> >> >> >
> >> >> > chrome://tracing shows that pp::FileRef::Query() is implemented in
> >> >> > Chrome_FileThread (in the browser process) while pp::FileIO::Query()
> >> >> > is
> >
> > in
> >>
> >> >> > Renderer::FILE. I know almost nothing about PPAPI internals, but
> >> >> > would it
> >> be
> >> >> > possible to do it in a renderer (or to send the IPC directly from the
> >> plugin
> >> >> > to
> >> >> > the browser) so that we can eliminate the additional renderer-browser
> >> >> > IPC
> >> >> > cost?
> >> >>
> >> >> I'll take a look at sending the request directly to the browser. It
> >> >> seems like this should be possible, but I don't know how ugly the code
> >> >> will look yet.
> >> >>
> >> >> >
> >> >> >
> >> >> >
> >> >> > On 2013/03/18 23:16:16, dmichael wrote:
> >> >> >>
> >> >> >>
> >> >> >>
> https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h
> >> >> >> File ppapi/cpp/file_ref.h (right):
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >
> >
> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newco...
> >>
> >> >> >>
> >> >> >> ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const
> >> >> >> CompletionCallback& callback);
> >> >> >> Please use "CompletionCallbackWithOutput<PP_FileInfo>" instead
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >
> >
> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages.h
> >>
> >> >> >>
> >> >> >> File ppapi/proxy/ppapi_messages.h (right):
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >
> >
> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppapi_messages...
> >>
> >> >> >>
> >> >> >> ppapi/proxy/ppapi_messages.h:813: PP_FileInfo /* info */)
> >> >> >> It doesn't look like you're using this message
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >
> >
> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> >> >>
> >> >> >> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >
> >
> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> >> >>
> >> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:10: #include
> >
> > "base/debug/stack_trace.h"
> >>
> >> >> >> ^^ leftover from debugging
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >
> >
> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> >> >>
> >> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
> >> >> >> PP_FileInfo*> PendingFileInfoMap;
> >> >> >> It looks like you're using |int| instead of |unsigned int| for the
> >> >> >> callback id
> >> >> >> elsewhere.
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >
> >
> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> >> >>
> >> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:183: PP_FileInfo *target_info =
> >> >> >
> >> >> > found->second;
> >> >> >>
> >> >> >> nit: '*' should be with PP_FileInfo
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >
> >
> >
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> >>
> >> >> >>
> >> >> >> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
> >> >> >> sizeof(tmp_info));
> >> >> >> Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
> >> >> >> shouldn't
> >> >> >> need to special case this here
> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> > https://codereview.chromium.org/12817009/
> >
> >
> >
> > https://codereview.chromium.org/12817009/

teravest

https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h File ppapi/cpp/file_ref.h (right): https://codereview.chromium.org/12817009/diff/2001/ppapi/cpp/file_ref.h#newcode157 ppapi/cpp/file_ref.h:157: int32_t Query(PP_FileInfo* info, const CompletionCallback& callback); On 2013/03/18 23:16:16, ...

7 years, 9 months ago (2013-03-21 16:45:03 UTC) #13

yzshen1

https://codereview.chromium.org/12817009/diff/19001/ppapi/cpp/file_ref.h File ppapi/cpp/file_ref.h (right): https://codereview.chromium.org/12817009/diff/19001/ppapi/cpp/file_ref.h#newcode153 ppapi/cpp/file_ref.h:153: /// @param[out] info A pointer to a <code>PP_FileInfo</code> which ...

7 years, 9 months ago (2013-03-21 17:18:10 UTC) #14

teravest

https://codereview.chromium.org/12817009/diff/19001/ppapi/cpp/file_ref.h File ppapi/cpp/file_ref.h (right): https://codereview.chromium.org/12817009/diff/19001/ppapi/cpp/file_ref.h#newcode153 ppapi/cpp/file_ref.h:153: /// @param[out] info A pointer to a <code>PP_FileInfo</code> which ...

7 years, 9 months ago (2013-03-21 19:48:56 UTC) #15

yzshen1

https://codereview.chromium.org/12817009/diff/29001/ppapi/proxy/ppb_file_ref_proxy.cc File ppapi/proxy/ppb_file_ref_proxy.cc (right): https://codereview.chromium.org/12817009/diff/29001/ppapi/proxy/ppb_file_ref_proxy.cc#newcode332 ppapi/proxy/ppb_file_ref_proxy.cc:332: PP_FileInfo* info = new PP_FileInfo(); Unfortunately, this may be ...

7 years, 9 months ago (2013-03-21 21:06:31 UTC) #16

teravest

On Thu, Mar 21, 2013 at 3:06 PM, <yzshen@chromium.org> wrote: > > https://codereview.chromium.org/12817009/diff/29001/ppapi/proxy/ppb_file_ref_proxy.cc > File ...

7 years, 9 months ago (2013-03-21 21:23:16 UTC) #17

yzshen1

Thanks Justin! https://codereview.chromium.org/12817009/diff/43001/ppapi/proxy/ppb_file_ref_proxy.cc File ppapi/proxy/ppb_file_ref_proxy.cc (right): https://codereview.chromium.org/12817009/diff/43001/ppapi/proxy/ppb_file_ref_proxy.cc#newcode336 ppapi/proxy/ppb_file_ref_proxy.cc:336: base::Owned(info), callback_id); I thought you will need ...

7 years, 9 months ago (2013-03-21 22:02:04 UTC) #18

teravest

On Thu, Mar 21, 2013 at 4:02 PM, <yzshen@chromium.org> wrote: > Thanks Justin! > > ...

7 years, 9 months ago (2013-03-21 22:04:27 UTC) #19

dmichael (off chromium)

https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_proxy.cc File ppapi/proxy/ppb_file_ref_proxy.cc (right): https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_proxy.cc#newcode82 ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int, PP_FileInfo*> PendingFileInfoMap; On 2013/03/21 16:45:04, teravest ...

7 years, 9 months ago (2013-03-21 22:11:28 UTC) #20

https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
File ppapi/proxy/ppb_file_ref_proxy.cc (right):

https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
PP_FileInfo*> PendingFileInfoMap;
On 2013/03/21 16:45:04, teravest wrote:
> On 2013/03/18 23:16:16, dmichael wrote:
> > It looks like you're using |int| instead of |unsigned int| for the callback
id
> > elsewhere.
> 
> This is a little gross. PpapiMsg_PPBFileRef_CallbackComplete uses an int for
the
> callback_id, but we're obviously using unsigned int internally here.
> 
> Do you want me to clean this all up as part of this patch?
> 
Hmm, I guess you aren't making it worse, so you could do it separately if you
want. But it should probably be changed. I'm guessing you could make them it
uint32_t everywhere and be more consistent with what other messages do.

https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0, sizeof(tmp_info));
On 2013/03/21 16:45:04, teravest wrote:
> On 2013/03/18 23:16:16, dmichael wrote:
> > Shouldn't this just be done in PPB_FileRef_Impl? It seems like you shouldn't
> > need to special case this here
> 
> I'm special casing it in the proxy because we can't send uninitialized data
over
> IPC. I don't think we need to zero-out the output parameter in every case.
What if you just implement the host side so that it's guaranteed not to modify
the PP_FileInfo (it's probably that way anyway). I think that's consistent with
what we do elsewhere. In that case, you can initialize PP_FileInfo to 0 before
the call unconditionally, and get rid of the if.

WDYT?

https://codereview.chromium.org/12817009/diff/43001/webkit/plugins/ppapi/ppb_...
File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):

https://codereview.chromium.org/12817009/diff/43001/webkit/plugins/ppapi/ppb_...
webkit/plugins/ppapi/ppb_file_ref_impl.cc:299: return PP_ERROR_NOACCESS;
Don't we want to support Query for external file systems too? I think
FileIO::Query does.

teravest

On Thu, Mar 21, 2013 at 4:11 PM, <dmichael@chromium.org> wrote: > > https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_proxy.cc > File ...

7 years, 9 months ago (2013-03-21 22:41:26 UTC) #21

On Thu, Mar 21, 2013 at 4:11 PM,  <dmichael@chromium.org> wrote:
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
> PP_FileInfo*> PendingFileInfoMap;
> On 2013/03/21 16:45:04, teravest wrote:
>>
>> On 2013/03/18 23:16:16, dmichael wrote:
>> > It looks like you're using |int| instead of |unsigned int| for the
>
> callback id
>>
>> > elsewhere.
>
>
>> This is a little gross. PpapiMsg_PPBFileRef_CallbackComplete uses an
>
> int for the
>>
>> callback_id, but we're obviously using unsigned int internally here.
>
>
>> Do you want me to clean this all up as part of this patch?
>
>
> Hmm, I guess you aren't making it worse, so you could do it separately
> if you want. But it should probably be changed. I'm guessing you could
> make them it uint32_t everywhere and be more consistent with what other
> messages do.

Fixed throughout.

>
>
>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
> sizeof(tmp_info));
> On 2013/03/21 16:45:04, teravest wrote:
>>
>> On 2013/03/18 23:16:16, dmichael wrote:
>> > Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
>
> shouldn't
>>
>> > need to special case this here
>
>
>> I'm special casing it in the proxy because we can't send uninitialized
>
> data over
>>
>> IPC. I don't think we need to zero-out the output parameter in every
>
> case.
> What if you just implement the host side so that it's guaranteed not to
> modify the PP_FileInfo (it's probably that way anyway). I think that's
> consistent with what we do elsewhere. In that case, you can initialize
> PP_FileInfo to 0 before the call unconditionally, and get rid of the if.
>
> WDYT?

Sounds good. Done.

>
>
https://codereview.chromium.org/12817009/diff/43001/webkit/plugins/ppapi/ppb_...
> File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):
>
>
https://codereview.chromium.org/12817009/diff/43001/webkit/plugins/ppapi/ppb_...
> webkit/plugins/ppapi/ppb_file_ref_impl.cc:299: return PP_ERROR_NOACCESS;
> Don't we want to support Query for external file systems too? I think
> FileIO::Query does.

I have no idea. This was from the original FileRef::Query code. I
figure we can always relax this to work for external file systems
later if it makes sense.

>
> https://codereview.chromium.org/12817009/

yzshen

> I figure we can always relax this to work for external file systems later ...

7 years, 9 months ago (2013-03-21 23:33:58 UTC) #22

> I figure we can always relax this to work for external file systems later if
it makes sense.
The API behavior is also part of the contract between us and plugins.
If we change the behavior later, we will have to increase the version
number. Otherwise it causes confusion: the same function doesn't work
with external file systems on some Chrome builds, but work on others.

Since this is a stable interface, I think it is better to make sure
the behavior is what we want from the beginning.


On Thu, Mar 21, 2013 at 3:41 PM, Justin TerAvest <teravest@chromium.org> wrote:
> On Thu, Mar 21, 2013 at 4:11 PM,  <dmichael@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>>
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
>> PP_FileInfo*> PendingFileInfoMap;
>> On 2013/03/21 16:45:04, teravest wrote:
>>>
>>> On 2013/03/18 23:16:16, dmichael wrote:
>>> > It looks like you're using |int| instead of |unsigned int| for the
>>
>> callback id
>>>
>>> > elsewhere.
>>
>>
>>> This is a little gross. PpapiMsg_PPBFileRef_CallbackComplete uses an
>>
>> int for the
>>>
>>> callback_id, but we're obviously using unsigned int internally here.
>>
>>
>>> Do you want me to clean this all up as part of this patch?
>>
>>
>> Hmm, I guess you aren't making it worse, so you could do it separately
>> if you want. But it should probably be changed. I'm guessing you could
>> make them it uint32_t everywhere and be more consistent with what other
>> messages do.
>
> Fixed throughout.
>
>>
>>
>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
>> sizeof(tmp_info));
>> On 2013/03/21 16:45:04, teravest wrote:
>>>
>>> On 2013/03/18 23:16:16, dmichael wrote:
>>> > Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
>>
>> shouldn't
>>>
>>> > need to special case this here
>>
>>
>>> I'm special casing it in the proxy because we can't send uninitialized
>>
>> data over
>>>
>>> IPC. I don't think we need to zero-out the output parameter in every
>>
>> case.
>> What if you just implement the host side so that it's guaranteed not to
>> modify the PP_FileInfo (it's probably that way anyway). I think that's
>> consistent with what we do elsewhere. In that case, you can initialize
>> PP_FileInfo to 0 before the call unconditionally, and get rid of the if.
>>
>> WDYT?
>
> Sounds good. Done.
>
>>
>>
https://codereview.chromium.org/12817009/diff/43001/webkit/plugins/ppapi/ppb_...
>> File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):
>>
>>
https://codereview.chromium.org/12817009/diff/43001/webkit/plugins/ppapi/ppb_...
>> webkit/plugins/ppapi/ppb_file_ref_impl.cc:299: return PP_ERROR_NOACCESS;
>> Don't we want to support Query for external file systems too? I think
>> FileIO::Query does.
>
> I have no idea. This was from the original FileRef::Query code. I
> figure we can always relax this to work for external file systems
> later if it makes sense.
>
>>
>> https://codereview.chromium.org/12817009/



--
Best regards,
Yuzhu Shen.

Yusuke Sato

https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc File ppapi/cpp/file_ref.cc (right): https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc#newcode110 ppapi/cpp/file_ref.cc:110: if (!has_interface<PPB_FileRef_1_0>()) I guess this (and all other v1.0 ...

7 years, 9 months ago (2013-03-24 08:00:05 UTC) #23

teravest1

On Thu, Mar 21, 2013 at 5:33 PM, Yuzhu Shen <yzshen@google.com> wrote: >> I figure ...

7 years, 9 months ago (2013-03-26 19:23:02 UTC) #24

On Thu, Mar 21, 2013 at 5:33 PM, Yuzhu Shen <yzshen@google.com> wrote:
>> I figure we can always relax this to work for external file systems later if
it makes sense.
> The API behavior is also part of the contract between us and plugins.
> If we change the behavior later, we will have to increase the version
> number. Otherwise it causes confusion: the same function doesn't work
> with external file systems on some Chrome builds, but work on others.
>
> Since this is a stable interface, I think it is better to make sure
> the behavior is what we want from the beginning.

That makes sense. I added support this morning for Query() for
external filesystems (and a supporting test case).

>
>
> On Thu, Mar 21, 2013 at 3:41 PM, Justin TerAvest <teravest@chromium.org>
wrote:
>> On Thu, Mar 21, 2013 at 4:11 PM,  <dmichael@chromium.org> wrote:
>>>
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>> File ppapi/proxy/ppb_file_ref_proxy.cc (right):
>>>
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>> ppapi/proxy/ppb_file_ref_proxy.cc:82: typedef std::map<unsigned int,
>>> PP_FileInfo*> PendingFileInfoMap;
>>> On 2013/03/21 16:45:04, teravest wrote:
>>>>
>>>> On 2013/03/18 23:16:16, dmichael wrote:
>>>> > It looks like you're using |int| instead of |unsigned int| for the
>>>
>>> callback id
>>>>
>>>> > elsewhere.
>>>
>>>
>>>> This is a little gross. PpapiMsg_PPBFileRef_CallbackComplete uses an
>>>
>>> int for the
>>>>
>>>> callback_id, but we're obviously using unsigned int internally here.
>>>
>>>
>>>> Do you want me to clean this all up as part of this patch?
>>>
>>>
>>> Hmm, I guess you aren't making it worse, so you could do it separately
>>> if you want. But it should probably be changed. I'm guessing you could
>>> make them it uint32_t everywhere and be more consistent with what other
>>> messages do.
>>
>> Fixed throughout.
>>
>>>
>>>
>>>
https://codereview.chromium.org/12817009/diff/2001/ppapi/proxy/ppb_file_ref_p...
>>> ppapi/proxy/ppb_file_ref_proxy.cc:391: memset(&tmp_info, 0,
>>> sizeof(tmp_info));
>>> On 2013/03/21 16:45:04, teravest wrote:
>>>>
>>>> On 2013/03/18 23:16:16, dmichael wrote:
>>>> > Shouldn't this just be done in PPB_FileRef_Impl? It seems like you
>>>
>>> shouldn't
>>>>
>>>> > need to special case this here
>>>
>>>
>>>> I'm special casing it in the proxy because we can't send uninitialized
>>>
>>> data over
>>>>
>>>> IPC. I don't think we need to zero-out the output parameter in every
>>>
>>> case.
>>> What if you just implement the host side so that it's guaranteed not to
>>> modify the PP_FileInfo (it's probably that way anyway). I think that's
>>> consistent with what we do elsewhere. In that case, you can initialize
>>> PP_FileInfo to 0 before the call unconditionally, and get rid of the if.
>>>
>>> WDYT?
>>
>> Sounds good. Done.
>>
>>>
>>>
https://codereview.chromium.org/12817009/diff/43001/webkit/plugins/ppapi/ppb_...
>>> File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):
>>>
>>>
https://codereview.chromium.org/12817009/diff/43001/webkit/plugins/ppapi/ppb_...
>>> webkit/plugins/ppapi/ppb_file_ref_impl.cc:299: return PP_ERROR_NOACCESS;
>>> Don't we want to support Query for external file systems too? I think
>>> FileIO::Query does.
>>
>> I have no idea. This was from the original FileRef::Query code. I
>> figure we can always relax this to work for external file systems
>> later if it makes sense.
>>
>>>
>>> https://codereview.chromium.org/12817009/
>
>
>
> --
> Best regards,
> Yuzhu Shen.

teravest

On Sun, Mar 24, 2013 at 2:00 AM, <yusukes@chromium.org> wrote: > > https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc > File ...

7 years, 9 months ago (2013-03-26 20:12:20 UTC) #25

Yusuke Sato

On 2013/03/26 20:12:20, teravest wrote: > On Sun, Mar 24, 2013 at 2:00 AM, <mailto:yusukes@chromium.org> ...

7 years, 9 months ago (2013-03-26 20:42:59 UTC) #26

On 2013/03/26 20:12:20, teravest wrote:
> On Sun, Mar 24, 2013 at 2:00 AM,  <mailto:yusukes@chromium.org> wrote:
> >
> > https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc
> > File ppapi/cpp/file_ref.cc (right):
> >
> >
>
https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc#new...
> > ppapi/cpp/file_ref.cc:110: if (!has_interface<PPB_FileRef_1_0>())
> > I guess this (and all other v1.0 methods) should be like:
> >
> > if (has_interface<1_1>())
> >   get_interface<1_1>()->Rename(...);
> > else if (has_interface<1_0>())
> >   get_interface<1_0>()->Rename(...);
> > else
> >   return cc.MayForce(PP_ERROR_NOINTERFACE);
> >
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/cpp/file_io.cc?view=markup
> > does that.
> 
> Hmm, you're right, and I see this pattern in other files too.
> Presumably the reason is so that the SDK will work properly if we
> deprecate and remove the 1.0 version of the interface? That seems like
> a bad thing to do-- we should keep around the old versions of the
> interface so that plugins built against older versions of the pepper
> SDK will continue to work.
> 
> I'd advocate for removing this kind of logic throughout ppapi/cpp. Why
> are we doing this?

I guess that the current way (i.e. using the latest interface whenever possible)
is more unittest friendly. Actually I noticed the issue when I was writing
gmock-based unit tests for our PPAPI plugin which calls into your new
pp::FileRef interface.

Our current testing code is usually like this:

PPB_FileRef_1_1_Mock* ppb_file_ref_;
..
EXPECT_CALL(*ppb_file_ref_, Create(xxx, _)).WillOnce(Return(..)));
EXPECT_CALL(*ppb_file_ref_, Query(xxx,...));

But with your current code (patch set #7), our testing code has to be changed to

PPB_FileRef_1_0_Mock* ppb_file_ref_1_0_;
PPB_FileRef_1_1_Mock* ppb_file_ref_1_1_;
..
EXPECT_CALL(*ppb_file_ref_1_0_, Create(xxx, _)).WillOnce(Return(..)));
EXPECT_CALL(*ppb_file_ref_1_1_, Query(xxx,...));

which I feel is a bit difficult to maintain.

teravest

On Tue, Mar 26, 2013 at 2:42 PM, <yusukes@chromium.org> wrote: > On 2013/03/26 20:12:20, teravest ...

7 years, 9 months ago (2013-03-26 21:22:28 UTC) #27

On Tue, Mar 26, 2013 at 2:42 PM,  <yusukes@chromium.org> wrote:
> On 2013/03/26 20:12:20, teravest wrote:
>
>> On Sun, Mar 24, 2013 at 2:00 AM,  <mailto:yusukes@chromium.org> wrote:
>> >
>> >
>> > https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc
>> > File ppapi/cpp/file_ref.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc#new...
>>
>> > ppapi/cpp/file_ref.cc:110: if (!has_interface<PPB_FileRef_1_0>())
>> > I guess this (and all other v1.0 methods) should be like:
>> >
>> > if (has_interface<1_1>())
>> >   get_interface<1_1>()->Rename(...);
>> > else if (has_interface<1_0>())
>> >   get_interface<1_0>()->Rename(...);
>> > else
>> >   return cc.MayForce(PP_ERROR_NOINTERFACE);
>> >
>> >
>
>
>
http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/cpp/file_io.cc?view=markup
>>
>> > does that.
>
>
>> Hmm, you're right, and I see this pattern in other files too.
>> Presumably the reason is so that the SDK will work properly if we
>> deprecate and remove the 1.0 version of the interface? That seems like
>> a bad thing to do-- we should keep around the old versions of the
>> interface so that plugins built against older versions of the pepper
>> SDK will continue to work.
>
>
>> I'd advocate for removing this kind of logic throughout ppapi/cpp. Why
>> are we doing this?
>
>
> I guess that the current way (i.e. using the latest interface whenever
> possible)
> is more unittest friendly. Actually I noticed the issue when I was writing
> gmock-based unit tests for our PPAPI plugin which calls into your new
> pp::FileRef interface.
>
> Our current testing code is usually like this:
>
> PPB_FileRef_1_1_Mock* ppb_file_ref_;
> ..
> EXPECT_CALL(*ppb_file_ref_, Create(xxx, _)).WillOnce(Return(..)));
> EXPECT_CALL(*ppb_file_ref_, Query(xxx,...));
>
> But with your current code (patch set #7), our testing code has to be
> changed to
>
> PPB_FileRef_1_0_Mock* ppb_file_ref_1_0_;
> PPB_FileRef_1_1_Mock* ppb_file_ref_1_1_;
> ..
> EXPECT_CALL(*ppb_file_ref_1_0_, Create(xxx, _)).WillOnce(Return(..)));
> EXPECT_CALL(*ppb_file_ref_1_1_, Query(xxx,...));
>
> which I feel is a bit difficult to maintain.

Hmm, that is gross.
I'll change my code to match the prevailing style, but I do wish there
was a better way to solve this.

Thanks for the testing example.

>
>
>
> https://codereview.chromium.org/12817009/

dmichael (off chromium)

On Tue, Mar 26, 2013 at 2:42 PM, <yusukes@chromium.org> wrote: > On 2013/03/26 20:12:20, teravest ...

7 years, 9 months ago (2013-03-26 21:24:27 UTC) #28

On Tue, Mar 26, 2013 at 2:42 PM, <yusukes@chromium.org> wrote:

> On 2013/03/26 20:12:20, teravest wrote:
>
>  On Sun, Mar 24, 2013 at 2:00 AM,  <mailto:yusukes@chromium.org> wrote:
>> >
>> > https://codereview.chromium.**org/12817009/diff/45019/ppapi/**
>>
cpp/file_ref.cc<https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc>
>> > File ppapi/cpp/file_ref.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/12817009/diff/45019/ppapi/**
>
cpp/file_ref.cc#newcode110<https://codereview.chromium.org/12817009/diff/45019/ppapi/cpp/file_ref.cc#newcode110>
>
>> > ppapi/cpp/file_ref.cc:110: if (!has_interface<PPB_FileRef_1_**0>())
>> > I guess this (and all other v1.0 methods) should be like:
>> >
>> > if (has_interface<1_1>())
>> >   get_interface<1_1>()->Rename(.**..);
>> > else if (has_interface<1_0>())
>> >   get_interface<1_0>()->Rename(.**..);
>> > else
>> >   return cc.MayForce(PP_ERROR_**NOINTERFACE);
>> >
>> >
>>
>
> http://src.chromium.org/**viewvc/chrome/trunk/src/ppapi/**
>
cpp/file_io.cc?view=markup<http://src.chromium.org/viewvc/chrome/trunk/src/ppapi/cpp/file_io.cc?view=markup>
>
>> > does that.
>>
>
>  Hmm, you're right, and I see this pattern in other files too.
>> Presumably the reason is so that the SDK will work properly if we
>> deprecate and remove the 1.0 version of the interface? That seems like
>> a bad thing to do-- we should keep around the old versions of the
>> interface so that plugins built against older versions of the pepper
>> SDK will continue to work.
>>
>
>  I'd advocate for removing this kind of logic throughout ppapi/cpp. Why
>> are we doing this?
>>
>
> I guess that the current way (i.e. using the latest interface whenever
> possible)
> is more unittest friendly. Actually I noticed the issue when I was writing
> gmock-based unit tests for our PPAPI plugin which calls into your new
> pp::FileRef interface.
>
> Our current testing code is usually like this:
>
> PPB_FileRef_1_1_Mock* ppb_file_ref_;
> ..
> EXPECT_CALL(*ppb_file_ref_, Create(xxx, _)).WillOnce(Return(..)));
> EXPECT_CALL(*ppb_file_ref_, Query(xxx,...));
>
> But with your current code (patch set #7), our testing code has to be
> changed to
>
> PPB_FileRef_1_0_Mock* ppb_file_ref_1_0_;
> PPB_FileRef_1_1_Mock* ppb_file_ref_1_1_;
> ..
> EXPECT_CALL(*ppb_file_ref_1_0_**, Create(xxx, _)).WillOnce(Return(..)));
> EXPECT_CALL(*ppb_file_ref_1_1_**, Query(xxx,...));
>
> which I feel is a bit difficult to maintain.
>
In a lot of cases, we're just appending to the struct. So an object
w/version 1.1 might be a perfectly valid response if asked for 1.0. E.g.,
that's true here. You would only have:
PPB_FileRef_1_1_Mock* ppb_file_ref_1_1_;

And make GetInterface return it whether you ask for 1.0 or 1.1. That would
work here, but not for everything. Also, do we want to prefer making our
test code simpler vs our production C++ code? Might make sense to
prioritize making the C++ simple.

I've always preferred just asking for the older version when we know the
versions are identical (like var does
it<https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/var.cc&q=var.cc&sq=package:chromium&type=cs&l=109>).
But this has come up before...  and I can't seem to find all of the debate.
I seem to remember viettrungluu was opposed to the shorter style.


>
>
>
https://codereview.chromium.**org/12817009/<https://codereview.chromium.org/1...
>

yzshen1

Some more comments. Thanks! https://codereview.chromium.org/12817009/diff/64002/ppapi/cpp/file_ref.cc File ppapi/cpp/file_ref.cc (right): https://codereview.chromium.org/12817009/diff/64002/ppapi/cpp/file_ref.cc#newcode51 ppapi/cpp/file_ref.cc:51: else if (has_interface<PPB_FileRef_1_0>()) I remember ...

7 years, 9 months ago (2013-03-26 23:32:42 UTC) #29

teravest

On Tue, Mar 26, 2013 at 5:32 PM, <yzshen@chromium.org> wrote: > Some more comments. > ...

7 years, 9 months ago (2013-03-27 16:35:20 UTC) #30

yzshen1

Thanks Justin! Only two more nits. > Done. I left the braces because the return ...

7 years, 9 months ago (2013-03-27 19:41:22 UTC) #31

teravest

On Wed, Mar 27, 2013 at 1:41 PM, <yzshen@chromium.org> wrote: > Thanks Justin! Only two ...

7 years, 9 months ago (2013-03-27 19:53:57 UTC) #32

On Wed, Mar 27, 2013 at 1:41 PM,  <yzshen@chromium.org> wrote:
> Thanks Justin! Only two more nits.
>
>
>
>> Done. I left the braces because the return statement is too big to fit
>> on one line. Let me know if I should get rid of the braces, too.
>
> That sounds good to me.
>
>
>> I thought this was odd too. file_system_ is always initialized to NULL
>> when it's external.
>> I didn't see a need to touch it in this change, but I can clean that
>> up if you want.
>
> No need to change it in this CL. I am just curious and want to make sure I
> didn't misunderstand things.
> Thanks!
>
>
>> Fixed. I no longer use weakptr factory, and now explicitly close the
>> PlatformFile. Should I block until the close completes before calling
>> the user callback? Right now, I ignore the return code from
>> ClosePlatformFile().
>
> I think that is fine.
>
>
>
https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_...
> File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right):
>
>
https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_...
> webkit/plugins/ppapi/ppb_file_ref_impl.cc:141:
> base::Bind(&GetFileInfoCallback, plugin_instance, file, info,
> callback)))
> nit: I think the reply won't be run when it returns false.

Can you clarify what you mean? There's a
"callback->Run(PP_ERROR_FAILED);" statement for when
GetFileInfoFromPlatformFile() returns false.

Looking at this did make me realize that I leak the file handle in
this case, though, so I'll add code to close the PlatformFile when
that happens.

>
>
https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_...
> webkit/plugins/ppapi/ppb_file_ref_impl.cc:364:
> base::Bind(&QueryCallback, plugin_instance, info, callback)))
> PPB_FileRef_Impl itself is ref-counted, maybe holding an extra ref to it
> is better than holding one to |plugin_instance|?

What's the advantage of holding a ref on PPB_FileRef_Impl instead of
the PluginInstance? I'd like to understand for the future.

>
> https://codereview.chromium.org/12817009/

dmichael (off chromium)

https://codereview.chromium.org/12817009/diff/79001/ppapi/api/ppb_file_ref.idl File ppapi/api/ppb_file_ref.idl (right): https://codereview.chromium.org/12817009/diff/79001/ppapi/api/ppb_file_ref.idl#newcode13 ppapi/api/ppb_file_ref.idl:13: M27 = 1.1 (nit: probably M28 now) https://codereview.chromium.org/12817009/diff/79001/ppapi/cpp/file_ref.cc File ...

7 years, 9 months ago (2013-03-27 20:20:06 UTC) #33

teravest

On Wed, Mar 27, 2013 at 2:20 PM, <dmichael@chromium.org> wrote: > > https://codereview.chromium.org/12817009/diff/79001/ppapi/api/ppb_file_ref.idl > File ...

7 years, 9 months ago (2013-03-27 20:38:10 UTC) #34

teravest

On Wed, Mar 27, 2013 at 2:37 PM, Justin TerAvest <teravest@chromium.org> wrote: > On Wed, ...

7 years, 9 months ago (2013-03-27 21:08:13 UTC) #35

dmichael (off chromium)

> There shouldn't be a WeakPtrFactory at all in there anymore. > What's the advantage ...

7 years, 9 months ago (2013-03-27 21:08:54 UTC) #36

teravest1

On Wed, Mar 27, 2013 at 3:08 PM, <dmichael@chromium.org> wrote: >> There shouldn't be a ...

7 years, 9 months ago (2013-03-27 21:20:39 UTC) #37

yzshen1

https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_... > webkit/plugins/ppapi/ppb_file_ref_impl.cc:141: > base::Bind(&GetFileInfoCallback, plugin_instance, file, info, > callback))) > nit: I think the ...

7 years, 9 months ago (2013-03-27 21:28:41 UTC) #38

https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_...
> webkit/plugins/ppapi/ppb_file_ref_impl.cc:141:
> base::Bind(&GetFileInfoCallback, plugin_instance, file, info,
> callback)))
> nit: I think the reply won't be run when it returns false.

> Can you clarify what you mean? There's a
> "callback->Run(PP_ERROR_FAILED);" statement for when
> GetFileInfoFromPlatformFile() returns false.

> Looking at this did make me realize that I leak the file handle in
> this case, though, so I'll add code to close the PlatformFile when
> that happens.
That is what I meant. It leaks when GetFileInfoFromPlatformFile fails.


https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_...
> webkit/plugins/ppapi/ppb_file_ref_impl.cc:364:
> base::Bind(&QueryCallback, plugin_instance, info, callback)))
> PPB_FileRef_Impl itself is ref-counted, maybe holding an extra ref to it
> is better than holding one to |plugin_instance|?

> What's the advantage of holding a ref on PPB_FileRef_Impl instead of
> the PluginInstance? I'd like to understand for the future.
(David has answered it thoroughly. Thanks!)

https://codereview.chromium.org/12817009/diff/79001/ppapi/cpp/file_ref.cc
File ppapi/cpp/file_ref.cc (right):

https://codereview.chromium.org/12817009/diff/79001/ppapi/cpp/file_ref.cc#new...
ppapi/cpp/file_ref.cc:38: } else if (has_interface<PPB_FileRef_1_0>()) {
It was in the thread "Provide a way to disable Nagle's algorithm on Pepper TCP
sockets."
I think all of us have expressed the reasons behind our preferences there. But a
clear conclusion wasn't reached.
Hmm... Honestly, I am kind of sitting on the fence right now. :)

Is it a good idea to send a mail to the team, fully describe the reasons from
both sides and then make a final decision which we will follow from now on? It
affects quite a lot of files, I think it should qualify for a thread.


On 2013/03/27 20:20:06, dmichael wrote:
> Wow, this is ugly. yzshen: I think we've talked about this before...  but I
> can't find the old conversation. How do you feel about just using the oldest
> version when we know the backing implementation is identical? I can't really
see
> the harm in it. That's what we do for Var.
> 
> Disadvantages here are code complexity and increasing the code that we force
the
> plugin or NaCl app to link in.
> 
> Advantages...  the pattern works even when the backing implementation differs
> (in practice that hasn't happened yet), so we don't have to think about it.
> 
> I feel like optimizing for the common case, and only doing the fallback code
> when it's necessary. That also makes it clearer  to the reader that it _is_
> necessary.

teravest

On Wed, Mar 27, 2013 at 3:28 PM, <yzshen@chromium.org> wrote: > > https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_... > >> ...

7 years, 9 months ago (2013-03-27 21:32:25 UTC) #39

On Wed, Mar 27, 2013 at 3:28 PM,  <yzshen@chromium.org> wrote:
>
>
https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_...
>
>> webkit/plugins/ppapi/ppb_file_ref_impl.cc:141:
>> base::Bind(&GetFileInfoCallback, plugin_instance, file, info,
>> callback)))
>> nit: I think the reply won't be run when it returns false.
>
>
>> Can you clarify what you mean? There's a
>> "callback->Run(PP_ERROR_FAILED);" statement for when
>> GetFileInfoFromPlatformFile() returns false.
>
>
>> Looking at this did make me realize that I leak the file handle in
>> this case, though, so I'll add code to close the PlatformFile when
>> that happens.
>
> That is what I meant. It leaks when GetFileInfoFromPlatformFile fails.

Cool! Good catch. That should be fixed now.

>
>
>
https://codereview.chromium.org/12817009/diff/79001/webkit/plugins/ppapi/ppb_...
>>
>> webkit/plugins/ppapi/ppb_file_ref_impl.cc:364:
>> base::Bind(&QueryCallback, plugin_instance, info, callback)))
>> PPB_FileRef_Impl itself is ref-counted, maybe holding an extra ref to it
>> is better than holding one to |plugin_instance|?
>
>
>> What's the advantage of holding a ref on PPB_FileRef_Impl instead of
>> the PluginInstance? I'd like to understand for the future.
>
> (David has answered it thoroughly. Thanks!)
>
>
>
>
>
> https://codereview.chromium.org/12817009/diff/79001/ppapi/cpp/file_ref.cc
> File ppapi/cpp/file_ref.cc (right):
>
>
https://codereview.chromium.org/12817009/diff/79001/ppapi/cpp/file_ref.cc#new...
> ppapi/cpp/file_ref.cc:38: } else if (has_interface<PPB_FileRef_1_0>()) {
> It was in the thread "Provide a way to disable Nagle's algorithm on
> Pepper TCP sockets."
> I think all of us have expressed the reasons behind our preferences
> there. But a clear conclusion wasn't reached.
> Hmm... Honestly, I am kind of sitting on the fence right now. :)
>
> Is it a good idea to send a mail to the team, fully describe the reasons
> from both sides and then make a final decision which we will follow from
> now on? It affects quite a lot of files, I think it should qualify for a
> thread.

Yeah, let's move this to its own discussion.

>
>
>
> On 2013/03/27 20:20:06, dmichael wrote:
>>
>> Wow, this is ugly. yzshen: I think we've talked about this before...
>
> but I
>>
>> can't find the old conversation. How do you feel about just using the
>
> oldest
>>
>> version when we know the backing implementation is identical? I can't
>
> really see
>>
>> the harm in it. That's what we do for Var.
>
>
>> Disadvantages here are code complexity and increasing the code that we
>
> force the
>>
>> plugin or NaCl app to link in.
>
>
>> Advantages...  the pattern works even when the backing implementation
>
> differs
>>
>> (in practice that hasn't happened yet), so we don't have to think
>
> about it.
>
>> I feel like optimizing for the common case, and only doing the
>
> fallback code
>>
>> when it's necessary. That also makes it clearer  to the reader that it
>
> _is_
>>
>> necessary.
>
>
> https://codereview.chromium.org/12817009/

dmichael (off chromium)

https://codereview.chromium.org/12817009/diff/27004/ppapi/tests/test_file_ref.cc File ppapi/tests/test_file_ref.cc (right): https://codereview.chromium.org/12817009/diff/27004/ppapi/tests/test_file_ref.cc#newcode83 ppapi/tests/test_file_ref.cc:83: // RUN_TEST_FORCEASYNC_AND_NOT(Query, filter); nit: Please remove ^^^ https://codereview.chromium.org/12817009/diff/27004/ppapi/tests/test_utils.h File ...

7 years, 9 months ago (2013-03-27 22:13:55 UTC) #40

teravest1

On Wed, Mar 27, 2013 at 4:13 PM, <dmichael@chromium.org> wrote: > > https://codereview.chromium.org/12817009/diff/27004/ppapi/tests/test_file_ref.cc > File ...

7 years, 9 months ago (2013-03-27 22:26:03 UTC) #41

dmichael (off chromium)

lgtm https://codereview.chromium.org/12817009/diff/93001/webkit/plugins/ppapi/ppb_file_ref_impl.cc File webkit/plugins/ppapi/ppb_file_ref_impl.cc (right): https://codereview.chromium.org/12817009/diff/93001/webkit/plugins/ppapi/ppb_file_ref_impl.cc#newcode95 webkit/plugins/ppapi/ppb_file_ref_impl.cc:95: scoped_refptr<base::MessageLoopProxy> message_loop_proxy, My bad, I think TaskRunner is ...

7 years, 9 months ago (2013-03-27 22:40:21 UTC) #42

teravest

On Wed, Mar 27, 2013 at 4:40 PM, <dmichael@chromium.org> wrote: > lgtm > > > ...

7 years, 9 months ago (2013-03-28 14:31:35 UTC) #44

teravest

+jschuh for ppapi/proxy/ppapi_messages.h jschuh, This change makes a few changes to ppapi_messages.h * Existing (PpapiHostMsg_PPBFileRef* ...

7 years, 9 months ago (2013-03-28 14:37:05 UTC) #45