utils: add hint to enable/disable object validity checks

[mechakotik]

These checks seem to affect performance, particularly they take a huge part of SDL_RenderCopy time:

44.54% SDL_RenderTextureRotated
   - 44.48% SDL_RenderTextureRotated_REAL
      - 43.57% SDL_RenderTexture_REAL
         + 20.38% SDL_RenderTextureInternal
         + 19.42% SDL_ObjectValid
         + 1.65% SDL_GetRectIntersectionFloat_REAL

This perf report is taken in debug build, as in release everything gets optimized out and perf doesn't report anything meaningful. So I've also made a simple RenderCopy benchmark:

Source

#include <SDL3/SDL.h>
#include <SDL3_image/SDL_image.h>
#include <random>

int main() {
    SDL_Init(SDL_INIT_VIDEO);
    SDL_Window* window = SDL_CreateWindow("window", 1280, 720, SDL_WINDOW_RESIZABLE);
    SDL_Renderer* renderer = SDL_CreateRenderer(window, nullptr);
    SDL_Surface* surface = IMG_Load("image.png");
    SDL_Texture* texture = SDL_CreateTextureFromSurface(renderer, surface);
    SDL_SetRenderVSync(renderer, SDL_RENDERER_VSYNC_DISABLED);

    SDL_FRect src, dst;
    src.x = src.y = 0;
    src.w = src.h = 8;
    dst.w = 32;
    dst.h = 32;

    std::mt19937 gen(42);

    for(int it = 0; it < 500000; it++) {
        SDL_RenderClear(renderer);
        for(int i = 0; i < 500; i++) {
            dst.x = gen() % 1280;
            dst.y = gen() % 720;
            SDL_RenderTexture(renderer, texture, &src, &dst);
        }
        SDL_RenderPresent(renderer);
    }

    SDL_DestroyTexture(texture);
    SDL_DestroyRenderer(renderer);
    SDL_DestroyWindow(window);
    SDL_Quit();
}

image.png:

Ran on Ryzen 9 7940HS, Gentoo Linux, performance governor. There is a noticeable performance difference (and it becomes more noticeable when increasing number of draws in single frame).

# with validity checks
Executed in   39.40 secs    fish           external
   usr time   26.64 secs    0.21 millis   26.64 secs
   sys time   10.08 secs    2.90 millis   10.08 secs

# without validity checks
Executed in   33.71 secs    fish           external
   usr time   22.34 secs    0.12 millis   22.34 secs
   sys time    9.70 secs    1.02 millis    9.70 secs

I've added a hint SDL_HINT_CHECK_OBJECT_VALIDITY to enable/disable validity checks at runtime.

P.S. Are these checks needed at all when there's AddressSanitizer to detect use-after-free efficiently?

[mechakotik]

Okay, looks like ObjectValid is used not only to detect use-after-free and can't be just disabled everywhere. What's the actual purpose of it?

[bjorn]

The related code looked rather more efficient for SDL2, where it was just a nullptr and magic field check:

SDL/src/render/SDL_render.c

Lines 50 to 67 in 2bc3ec4

	#define CHECK_RENDERER_MAGIC_BUT_NOT_DESTROYED_FLAG(renderer, retval) \
	if (!renderer || renderer->magic != &renderer_magic) { \
	SDL_InvalidParamError("renderer"); \
	return retval; \
	}
	#define CHECK_RENDERER_MAGIC(renderer, retval) \
	CHECK_RENDERER_MAGIC_BUT_NOT_DESTROYED_FLAG(renderer, retval); \
	if (renderer->destroyed) { \
	SDL_SetError("Renderer's window has been destroyed, can't use further"); \
	return retval; \
	}
	#define CHECK_TEXTURE_MAGIC(texture, retval) \
	if (!texture || texture->magic != &texture_magic) { \
	SDL_InvalidParamError("texture"); \
	return retval; \
	}

With the introduction of SDL_ObjectValid in b0e93e4 I guess this check has gotten more expensive. Personally I'd rather have my application crash than silently ignoring bugs in my code, but apparently some consider it more helpful to continue in a kind of best-effort mode. A way to disable these checks was considered out of scope for SDL3 (see #9235).

