Dealing with multiple interface versions in ppapi/cpp files.

[Justin TerAvest]

I'd like to simplify ppapi/cpp files by getting rid some some
boilerplate that I'm not convinced is necessary. Details are below.


In many interfaces, function implementations are identical across versions.

See ppapi/thunk/ppb_file_io_thunk.cc for an example. All of the functions
pointed to in the interface are the same, except 1.1 adds "ReadToArray".

However, ppapi/cpp/file_io.cc has code that looks like this:

int32_t FileIO::Query(PP_FileInfo* result_buf,
const CompletionCallback& cc) {
if (has_interface<PPB_FileIO_1_1>()) {
return get_interface<PPB_FileIO_1_1>()->Query(
pp_resource(), result_buf, cc.pp_completion_callback());
} else if (has_interface<PPB_FileIO_1_0>()) {
return get_interface<PPB_FileIO_1_0>()->Query(
pp_resource(), result_buf, cc.pp_completion_callback());
}
return cc.MayForce(PP_ERROR_NOINTERFACE);
}

This approach has some advantages:
* If we ever remove version 1.0 of the interface, things will still work.
This seems extremely unlikely for stable APIs, however.
* Using a mock of the PPB_FileIO_1_1 interface is very straightforward in
tests.
* We will consistently use the same version of the interface for every call
on a system. Yuzhu has voiced a concern that a Create() call at version 1.0
could be incompatible with (say) a function call at version 1.1. I don't·
know if we have concrete cases where this could happen.

And some disadvantages:
* There's a lot of boilerplate, and the result is that the same code ends up
being executed.
* It makes the code harder to read by making there appear to be multiple
implementations when that is not the case.

I don't think the advantages are worth the loss in readability, but
I'd like to hear what everyone thinks.

[David Michael]

My preference is for keeping the places where the backing implementation is the same as simple as possible (i.e., just use the older version). But I think this has come up before and I was voted down at the time.

Context:

https://groups.google.com/a/chromium.org/d/msg/chromium-reviews/OcHoA1pZhVo/C4_EtQg1wmYJ

https://chromiumcodereview.appspot.com/12220050/diff/12001/ppapi/cpp/private/tcp_socket_private.cc#oldcode51ppapi/cpp/private/tcp_socket_private.cc:51

