utils: disable object validity check in release builds

[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 CMake option to enable/disable validity checks. It is enabled by default in Debug builds and disabled otherwise. There's a problem that default value doesn't get updated when changing build type in existing build directory, I don't know how to fix it.

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:

https://github.com/libsdl-org/SDL/blob/2bc3ec44b1b41fdd41b97d17afa16754579c1502/src/render/SDL_render.c#L50-L67

With the introduction of SDL_ObjectValid in b0e93e4e63e825329e6b10072d172f808d7fcd9b 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 7570ab106da5888803bb4269144c0a687f849614.

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.

[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.

[mechakotik]

@slouken @icculus what's your opinion on this?

[slouken]

I still want to compile out the validity checks entirely if someone builds with a flag to do so, and possibly disable parameter validation entirely, so I'm not sure whether this is the approach we want to take.

[mechakotik]

Do you want validity check to be enabled by default? If so, it is reasonable to support disabling them in runtime, because such environments as Linux distros and Steam Runtime use system-wide shared SDL library, and it's often painful to replace it with self-built version.

Checking hint value should have almost zero overhead, because checking value of non-changing variable can be easily handled by branch predictor. If this is not true, it's easy to support both compile- and run-time control of validity checks.