Re: [PATCH] Add 'bytevector-slice'.

[Ludovic Courtès]

Hi,

Maxime Devos <maximedevos@telenet.be> skribis:

> The only thing missing for me, is a procedure
> 'bytevector-slice/read-only' and 'bytevector-slice/write-only', then I
> could throw the module implementing the wrapping in Scheme-GNUnet and
> the overhead incurred by wrapping away.

I did consider the read-only part, but it’s not really implementable
currently as SCM_F_BYTEVECTOR_IMMUTABLE flag is even ignored by
instructions, and fixing it is far beyond the scope of this patch:

  https://issues.guix.gnu.org/60779

> On 11-01-2023 16:00, Ludovic Courtès wrote:

[...]

>> +(use-modules (rnrs bytevectors)
>> +             (rnrs bytevectors gnu))
>
> I thought that R6RS reserved (rnrs ...) to the RnRS process, so I
> would think that naming it (rnrs bytevectors gnu) would be
> standards-incompliant, though I cannot find in the specification, so
> maybe it isn't actually reserved.
>
> (SRFI looks a bit looser to me, so I find the (srfi ... gnu)
> acceptable, but (rnrs ... gnu) looks weird to me, I would propose
> (ice-9 bytevector-extensions) or such.).

I’ll stick to gnu.scm because that’s the convention we’ve used for
extensions so far, and “gnu” is arguably “reserved”.

>> +Copyright (C) 1996-1997, 2000-2005, 2009-2023 Free Software Foundation,
>
>
> Where does this year 2022 come from?  Does a previous version of this
> patch predate the new year?

I started working on it in December and my ‘write-file-hooks’ updated it.

>> +  c_offset = scm_to_size_t (offset);
>> +
>> +  if (SCM_UNBNDP (size))
>> +    {
>> +      if (c_offset < SCM_BYTEVECTOR_LENGTH (bv))
>> +        c_size = SCM_BYTEVECTOR_LENGTH (bv) - c_offset;
>> +      else
>> +        c_size = 0; > +    }
>> +  else
>> +    c_size = scm_to_size_t (size);
>> +
>> +  if (c_offset + c_size > SCM_BYTEVECTOR_LENGTH (bv))
>> +    scm_out_of_range (FUNC_NAME, offset);
>
>
> If offset=SIZE_MAX-1 and size=1, this will overflow to 0 and hence not
> trigger the error reporting.  This bounds check needs to be rewritten,
> with corresponding additional tests.

OK.

> IIUC, if you use bytevector-slice iteratively, say:
>
> (let loop ((bv some-initial-value)
>            (n large-number))
>   (if (> n 0)
>       (loop (bytevector-slice bv 0 (bytevector-length bv))
>             (- n 1))
>       bv))
>
> you will end up with a bytevector containing a reference to a
> bytevector containing a reference to ... containing a reference to the
> original reference, in a chain of length ≃ large-number.

The ‘parent’ word is here just so the backing memory isn’t reclaimed
while the slice is still alive.

Whether there’s a long chain of ‘parent’ links or not makes no
difference to GC performance nor to memory usage.

> Do you know what this 'parent' field even is for?  Going by some
> comments in 'libguile/bytevectors.c', it is for GC reasons, but going
> by the existence of the 'bytevector->pointer' + 'pointer->bytevector'
> trick which destroys 'parent' information, it seems unnecessary to me.

Like I wrote, it was introduced to keep backing memory alive, generally.
With ‘pointer->bytevector’, we want to make sure the original pointer
remains live as long as the bytevector is live (pointer objects can have
a finalizer, and that finalizer must not run while the bytevector is
live).

> Nowadays a https://.../ instead of a mail address, is more
> conventional and useful, I'd think.  Its even used in old files,
> e.g. libguile/r6rs-ports.c.

Noted.

> A test is missing for the case where the size is unaligned instead of
> the offset.

Noted.

I’ll come up with a second version taking this into account.

Thanks!

Ludo’.