(I can't seem to find the more detailed discussion about this, but I think we had one!)

On Thu, Mar 28, 2013 at 10:43 AM, Justin TerAvest <tera...@google.com> wrote:

I'd like to simplify ppapi/cpp files by getting rid some some
boilerplate that I'm not convinced is necessary. Details are below.


In many interfaces, function implementations are identical across versions.

See ppapi/thunk/ppb_file_io_thunk.cc for an example. All of the functions
pointed to in the interface are the same, except 1.1 adds "ReadToArray".

However, ppapi/cpp/file_io.cc has code that looks like this:

  int32_t FileIO::Query(PP_FileInfo* result_buf,
                        const CompletionCallback& cc) {
    if (has_interface<PPB_FileIO_1_1>()) {
      return get_interface<PPB_FileIO_1_1>()->Query(
          pp_resource(), result_buf, cc.pp_completion_callback());
    } else if (has_interface<PPB_FileIO_1_0>()) {
      return get_interface<PPB_FileIO_1_0>()->Query(
          pp_resource(), result_buf, cc.pp_completion_callback());
    }
    return cc.MayForce(PP_ERROR_NOINTERFACE);
  }

This approach has some advantages:
  * If we ever remove version 1.0 of the interface, things will still work.
    This seems extremely unlikely for stable APIs, however.

Right, older apps that use 1.0 will be broken if we do that, so we hope to never do it. OTOH...  we might break fewer apps if the C++ wrappers use the latest version they can if we ever have to deprecate something. This is the biggest plus for this strategy to me.

  * Using a mock of the PPB_FileIO_1_1 interface is very straightforward in
    tests.

(The issue being that you might need to mock both 1.0 and 1.1 if the C++ wrapper routinely uses both.) I mentioned to you elsewhere that I think this is easy to mitigate in most cases (if you're only appending to the struct, you can return the same vtable for >1 interface version string).

 

  * We will consistently use the same version of the interface for every call
    on a system. Yuzhu has voiced a concern that a Create() call at version 1.0
    could be incompatible with (say) a function call at version 1.1. I don't·
    know if we have concrete cases where this could happen.

My contention is still that we should try to avoid that as much as possible anyway. Right now, we always use the same backing object regardless of what version  of the API the plugin is using. That keeps our own implementation simpler and allows for things like plugins or NaCl apps using multiple libraries that might use different versions of APIs. As much as possible, it's good if we can make a Resource respond to any calls from any of its API versions. If we run into a case where we need multiple versions, our C++ code will reflect that, and it will be clearer to a reader that the version for that API must be accounted for if it stands out a little that we need to do fallback code.

 

And some disadvantages:
  * There's a lot of boilerplate, and the result is that the same code ends up
    being executed.
  * It makes the code harder to read by making there appear to be multiple
    implementations when that is not the case.

I don't think the advantages are worth the loss in readability, but
I'd like to hear what everyone thinks.

I still lean towards the shorter style, using the oldest version when the implementations are identical. I tried to think about a middle ground where we just make it easier to do the fallback code with macros or something, but I think it would end up being the worst of both worlds.

(If we had any actual data and strategy that would allow us to deprecate older versions safely, I might feel differently...  as it is, it seems like we're stuck maintaining old versions regardless).

[Yuzhu Shen]

Hi,

Thanks for starting this thread! We have discussed this issue here and there for several times but didn't reach a final decision. Hopefully we could make a guideline this time.

I personal prefer the approach of always trying to use the latest version and doing fallback in the C++ wrapper. (But I admit I am softening a little after rounds of discussions. :))

I think this is an interface-level decision, which shouldn't be affected too much by the current implementation. We shouldn't make the decision based on the fact that almost all our implementation can support "using the oldest version". What we should consider is that whether we want to make the promise that we will support/encourage the usage of mixing different versions of API calls with the same resource. For example, in general do we promise that a resource created by the v1.1 interface will work with the v1.0 interface? AFAIK, we never make such a promise. And it seems to me that the easiest way is not making such a promise in general, instead of making the decision case-by-case and depending on the internal implementation.

That being said, if we really want to simplify the C++ wrapper and only call the oldest available version, at least we should not do that for dev/private interface. We rely on the fallback logic of the C++ wrappers to deprecate old versions easily.

What do you think?

--
Best regards,
Yuzhu Shen.

[Viet-Trung Luu]

The idea that using the older version is simpler has come up a number of times. I guess it depends on what you mean by "simple".

I find having a bunch of identical boilerplate everywhere consistently mentally simpler for me than inconsistent boilerplate, even if it's a bit longer and more tedious. I agree that it'd be nice to have macros or whatever to make the boilerplate more concise.

YMMV.

- Trung

[David Michael]

On Thu, Mar 28, 2013 at 12:04 PM, Viet-Trung Luu <v...@google.com> wrote:

The idea that using the older version is simpler has come up a number of times. I guess it depends on what you mean by "simple".

I find having a bunch of identical boilerplate everywhere consistently mentally simpler for me than inconsistent boilerplate, even if it's a bit longer and more tedious.

I guess I would argue that it gives the mistaken impression that there's a difference between the versions of the function, when there really isn't. So it's mentally simpler in a way, but also misleading.

[Wez]

When we last discussed this I started out strongly in favour of simplifying, but a couple of good points were raised, most significantly around hampering interface deprecation.

FWIW, I think it'd be worth having macros, if possible, so we can maintain the current semantics but reduce boilerplate.

[Justin TerAvest]

I guess we should just keep things the way they are.

On Thu, Mar 28, 2013 at 3:09 PM, Wez <w...@chromium.org> wrote:
> When we last discussed this I started out strongly in favour of simplifying,
> but a couple of good points were raised, most significantly around hampering
> interface deprecation.
>
> FWIW, I think it'd be worth having macros, if possible, so we can maintain
> the current semantics but reduce boilerplate.

I really don't like the idea of hiding the boilerplate behind macros.
It still makes it just as hard for someone to read the code and
understand what's going on. :(

[David Michael]

On Thu, Mar 28, 2013 at 3:09 PM, Wez <w...@chromium.org> wrote:

When we last discussed this I started out strongly in favour of simplifying, but a couple of good points were raised, most significantly around hampering interface deprecation.

Yeah, I'm starting to feel the same way. Now that I realize doing it the shorter way makes it virtually impossible to deprecate an interface (e.g. PPB_FileIO_1_0), I'm more interested in the verbose fallback code. That said, it's not enough for me to feel comfortable deprecating any stable interface.

 

FWIW, I think it'd be worth having macros, if possible, so we can maintain the current semantics but reduce boilerplate.

I'd be open to seeing what it looks like. I started typing up a proposal in my original e-mail, but I'm not really sure if it's better. Makes it less mistake-prone, but not necessarily more readable.

[Wez]

On 28 March 2013 14:29, David Michael <dmic...@google.com> wrote:

On Thu, Mar 28, 2013 at 3:09 PM, Wez <w...@chromium.org> wrote:

When we last discussed this I started out strongly in favour of simplifying, but a couple of good points were raised, most significantly around hampering interface deprecation.

Yeah, I'm starting to feel the same way. Now that I realize doing it the shorter way makes it virtually impossible to deprecate an interface (e.g. PPB_FileIO_1_0), I'm more interested in the verbose fallback code. That said, it's not enough for me to feel comfortable deprecating any stable interface.

 

FWIW, I think it'd be worth having macros, if possible, so we can maintain the current semantics but reduce boilerplate.

I'd be open to seeing what it looks like. I started typing up a proposal in my original e-mail, but I'm not really sure if it's better. Makes it less mistake-prone, but not necessarily more readable.

Less mistake-prone would be a win.  Send me what you've got so far and I'll take a look, if you like.

[David Michael]

Here's an off-the-top-of-my-head, not-run-through-a-compiler, untested version:

Something like this in a private .h file somewhere:

#define CALL_API(interface, version, function, params) \

  if (has_interface<interface##_##version>()) { \

    return get_interface<interface##_##version>()->function params; \

  }

Optionally something like this in the body:

#define CALL_FILEIO(version, function, params) \

  CALL_API(PPB_FileIO, version, function, params)

So this:

int32_t FileIO::Query(PP_FileInfo* result_buf,
                      const CompletionCallback& cc) {
  if (has_interface<PPB_FileIO_1_1>()) {
    return get_interface<PPB_FileIO_1_1>()->Query(
        pp_resource(), result_buf, cc.pp_completion_callback());
  } else if (has_interface<PPB_FileIO_1_0>()) {
    return get_interface<PPB_FileIO_1_0>()->Query(
        pp_resource(), result_buf, cc.pp_completion_callback());
  }
  return cc.MayForce(PP_ERROR_NOINTERFACE);
}

becomes this:

int32_t FileIO::Query(PP_FileInfo* result_buf,

                      const CompletionCallback& cc) {

#define PARAMS (pp_resource(), result_buf, cc.pp_completion_callback()));

  CALL_FILEIO(1_1, Query, PARAMS);

  else CALL_FILEIO(1_0, Query, PARAMS);

#undef PARAMS

  return cc.MayForce(PP_ERROR_NOINTERFACE);

}