The following C function is a wrapper for the `read' system call. It returns a pair that contains the number of bytes read and the data. Is this the right way to initialize a u8vector from C?
___SCMOBJ read_bytes (int fd, size_t count) { void *data = malloc (count); ___SCMOBJ result;
if (data == NULL) result = ___make_pair (___BOOLEAN (0), ___BOOLEAN (0), ___STILL); else { int r = read (fd, data, count); ___SCMOBJ retcode; ___SCMOBJ ___err; ___BEGIN_SFUN_INT_TO_SCMOBJ (r, retcode, 1); ___END_SFUN_INT_TO_SCMOBJ (r, retcode, 1); if (r == 0) result = ___make_pair (retcode, ___BOOLEAN (0), ___STILL); else if (r > 0) { ___SCMOBJ data_vect; data_vect = ___alloc_scmobj(___sU8VECTOR, count, ___STILL); ___EXT (___release_scmobj) (data_vect); memcpy (___BODY (data_vect), data, r); result = ___make_pair (retcode, data_vect, ___STILL); } else result = ___make_pair (retcode, ___BOOLEAN (0), ___STILL); free (data); ___release_scmobj (result); } return result; }
Thanks,
--Vijay
Spontaneously I'd say so, with one important point missing:
You need handling for heap overflow.
For the u8vector clearly, and I believe for each pair allocation too.
Please update your code and post again for another round of feedback.
For your reference find a sample routine with heap overflow allocation below, and also see places doing the same in Gambit's code.
I'm not clear on the difference between the ___release_scmobj call you do and the ___still_obj_refcount_dec call the code below does - anyone feel free to clarify/remind.
Mikael
(define (make-still-u32vector len) (let loop () (let ((r ((c-lambda (unsigned-int) scheme-object "___result = ___EXT(___alloc_scmobj)(___sU32VECTOR,___arg1*4,___STILL); if (!___FIXNUMP(___result)) ___still_obj_refcount_dec(___result); // Inspired by ##make-u8vector of ___kernel.scm . ") len)))
; Recursion here inspired by ##make-u8vector of ___kernel.scm . (if (not (u32vector? r)) (begin (error "Heap overflow when allocating u32vector" len) ; (##raise-heap-overflow-exception) should do fine too. (loop)) (begin (u32vector-fill! r 0) r)))))
2013/9/23 Vijay Mathew vijay.the.lisper@gmail.com
The following C function is a wrapper for the `read' system call. It returns a pair that contains the number of bytes read and the data. Is this the right way to initialize a u8vector from C?
___SCMOBJ read_bytes (int fd, size_t count) { void *data = malloc (count); ___SCMOBJ result;
if (data == NULL) result = ___make_pair (___BOOLEAN (0), ___BOOLEAN (0), ___STILL); else { int r = read (fd, data, count); ___SCMOBJ retcode; ___SCMOBJ ___err; ___BEGIN_SFUN_INT_TO_SCMOBJ (r, retcode, 1); ___END_SFUN_INT_TO_SCMOBJ (r, retcode, 1); if (r == 0) result = ___make_pair (retcode, ___BOOLEAN (0), ___STILL); else if (r > 0) { ___SCMOBJ data_vect; data_vect = ___alloc_scmobj(___sU8VECTOR, count, ___STILL); ___EXT (___release_scmobj) (data_vect); memcpy (___BODY (data_vect), data, r); result = ___make_pair (retcode, data_vect, ___STILL); } else result = ___make_pair (retcode, ___BOOLEAN (0), ___STILL); free (data); ___release_scmobj (result); } return result; }
Thanks,
--Vijay
Gambit-list mailing list Gambit-list@iro.umontreal.ca https://webmail.iro.umontreal.ca/mailman/listinfo/gambit-list
On 2013-09-23, at 11:14 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
The following C function is a wrapper for the `read' system call. It returns a pair that contains the number of bytes read and the data. Is this the right way to initialize a u8vector from C?
There are a few things I would do differently, such as heap overflow checking. But for the particular task of providing a wrapper to the read system call I would use a different interface. Why don't you allocate the buffer in Scheme before calling read? That would simplify the C code (avoiding GC issues) and would be more efficient.
Marc
Hi,
Thanks for reviewing my code.
I have updated the C function as follows:
int read_bytes (int fd, size_t count, ___SCMOBJ data_vect) { void *data = malloc (count); int result;
if (data == NULL) result = -1; else { result = read (fd, data, count); if (result > 0) memcpy (___BODY (data_vect), data, result); free (data); } return result; }
A call from scheme will look like:
(read-bytes fd count (make-u8vector count 0))
I suppose now I don't have to worry about leaks or heap-overflow-exceptions.
Thanks,
--Vijay
On Tue, Sep 24, 2013 at 8:52 PM, Marc Feeley feeley@iro.umontreal.cawrote:
On 2013-09-23, at 11:14 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
The following C function is a wrapper for the `read' system call. It returns a pair that contains the number of bytes read and the data. Is this the right way to initialize a u8vector from C?
There are a few things I would do differently, such as heap overflow checking. But for the particular task of providing a wrapper to the read system call I would use a different interface. Why don't you allocate the buffer in Scheme before calling read? That would simplify the C code (avoiding GC issues) and would be more efficient.
Marc
Hi Vijay,
Looks good.
A possible improvement is to drop the malloc() call you do and just read() straight into ___BODY(data_vect) , this saves you a round of copying too, and is also in some limited sense safer as there's no malloc() that could return NULL anymore.
Last note would be that at the time SMP is released, every access out there of the Scheme heap from C calls such as ___BODY() will need to be gone through as then, I suppose, unlike now, GC:s could happen also during FFI calls - my best understanding is that the primary solution will be using ___STILL objects only. Interestingly enough, this is what |make-u8vector| makes already today, for u8vectors bigger than ___MSECTION_BIGGEST , however that logic is not a lot to be relied on so I guess it'll be relevant to implement a |make-still-u8vector| or alike then.
Mikael
2013/9/25 Vijay Mathew vijay.the.lisper@gmail.com
Hi,
Thanks for reviewing my code.
I have updated the C function as follows:
int read_bytes (int fd, size_t count, ___SCMOBJ data_vect)
{ void *data = malloc (count); int result;
if (data == NULL) result = -1; else { result = read (fd, data, count); if (result > 0) memcpy (___BODY (data_vect), data, result); free (data); } return result; }
A call from scheme will look like:
(read-bytes fd count (make-u8vector count 0))
I suppose now I don't have to worry about leaks or heap-overflow-exceptions.
Thanks,
--Vijay
On Tue, Sep 24, 2013 at 8:52 PM, Marc Feeley feeley@iro.umontreal.cawrote:
On 2013-09-23, at 11:14 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
The following C function is a wrapper for the `read' system call. It returns a pair that contains the number of bytes read and the data. Is this the right way to initialize a u8vector from C?
There are a few things I would do differently, such as heap overflow checking. But for the particular task of providing a wrapper to the read system call I would use a different interface. Why don't you allocate the buffer in Scheme before calling read? That would simplify the C code (avoiding GC issues) and would be more efficient.
Marc
Hallo,
On Wed, Sep 25, 2013 at 10:48 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
free (data); }
return result; }
I suppose you must return ___FIX(result)
Cheers,
On 2013-09-25, at 4:48 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
Hi,
Thanks for reviewing my code.
I have updated the C function as follows:
int read_bytes (int fd, size_t count, ___SCMOBJ data_vect) { void *data = malloc (count); int result;
if (data == NULL) result = -1; else { result = read (fd, data, count); if (result > 0) memcpy (___BODY (data_vect), data, result); free (data); } return result; }
A call from scheme will look like:
(read-bytes fd count (make-u8vector count 0))
I suppose now I don't have to worry about leaks or heap-overflow-exceptions.
You can simplify you code further to:
int read_bytes (int fd, size_t count, ___SCMOBJ data_vect) { return ___FIX(read (fd, ___BODY(data_vect), count)); }
Marc
Thanks again for all the great feedback!
I think the call to ____FIX() is not required if Scheme code access the C function through a declaration like this:
(define read-bytes (c-lambda (int unsigned-int scheme-object) int "read_bytes"))
Is that correct?
Thanks,
--Vijay
On Wed, Sep 25, 2013 at 8:34 PM, Marc Feeley feeley@iro.umontreal.cawrote:
On 2013-09-25, at 4:48 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
Hi,
Thanks for reviewing my code.
I have updated the C function as follows:
int read_bytes (int fd, size_t count, ___SCMOBJ data_vect) { void *data = malloc (count); int result;
if (data == NULL) result = -1; else { result = read (fd, data, count); if (result > 0) memcpy (___BODY (data_vect), data, result); free (data); } return result; }
A call from scheme will look like:
(read-bytes fd count (make-u8vector count 0))
I suppose now I don't have to worry about leaks or
heap-overflow-exceptions.
You can simplify you code further to:
int read_bytes (int fd, size_t count, ___SCMOBJ data_vect) { return ___FIX(read (fd, ___BODY(data_vect), count)); }
Marc
Yes, and then take proper care of the other arguments i.e.
(define read-bytes (c-lambda (int unsigned-int scheme-object) int "___result = read_bytes(___ARG1,___BODY(___ARG3),___ARG2);"))
2013/9/26 Vijay Mathew vijay.the.lisper@gmail.com
Thanks again for all the great feedback!
I think the call to ____FIX() is not required if Scheme code access the C function through a declaration like this:
(define read-bytes (c-lambda (int unsigned-int scheme-object) int "read_bytes"))
Is that correct?
Thanks,
--Vijay
On Wed, Sep 25, 2013 at 8:34 PM, Marc Feeley feeley@iro.umontreal.cawrote:
On 2013-09-25, at 4:48 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
Hi,
Thanks for reviewing my code.
I have updated the C function as follows:
int read_bytes (int fd, size_t count, ___SCMOBJ data_vect) { void *data = malloc (count); int result;
if (data == NULL) result = -1; else { result = read (fd, data, count); if (result > 0) memcpy (___BODY (data_vect), data, result); free (data); } return result; }
A call from scheme will look like:
(read-bytes fd count (make-u8vector count 0))
I suppose now I don't have to worry about leaks or
heap-overflow-exceptions.
You can simplify you code further to:
int read_bytes (int fd, size_t count, ___SCMOBJ data_vect) { return ___FIX(read (fd, ___BODY(data_vect), count)); }
Marc
On 2013-09-26, at 12:21 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
Thanks again for all the great feedback!
I think the call to ____FIX() is not required if Scheme code access the C function through a declaration like this:
(define read-bytes (c-lambda (int unsigned-int scheme-object) int "read_bytes"))
Is that correct?
That is correct. However, using ___FIX() is faster for converting a C integer to a Scheme fixnum. And ___INT() is faster for converting a Scheme fixnum to a C integer. So better performance is obtained with:
(define read-bytes (c-lambda (scheme-object scheme-object scheme-object) scheme-object "___result = ___FIX(read (___INT(___arg1),___BODY(___arg3),___INT(___arg2));"))
Marc
Cool! Ahh, this is because with ___FIX() it's implicit that the passed value will fit in a fixnum whereas with (c-lambda ([args]) int code) needs a conditional for if the return value would be a bignum.
Beyond this, can it ever be faster with regard to use (##c-code code) than (c-lambda (scheme-object:s) scheme-object code)?
Vijay: Feel free to document what you got from this on the wiki - this is like the most essential stuff ever so it deserves a place there.
2013/9/26 Marc Feeley feeley@iro.umontreal.ca
On 2013-09-26, at 12:21 AM, Vijay Mathew vijay.the.lisper@gmail.com wrote:
Thanks again for all the great feedback!
I think the call to ____FIX() is not required if Scheme code access the
C function through a declaration like this:
(define read-bytes (c-lambda (int unsigned-int scheme-object) int
"read_bytes"))
Is that correct?
That is correct. However, using ___FIX() is faster for converting a C integer to a Scheme fixnum. And ___INT() is faster for converting a Scheme fixnum to a C integer. So better performance is obtained with:
(define read-bytes (c-lambda (scheme-object scheme-object scheme-object) scheme-object "___result = ___FIX(read (___INT(___arg1),___BODY(___arg3),___INT(___arg2));"))
Marc