Given the memory overhead of a pn_data_t before encoding, why not have it own an encode buffer? it could get by with exactly that grow_buffer() callback if ownership is the issue .
Bozzo On Mar 31, 2015 6:10 PM, "Rafael Schloming" <[email protected]> wrote: > Hi Alan, > > Sorry I didn't comment on this sooner, I didn't have time to comment on > your original review request during my travels, however I do have some > thoughts on the changes you made to the codec interface. I noticed you > added a separate accessor for the size: > > ssize_t pn_data_encoded_size(pn_data_t *data); > > This is alongside the original encode method: > > ssize_t pn_data_encode(pn_data_t *data, char *bytes, size_t size); > > I think this API choice while nice in that it is backwards compatible is > also going to result in code that is roughly twice as slow as it needs to > be in the most common case. Based on my experience implementing and > profiling codec in C, Python, and Java, computing the size of the encoded > data seems to usually be roughly the same amount of work as actually > encoding it regardless of the implementation language. Therefore code like > this: > > if (buffer_size() < pn_data_encoded_size(data)) grow_buffer(); > pn_data_encode(data, buffer, buffer_size()); > > Can end up being roughly twice as slow as code like this: > > ssize_t err; > while ((err = pn_data_encode(data, buffer, buffer_size())) == > PN_OVERFLOW) { > grow_buffer(); > } > > Admittedly the latter form is much more awkward in those cases where you > don't care about performance, so I'm all for providing something nicer, but > I think a better API change would be to steal a page from the C stdio.h > APIs and have pn_data_encode always return the number of bytes that would > have been written had there been enough space. This allows you to write the > simplified encode as above: > > if (buffer_size() < pn_data_encode(data, NULL, 0)) grow_buffer(); > pn_data_encode(data, buffer, buffer_size()); > > Or use a more optimal form: > > ssize_t n = pn_data_encode(data, buffer, buffer_size()); > if (n > buffer_size()) { > grow_buffer(); > pn_data_encode(data, buffer, buffer_size()); > } > > This makes the slow/convenient form possible, and provides some options > that are a bit less awkward than the loop, but it also makes it very clear > that when you use the slow/convenient form you are incurring roughly twice > the cost of the alternative. > > Normally I wouldn't be overly fussed by something like this, and I realize > what I'm suggesting is a breaking change relative to what you provided, but > based on what profiling we've done in the past, codec is probably the most > significant source of overhead that we add to an application, and exactly > this sort of double encode effect is almost always one of the first things > you hit when you try to optimize. Given this, I think it would be a good > thing if the API accurately reflects the relative cost of the different > styles of use. > > Thoughts? > > --Rafael >
