Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

Hi Ludovic,

I hope you don't mind if I continue to pursue this a little...

2008/6/12 Ludovic Courtès <address@hidden>:
> Hi Neil,
>
> "Neil Jerram" <address@hidden> writes:
>
>> So, one way of solving this would be for an `unbuffered' port
>> temporarily to get a buffer of the right size, and then to use the
>> fill_input logic as it currently stands.  That would achieve your
>> objective of mapping onto a single N-byte read(2) call.
>
> Surely this would work, but I can think of several issues:
>
>  1. Thread safety.  OK, ports are not thread-safe and it'd take more
>     than this to make them thread-safe, but passing arguments
>     explicitly (as the BUFFER and SIZE arguments of `read') is a step
>     forward, while mutating the buffer pointer and size in the port is
>     a step backward.

Agreed in theory, but I don't think it's a significant step backwards,
because ports are fundamentally not thread-safe (as you've already
said above), and already have lots of implicit arguments.

>  2. Performance.  Another case where the `read' method improves on
>     performance: when more data is requested than can fit in the port's
>     buffer, the port buffer is completely bypassed.  This allows the
>     data to be read in one shot, avoiding memcpys from the port's
>     buffer to the caller's buffer.

Agreed.  Yesterday I was worried about possible confusion from
bypassing a port's read buffer at a time when the read buffer has some
data in it.  But in fact this is not a problem, because
scm_fill_input() should be (and in practice is) only called when the
port's read buffer is empty.

>  3. Aesthetics.  The `read' method looks more natural to me, since it's
>     just like `read(2)'.  Also, as mentioned earlier, `read' methods
>     don't have to be aware of the port's internal buffer: they just
>     need to fill in the caller-supplied buffer.  When `scm_fill_input ()'
>     is called, it just invokes `read', passing it the port's internal
>     buffer and using its return value to update `pt->read_end'.

Aesthetics are always tricky.  I accept your point here, but against
that we have to count

- the cost of adding a new method to libguile's port definition

- the benefit of (some) existing fill_input code being able to work
with caller-supplied buffers automatically.  (I say "some", because
some fill_input implementations may make assumptions about the read
buffer size and so not automatically take advantage of a larger
caller-supplied buffer.  But at least the important fport case will
work automatically.)

>> But that advantage is also (in some cases) the disadvantage of having
>> to do an additional memcpy of the data.
>
> There's no additional memcpy, as mentioned above.

Indeed, my mistake.

Based on all this, I'd like to update my suggestion so that

- scm_fill_input_buf always uses the caller's buffer

- the remaining scm_fill_input_buf logic is inlined into scm_c_read
(since that is the only place that uses scm_fill_input_buf, and
inlining it permits some further simplifications).

New patch (smaller and simpler) is attached; please let me know if
this sways your opinion at all!

         Neil

scm_fill_input_buf.patch
Description: Binary data