On Mon, 16 Aug 2021 at 18:39, Matteo Bruni matteo.mystral@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. 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, ...);