[PATCH vkd3d 1/3] vkd3d-shader: Don't resize the buffer when there is enough free space.
Signed-off-by: Matteo Bruni mbruni@codeweavers.com --- libs/vkd3d-shader/vkd3d_shader_main.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 78312a14..9a4560a3 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -41,10 +41,12 @@ static void vkd3d_string_buffer_clear(struct vkd3d_string_buffer *buffer)
static bool vkd3d_string_buffer_resize(struct vkd3d_string_buffer *buffer, int rc) { - unsigned int new_buffer_size = buffer->buffer_size * 2; + unsigned int new_buffer_size = max(buffer->buffer_size * 2, 32); char *new_buffer;
- new_buffer_size = max(new_buffer_size, 32); + if (rc < buffer->buffer_size - buffer->content_size) + return true; + while (rc > 0 && (unsigned int)rc >= new_buffer_size - buffer->content_size) new_buffer_size *= 2; if (!(new_buffer = vkd3d_realloc(buffer->buffer, new_buffer_size)))
Signed-off-by: Matteo Bruni mbruni@codeweavers.com --- libs/vkd3d-shader/vkd3d_shader_main.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/libs/vkd3d-shader/vkd3d_shader_main.c b/libs/vkd3d-shader/vkd3d_shader_main.c index 9a4560a3..8bb5ac42 100644 --- a/libs/vkd3d-shader/vkd3d_shader_main.c +++ b/libs/vkd3d-shader/vkd3d_shader_main.c @@ -35,7 +35,6 @@ void vkd3d_string_buffer_cleanup(struct vkd3d_string_buffer *buffer)
static void vkd3d_string_buffer_clear(struct vkd3d_string_buffer *buffer) { - buffer->buffer[0] = '\0'; buffer->content_size = 0; }
@@ -66,9 +65,6 @@ int vkd3d_string_buffer_vprintf(struct vkd3d_string_buffer *buffer, const char * va_list a; int rc;
- if (!buffer->content_size && !vkd3d_string_buffer_resize(buffer, 32)) - return -1; - for (;;) { rem = buffer->buffer_size - buffer->content_size; @@ -147,11 +143,6 @@ struct vkd3d_string_buffer *vkd3d_string_buffer_get(struct vkd3d_string_buffer_c if (!(buffer = vkd3d_malloc(sizeof(*buffer)))) return NULL; vkd3d_string_buffer_init(buffer); - if (!vkd3d_string_buffer_resize(buffer, 1)) - { - vkd3d_free(buffer); - return NULL; - } } else {
On Fri, 13 Aug 2021 at 16:15, Matteo Bruni mbruni@codeweavers.com wrote:
@@ -147,11 +143,6 @@ struct vkd3d_string_buffer *vkd3d_string_buffer_get(struct vkd3d_string_buffer_c if (!(buffer = vkd3d_malloc(sizeof(*buffer)))) return NULL; vkd3d_string_buffer_init(buffer);
if (!vkd3d_string_buffer_resize(buffer, 1)){vkd3d_free(buffer);return NULL;}
I think Zebediah successfully convinced me it's useful for vkd3d_string_buffer_get() to return empty but initialised buffers, like wined3d's string_buffer_get() does. Note that a consequence of not doing this is that we can no longer safely do something like the following:
buffer = vkd3d_string_buffer_get(...);
if (flags & FLAT) vkd3d_string_buffer_printf(buffer, "flat "); if (flags & NOPERSPECTIVE) vkd3d_string_buffer_printf(buffer, "noperspective "); if (flags & CENTROID) vkd3d_string_buffer_printf(buffer, "centroid "); printf("%svec4 v;\n", buffer->buffer);
vkd3d_string_buffer_release(..., buffer);
because now buffer->buffer is potentially NULL.
On Mon, Aug 16, 2021 at 6:05 PM Henri Verbeet hverbeet@gmail.com wrote:
On Fri, 13 Aug 2021 at 16:15, Matteo Bruni mbruni@codeweavers.com wrote:
@@ -147,11 +143,6 @@ struct vkd3d_string_buffer *vkd3d_string_buffer_get(struct vkd3d_string_buffer_c if (!(buffer = vkd3d_malloc(sizeof(*buffer)))) return NULL; vkd3d_string_buffer_init(buffer);
if (!vkd3d_string_buffer_resize(buffer, 1)){vkd3d_free(buffer);return NULL;}I think Zebediah successfully convinced me it's useful for vkd3d_string_buffer_get() to return empty but initialised buffers, like wined3d's string_buffer_get() does. Note that a consequence of not doing this is that we can no longer safely do something like the following:
buffer = vkd3d_string_buffer_get(...); if (flags & FLAT) vkd3d_string_buffer_printf(buffer, "flat "); if (flags & NOPERSPECTIVE) vkd3d_string_buffer_printf(buffer, "noperspective "); if (flags & CENTROID) vkd3d_string_buffer_printf(buffer, "centroid "); printf("%svec4 v;\n", buffer->buffer); vkd3d_string_buffer_release(..., buffer);because now buffer->buffer is potentially NULL.
Makes sense (and I totally forgot, not surprising in the least). In that case though, I think it would be nicer to allocate the initial buffer in vkd3d_string_buffer_init(), like in wined3d, instead of having to handle it in two places (with different allocation sizes, and missing the case where you don't call vkd3d_string_buffer_get() at all).
On Mon, 16 Aug 2021 at 18:49, Matteo Bruni matteo.mystral@gmail.com wrote:
Makes sense (and I totally forgot, not surprising in the least). In that case though, I think it would be nicer to allocate the initial buffer in vkd3d_string_buffer_init(), like in wined3d, instead of having to handle it in two places (with different allocation sizes, and missing the case where you don't call vkd3d_string_buffer_get() at all).
In some ways, yes. I think the main point behind 5fb9bcdd148ed728d90a420f209cdfcc03af24ac was to make vkd3d_string_buffer_init() never fail though, and by extension things like vkd3d_shader_message_context_init(). I'm fine with either variant, but would prefer not switching it back and forth every couple of months.
Signed-off-by: Matteo Bruni mbruni@codeweavers.com --- I find it can be helpful in the unlikely case that it triggers.
I guess it could be considered a bit of an information leak but probably not a big deal?
include/private/vkd3d_memory.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/private/vkd3d_memory.h b/include/private/vkd3d_memory.h index f6846d66..6f264e30 100644 --- a/include/private/vkd3d_memory.h +++ b/include/private/vkd3d_memory.h @@ -37,7 +37,7 @@ static inline void *vkd3d_malloc(size_t size) static inline void *vkd3d_realloc(void *ptr, size_t size) { if (!(ptr = realloc(ptr, size))) - ERR("Out of memory.\n"); + ERR("Out of memory, size %u.\n", size); return ptr; }
On Fri, 13 Aug 2021 at 16:15, Matteo Bruni mbruni@codeweavers.com wrote:
@@ -37,7 +37,7 @@ static inline void *vkd3d_malloc(size_t size) static inline void *vkd3d_realloc(void *ptr, size_t size) { if (!(ptr = realloc(ptr, size)))
ERR("Out of memory.\n");
ERR("Out of memory, size %u.\n", size);}
The change seems fine in principle, but using %u would introduce compiler warnings for 64-bit builds.
On Fri, 13 Aug 2021 at 16:15, Matteo Bruni mbruni@codeweavers.com wrote:
static bool vkd3d_string_buffer_resize(struct vkd3d_string_buffer *buffer, int rc) {
- unsigned int new_buffer_size = buffer->buffer_size * 2;
- unsigned int new_buffer_size = max(buffer->buffer_size * 2, 32); char *new_buffer;
- new_buffer_size = max(new_buffer_size, 32);
- if (rc < buffer->buffer_size - buffer->content_size)
return true;
That doesn't quite handle the case where "rc" is -1. Perhaps it's fine to assume it isn't these days, but in that case we might as well get rid of the rest of the code handling that case as well. The patch also seems a little superfluous though; we already have a similar check in vkd3d_string_buffer_vprintf().
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.
On Mon, Aug 16, 2021 at 6:05 PM Henri Verbeet hverbeet@gmail.com wrote:
On Fri, 13 Aug 2021 at 16:15, Matteo Bruni mbruni@codeweavers.com wrote:
static bool vkd3d_string_buffer_resize(struct vkd3d_string_buffer *buffer, int rc) {
- unsigned int new_buffer_size = buffer->buffer_size * 2;
- unsigned int new_buffer_size = max(buffer->buffer_size * 2, 32); char *new_buffer;
- new_buffer_size = max(new_buffer_size, 32);
- if (rc < buffer->buffer_size - buffer->content_size)
return true;That doesn't quite handle the case where "rc" is -1. Perhaps it's fine to assume it isn't these days, but in that case we might as well get rid of the rest of the code handling that case as well. The patch also seems a little superfluous though; we already have a similar check in vkd3d_string_buffer_vprintf().
I entirely forgot about the rc == -1 case... I can fix that.
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.
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.
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, ...);
On Mon, Aug 16, 2021 at 7:06 PM Henri Verbeet hverbeet@gmail.com wrote:
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.
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.
-
Henri Verbeet -
Matteo Bruni -
Matteo Bruni