[Development] Enhancement to QIODevice?

[Mark Gaiser]

On Wed, Sep 16, 2015 at 1:13 PM, Andrzej Ostruszka <
andrzej.ostruszka at gmail.com> wrote:

> On 09/16/2015 08:57 AM, Thiago Macieira wrote:
> >>> You could use peek() to search the buffer, then read() exactly as much
> as
> >>> you really need.
> >>
> >> I understand that we are talking about
> >> QIODevicePrivateLinearBuffer::peek() here.
> >
> > No, I meant QIODevice::peek()
>
> First of all - I do not claim that I have mastered Qt internals!
> So everything below you should take with a grain of salt :)
>
> QIODevice::peek() is implemented in terms of QIODevice::read() (which
> then uses QIODevicePrivate::read() which does the memcpy()) and later
> putting it back.  Pretty convoluted but it really does not matter - I
> just assumed the simpler case of direct peek into the buffer.
>
> >> In your approach I'd be copying data twice, but I can eliminate one with
> >> peek()ing
> >> directly to the output buffer and later skip()ping in successful case.
> >> However in case
> >> when delimiter is not yet available I'd have unnecessary copying.
> >
> > Then please provide a patch that avoids the unnecessary second copy when
> the
> > read() destination is a null pointer.
>
> Here I must say that I don't understand.  Let me rephrase what I've said
> earlier.  Suppose that there is some data in buffer and I want to know
> if delimiter is in it.  Without direct access to this buffer memory I
> can only:
> 1. copy (by peeking()) it to some other place (possibly the final output
> buffer)
> 2. search for delimiter in my copy
> 3. if found: drop "number of bytes to delimiter" from buffer
>
> What I'm saying is that in successful case (when delimiter is found)
> this sequence is optimal.  However, in case when there is no delimiter
> yet in the buffer, operation 1 is wasted (not really needed).  And I
> don't see how to avoid it without allowing user to search in the
> internal buffer.
>
> Possible ways to allow for searching in this buffer are (I'm shortening
> names here):
> A. Add optional param to canReadLine() readLine() (for QIODP or maybe
> even QIOD)
> B. Add this QIODPLB::countTo() functionality (which does not break or
> interfere with anything)
> C. Add QIODPLB::data() function to buffer which will return "first" (I'm
> talking in terms of QIODPLB implementation)
> D. ???
>
> As for C.  This is another solution - which also does not interfere with
> anything.  The amount of data is already available via size().
> Then any kind of "packet" termination can be implemented (not just by
> single char) on top of it.  Value returned by this QIODPLB::data() is
> only invalidated by explicit user reading so this would be pretty safe
> too and its only one line of code.
>
> So what do you think?
>
> >> And while peek()ing I'd have to use the min(max size of my output
> >> buffer, size())
> >> which would mean that I'd be copying "overlapping" parts more than once.
> >
> > I didn't understand this.
>
> What I wanted to say is that:
> - in operation 1 above the number of bytes to peek need to be specified
> - the only reasonable choice - without using some application specific
> heuristic - is the amount of bytes already available (lets forget for a
> moment possible constrains of my output buffer size)
>
> Then in situation when in buffer is more then just one "line" available
> I would be copying parts of data after first delimiter in order to later
> put them back and read them again on next data arrival.
>
> > I think there are easier ways to do this, so I don't think it makes
> sense to
> > accept this suggestion.
>
> Yes one can peek() at buffer but as I've tried to state this approach
> has some drawbacks (from performance point of view).  I can also
> understand that maintainers are worried about growing complexity and
> maintenance burden but honestly I don't see any growth in these from
> options B and C above.  These are internal, no testing for them is
> needed, they don't interfere with any other functionality etc.
>
> So Your Honour, what is the verdict? ;)
>
> Regards
> Andrzej
>

I'm curious, why is QIODevicePrivate::peek (both versions of it) doing:
- calling read (which resizes the internal buffer to whatever is left after
reading). It does this via memmove.
- then it restores the buffer to it's original by taking the current buffer
and appending what was read. It does this via memcpy.

Rather then doing:
- call QIODevicePrivateLinearBuffer::peek (on the buffer object in
QIODevicePrivate) and leave the internal buffer untouched? The function is
just sitting there [1], but it isn't being used by either
QIODevice(Private)::peek function.

It would avoid fiddling with the internal buffer and just copy (a part of)
the internal buffer to the user. That is what peek should do according to
the documentation.
Peek would the not call QIODevice::read anymore.

Performance wise, this likely makes peek faster (no more read call, no more
memmove, no more memcpy on the internal buffer), memory wise there still is
the redundant copy from (a part of) the internal buffer to the user side.
Would it be an idea to add a third peek method to QIODevice that returns
the const char* from the internal buffer, perhaps offset by a given
position if needed?
That would allow for truly "read only" peeks with no copy in between. A
"low level peek" if you will.

[1]
http://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/io/qiodevice_p.h#n110
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20150916/bbeaaa80/attachment.html>