[PATCH vkd3d 1/3] vkd3d-shader: Don't resize the buffer when there is enough free space.

Matteo Bruni matteo.mystral at gmail.com
Mon Aug 16 12:14:27 CDT 2021


On Mon, Aug 16, 2021 at 7:06 PM Henri Verbeet <hverbeet at gmail.com> wrote:
>
> On Mon, 16 Aug 2021 at 18:39, Matteo Bruni <matteo.mystral at gmail.com> wrote:
> > Otherwise, I know that I only noticed this issue and started writing
> > these patches because I was in fact getting out of memory errors :)
> > IIRC vkd3d_string_buffer_resize() would at least double the size of
> > the buffer every time it's called and vkd3d_string_buffer_vprintf() in
> > practice ended up writing 0 bytes to the buffer relatively often (I
> > haven't investigated that part, but it seems legit, if weird). Also
> > apparently the code often doubled the size one too many times. I'll
> > check that again but I don't think I saw that after this patch.
> >
> My guess would be that it's caused by the "if (!buffer->content_size
> && !vkd3d_string_buffer_resize(buffer, 32))" line. As mentioned, that
> will cause the buffer size to double when calling
> vkd3d_string_buffer_vprintf() after a vkd3d_string_buffer_clear(),
> until "content_size" actually becomes non-zero.

Ah yeah, it's not just the first time, but "every" first time the same
buffer is returned after a get().

> That could be
> mitigated by doing something like this in vkd3d_string_buffer_resize()
>
>     if (rc >= 0)
>     {
>         new_buffer_size = max(buffer->buffer_size, 32);
>         while ((unsigned int)rc >= new_buffer_size - buffer->content_size)
>             new_buffer_size *= 2;
>     }
>     else
>     {
>         new_buffer_size = max(buffer->buffer_size * 2, 32);
>     }
>
> but in the end there's just no need for that particular
> vkd3d_string_buffer_resize() call.
>
> > > We would do an unnecessary resize when calling
> > > vkd3d_string_buffer_vprintf() the first time after a
> > > vkd3d_string_buffer_clear(), although the best way to fix that would
> > > seem to be simply getting rid of the vkd3d_string_buffer_resize() call
> > > responsible for that, like patch 2/3 in this series does. It was added
> > > by commit 5fb9bcdd148ed728d90a420f209cdfcc03af24ac, but that same
> > > commit also adds the max() in vkd3d_string_buffer_resize() that should
> > > make it superfluous.
> >
> > I guess one possible mitigation would be to rename
> > vkd3d_string_buffer_resize() to vkd3d_string_buffer_reserve() or
> > similar, to clarify that it won't reallocate the buffer when
> > unnecessary.
>
> We could also simply use vkd3d_array_reserve() in
> vkd3d_string_buffer_vprintf(); it would essentially look something
> like this:
>
>     if (rc >= 0)
>         vkd3d_array_reserve(..., rc, ...);
>     else
>         vkd3d_array_reserve(..., buffer->buffer_size * 2, ...);

Yeah, I realized while updating the patch that the current
vkd3d_string_buffer_resize() is strictly implementing the reallocation
side of the vsnprintf() loop (while wined3d's version most likely
predated the more generic reallocation helper). I'll probably go with
this, thanks.



More information about the wine-devel mailing list