nicbarker
[Renderers/Raylib] Remove allocations from raylib text rendering
https://github.com/nicbarker/clay/blob/5831a8ac7c98a26c86d2dc8dc09d55d2235443f5/renderers/raylib/clay_renderer_raylib.c#L142
At least once per frame (if text is present), memory is being allocated and freed from the heap. This is bad practice, as memory allocations can be nondeterministic in run time and lead to heap fragmentation. Some suggested remedies:
- Allocate once, resize later: Create a buffer that is resized when necessary such that no further allocations will be necessary after the largest growth. There will be a new renderer cleanup function to free this buffer.
- Fixed buffer: Use a static buffer whose max length is configurable at compile-time.
- User provided: Have it be the user's responsibility to provide a buffer to the render function that can be used as a scratchpad. This could potentially be done by having a callback as an argument for requesting a buffer of a given size. The user will manually free the buffer themselves when clay is no longer needed.
- Null terminate strings: Given how many libraries expect strings to be null-terminated, it might just be worth it to have strings null terminated in library.
I am personally leaning towards the allocate once resize later option as the most sensible approach that reduces weight on the user. However, I do also think ensuring null terminated strings may be something to consider in the future of this library.
I am new to this library so this may make no sense. Why not just pass in a pointer to free memory and if the pointer is NULL use Malloc instead?
The same applies to the SDL backend, where this issue is worse because SDL allocates a texture and surface.
I am new to this library so this may make no sense. Why not just pass in a pointer to free memory and if the pointer is NULL use Malloc instead?
@KellanHiggins I believe this string is being allocated because clay's string does not have a null terminator. To solve this, each time a string needs to be sent to Raylib, which expects a null terminator (IMO Raylib should also include a function that asks for the length), Clay allocates a new string with the original string's length + 1 to have room for the null terminator.
I think a buffer pointer and a buffer length initialized to zero should be outside the loop. If the string switch statement hits a NULL pointer it mallocs enough to cover that string. Each string copy check the length to confirm the buffer is big enough. If not big enough it frees the old pointer, mallocs a new buffer and continues on. No need to use realloc because the memory doesn't have to transfer.
A downside would be now the loop needs to do in order and the compiler can't do the whole loop in parallel.
And that buffer could get so huge that it would take up an insane amount of memory.
@KellanHiggins you'd need a good default size for the buffer, IMO the solution would be to add a null terminator to the Clay string.
...Again, it'd be better if Raylib would support text without a null terminator, but I don't think Raysan is interested in adding that feature.
Thanks for opening this PR @Gota7 !
For some clarity here, the reason that Clay_String isn't guaranteed to have a null terminator is because it's actually a string view. The best example is if we take a large body of wrapped text:
If we were to emit null terminated strings, clay would be forced to memory clone every line of wrapped text from the original long text input and append a null terminator. This not only costs time to actually make the copy, but it doubles the internal memory required to represent text in Clay. By using a string view, we basically just say "there is text at this address with this length", which is a view into the original string that was passed to clay.
It provides much performance in terms of cache locality for renderers that do support strings with length - it just happens that raylib is not one of those 🙂
@ColleagueRiley There is already a malloc being used in this so instead of constantly heap allocating and destroying, keep a single buffer area and free and malloc as is needed. Should speed up the array building as the array no longer needs to malloc a bunch of stuff.
I think removing null terminators is a good idea, no reason to waste a ton of memory since null terminated strings are a memory security nightmare.
@nicbarker Is there a reason you don't want to pass a buffer into this rendering to handle adding null terminators to the string?
void Clay_Raylib_Render_With_Buffer(Clay_RenderCommandArray renderCommands, char *bufferPtr, int bufferLength )
If the buffer ptr is null or the size isn't large enough malloc this frame. This would eliminate all heap allocations during the render draw call. Thus sweet sweet performance.
I made #298, It just holds onto the memory that was allocated and resizes the memory if the string is longer then it has room for.