I think it's great that you're trying to introduce an option to disable the checks. What issue did you actually run into, though? If it's just about the CI build error, that's only because you have left in some static functions which are now unused, causing compiler warnings, which are set to errors:

/__w/SDL/SDL/src/SDL_utils.c:146:13: error: 'SDL_KeyMatchObject' defined but not used [-Werror=unused-function]
  146 | static bool SDL_KeyMatchObject(void *unused, const void *a, const void *b)
      |             ^~~~~~~~~~~~~~~~~~
/__w/SDL/SDL/src/SDL_utils.c:141:23: error: 'SDL_HashObject' defined but not used [-Werror=unused-function]
  141 | static Uint32 SDLCALL SDL_HashObject(void *unused, const void *key)
      |                       ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors

So just extend your patch to wrap also these functions in #ifdef SDL_OBJECT_VALIDITY_CHECK.

P.S. Are these checks needed at all when there's AddressSanitizer to detect use-after-free efficiently?

In this case, I think these checks are even harmful! Because now instead of being able to rely on AddressSanitizer to detect issues in our code, we would need to litter our code with manual checks on the return values of all the SDL API calls, and even then we can only guess at what caused the actual issue if things fail. :-(

[bjorn]

Btw, did you consider disabling these checks based on a hint (see SDL_SetHint)? That should make the speedup accessible to a lot more people, assuming the hint can be checked very fast (could probably be read only once). At least personally I tend to rely on system packages.

[mechakotik]

What issue did you actually run into, though?

I don't really understand the purpose of ObjectValid. At first glance it's just optional validity check that can be disabled safely, but looking at e.g. SDL_tray_utils.c it's used in more complex logic of cleaning up trays. I doubt it can be just disabled without breaking things.

[bjorn]

but looking at e.g. SDL_tray_utils.c it's used in more complex logic of cleaning up trays. I doubt it can be just disabled without breaking things.

Hmm, indeed, this SDL_GetObjects usage means such objects will at least need to be added to the hash table. It's actually the only usage though, with the function having been explicitly added for this purpose in 7570ab1.

Since you're mainly worried about the performance of SDL_ObjectValid though, you could adjust your patch to disable the expensive check in that function but leave SDL_SetObjectValid alone. Then SDL_GetObjects should keep working for the tray objects.

[mechakotik]

Looked into it again and it seems like SDL_CleanupTrays is not required and is a protection in case user forgets to call SDL_DestroyTray in the end.

Btw, did you consider disabling these checks based on a hint (see SDL_SetHint)? That should make the speedup accessible to a lot more people, assuming the hint can be checked very fast (could probably be read only once). At least personally I tend to rely on system packages.

That's reasonable, thanks for the idea!

[bjorn]

it seems like SDL_CleanupTrays is not required and is a protection in case user forgets to call SDL_DestroyTray in the end.

In that case this sounds like another case of hand-holding which makes life harder for people who're checking their code with a memory leak detector. It may then be acceptable that this cleanup no longer works when object validity checks are disabled, though it does seem like something that should be mentioned in the documentation of the hint.

[bjorn]

In my personal opinion, it would be alright to always initialize the hash table, but just not put any objects in it. This would rather simplify the patch because:

• We'd need only a single check on check_validity in this function (the one below, which I think is better written as early-out rather than nesting the insert/remove code).
• We would not need to check the value of check_validity in SDL_GetObjects or SDL_SetObjectsInvalid at all. These rarely used functions would automatically not do anything with an empty hash map.

The empty hash table should have basically zero overhead anyway.

[mechakotik]

There's actually a massive overhead. It needs to lock and unlock hashtable's RWLock (which is like a half of the lookup time), calculate hash and do one iteration in find_first_item which will break because of empty hashtable.

[bjorn]

It seems you're talking about SDL_ObjectValid. I wasn't suggesting to remove the check_validity condition from there.

[AntTheAlchemist]

I'm also not a fan of these validity checks. Imagine rendering 1000 textures of different colours every frame. That's 120,000 validity checks a second on the same renderer object that you already know is valid at the start of the app.

I remember @slouken saying they already had something in mind to optionally disable this at some point.

I'd like for it to be all hidden behind an #ifdef, or to only be included in the debug build.