[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Add a `read' method for ports
From: |
Neil Jerram |
Subject: |
Re: [PATCH] Add a `read' method for ports |
Date: |
Fri, 13 Jun 2008 01:18:13 +0100 |
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
- [PATCH] Add a `read' method for ports, Ludovic Courtès, 2008/06/01
- Re: [PATCH] Add a `read' method for ports, Ludovic Courtès, 2008/06/08
- Re: [PATCH] Add a `read' method for ports, Neil Jerram, 2008/06/09
- Re: [PATCH] Add a `read' method for ports, Ludovic Courtès, 2008/06/09
- Re: [PATCH] Add a `read' method for ports, Neil Jerram, 2008/06/11
- Re: [PATCH] Add a `read' method for ports, Neil Jerram, 2008/06/11
- Re: [PATCH] Add a `read' method for ports, Ludovic Courtès, 2008/06/12
- Re: [PATCH] Add a `read' method for ports,
Neil Jerram <=
- Re: [PATCH] Add a `read' method for ports, Ludovic Courtès, 2008/06/24