Oh, and +1 to the patch. The names are much better, and the descriptions make the use model much clearer.
-K ----- Original Message ----- > "What we've got here is failure to communicate." > > > > There aren't necessarily distinct > > input/output > > buffer objects, rather the whole transport interface itself is > > really > > just > > single structure (a [transforming] byte queue) with push/peek/pop > > corresponding exactly to any standard queue interface. > > Aha! Well, that explains it - I've always though that the transport > was composed of two separate buffers - one for input, the other for > output. At least, that's my interpretation of the existing API. > > A "transforming byte queue" didn't immediately pop into my mind when > reading these new APIs. You may want to add a bit of documentation > to that patch explaining this meme before the APIs are described. > Would be quite useful to anyone attempting to implement a driver. > > > -K > > ----- Original Message ----- > > Looks like the attachement didn't make it. Here's the link to the > > patch on > > JIRA: > > https://issues.apache.org/jira/secure/attachment/12568408/transport.patch > > > > --Rafael > > > > On Thu, Feb 7, 2013 at 8:10 AM, Rafael Schloming <r...@alum.mit.edu> > > wrote: > > > > > Here's a patch to engine.h with updated docs/naming. I've > > > documented what > > > I believe would be future lifecycle semantics, however as I said > > > before I > > > think an initial patch would need to be more conservative. I > > > think > > > these > > > names would also work with input/output prefixes, although as the > > > interface > > > now pretty much exactly matches a circular buffer (i.e. a byte > > > queue), I > > > find the input/output prefixes a bit jarring. > > > > > > --Rafael > > > > > > > > > On Thu, Feb 7, 2013 at 5:53 AM, Rafael Schloming > > > <r...@alum.mit.edu> > > > wrote: > > > > > >> On Wed, Feb 6, 2013 at 5:12 PM, Rafael Schloming > > >> <r...@alum.mit.edu>wrote: > > >> > > >>> On Wed, Feb 6, 2013 at 2:13 PM, Ken Giusti <kgiu...@redhat.com> > > >>> wrote: > > >>> > > >>>> Rafi, I agree with the rational behind these changes. > > >>>> > > >>>> > /** Return a pointer to a transport's input buffer. This > > >>>> > pointer > > >>>> > may > > >>>> > * change when calls into the engine are made. > > >>>> > > >>>> I think we'll need to be a little more liberal with the > > >>>> lifecycle > > >>>> guarantee of these buffer pointers. Drivers based on > > >>>> "completion" models > > >>>> (rather than sockets) could be forced to do data copies rather > > >>>> than > > >>>> supplying the pointer directly to the completion mechanism. > > >>>> > > >>> > > >>> That sentence was actually supposed to be deleted. The > > >>> sentences > > >>> after > > >>> that describes the intended lifecycle policy for the input > > >>> buffer: > > >>> > > >>> Calls to ::pn_transport_push may change the value of this > > >>> pointer and > > >>> the amount of free space reported by ::pn_transport_capacity. > > >>> > > >>> > > >>>> > > >>>> Could we instead guarantee that pointers (and space) returned > > >>>> from the > > >>>> transport remain valid until 1) the transport is released or > > >>>> 2) > > >>>> the > > >>>> "push"/"pop" call is made against the transport? > > >>>> > > >>> > > >>> That is in fact what I intended for push. For pop this would > > >>> place a lot > > >>> more restrictions on the engine implementation. Say for example > > >>> peek is > > >>> called and then the top half is used to write message data. > > >>> Ideally there > > >>> should actually be more data to write over the network, which > > >>> means that > > >>> the transport may want to grow (realloc) the output buffer, and > > >>> this of > > >>> course is more complex if the external pointer needs to stay > > >>> valid. Given > > >>> that at worst this will incur an extra copy that is equivalent > > >>> to > > >>> what > > >>> we're currently doing, I figured it would be safer to start out > > >>> with more > > >>> conservative semantics. We can always relax them later when we > > >>> have had > > >>> more time to consider the implementation. > > >>> > > >>> > > >>>> Or perhaps a reference count-based interface would be safer? > > >>>> Once the > > >>>> driver determines there is capacity/pending, it "reserves" the > > >>>> buffer and > > >>>> is required to "push"/"pop" to release it? > > >>>> > > >>>> Oh, and those names absolutely stink. ;) It's unclear from > > >>>> the > > >>>> function names what components of the transport they are > > >>>> affecting. I'd > > >>>> rather something more readable: > > >>>> > > >>>> pn_transport_capacity() --> pn_transport_input_capacity() > > >>>> pn_transport_buffer() -> pn_transport_input_buffer() > > >>>> pn_transport_push() --> pn_transport_input_written() > > >>>> > > >>> > > >>> I think your names (and my documentation) actually suffer from > > >>> exposing > > >>> too much of the implementation. There aren't necessarily > > >>> distinct > > >>> input/output buffer objects, rather the whole transport > > >>> interface > > >>> itself is > > >>> really just single structure (a [transforming] byte queue) with > > >>> push/peek/pop corresponding exactly to any standard queue > > >>> interface. > > >>> > > >>> FWIW, I do agree pn_transport_buffer() could be better, however > > >>> capacity, pending, push, peek, and pop all correspond to pretty > > >>> much every > > >>> standard queue/circular buffer/stack interface I've ever seen, > > >>> and my hasty > > >>> documentation notwithstanding, I think that is a good meme to > > >>> take > > >>> advantage of, particularly since the push/peek/pop semantics > > >>> and > > >>> their > > >>> relationship to each other is actually quite important in > > >>> guaranteeing that > > >>> the pitfall occurring in PROTON-222 doesn't come up when people > > >>> code their > > >>> own drivers. > > >>> > > >> > > >> To elaborate on this a little, I think the key part of the meme > > >> that I > > >> like is how peek strongly signals "non-destructive-read" and pop > > >> strongly > > >> signals a destructive operation. I don't get that same > > >> destructive > > >> operation sense from output_written, and I think particularly > > >> when > > >> considering the lifecycle semantics, the > > >> non-destructive/destructive nature > > >> of the operations is kind of key. > > >> > > >> FWIW, I can buy the input/output prefix as it's possibly equally > > >> valid to > > >> view the interface as a queue head and a queue tail that aren't > > >> on > > >> the same > > >> queue, however I would personally omit them for two reasons (1) > > >> the notion > > >> of a transforming queue signals to the user that when you > > >> provide > > >> input, > > >> you should expect there to be output, and (2) I appreciate > > >> brevity. ;-) > > >> > > >> --Rafael > > >> > > >> > > > > > >
