Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SDL_malloc num_allocations tracking should be optional #11099

Closed
kg opened this issue Oct 7, 2024 · 4 comments · Fixed by #11681
Closed

SDL_malloc num_allocations tracking should be optional #11099

kg opened this issue Oct 7, 2024 · 4 comments · Fixed by #11681
Assignees
Milestone

Comments

@kg
Copy link
Contributor

kg commented Oct 7, 2024

While kicking the tires on SDL_GPU, I noticed that SDL_AddAtomicInt_REAL was near the top of my profiles (7th, to be precise). Most of the calls to this function are from SDL_malloc, where it tracks the amount of memory allocated. This probably indicates that SDL_GPU has high malloc traffic (and it does), but it also seems like it would be an easy small win for performance-sensitive users to not do these atomics on every malloc/free operation, or find a way to make them cheaper. Alternately, you could make them optional so that people like me can turn them off to squeeze out extra performance on underpowered targets like Steam Deck.

To provide concrete numbers, SDL_AddAtomicInt_REAL was 527ms of exclusive execution time in a ~75000ms long capture, so around 0.7% of execution time.

@AntTheAlchemist
Copy link
Contributor

There are a few areas where things like this could be filtered out at build-time. I'd be interested in using it.

In the mean time, is SDL_SetMemoryFunctions() any use?

@kg
Copy link
Contributor Author

kg commented Oct 7, 2024

There are a few areas where things like this could be filtered out at build-time. I'd be interested in using it.

In the mean time, is SDL_SetMemoryFunctions() any use?

If I understand the code correctly, the atomics are outside the memory function, so it wouldn't help:

SDL/src/stdlib/SDL_malloc.c

Lines 6431 to 6436 in 9f17028

mem = s_mem.malloc_func(size);
if (mem) {
SDL_AtomicIncRef(&s_mem.num_allocations);
} else {
SDL_OutOfMemory();
}

The atomics could be made optional by providing a memory function that does recording and one that doesn't, perhaps.

@AntTheAlchemist
Copy link
Contributor

I see how SDL_SetMemoryFunctions() works now, so yes, that's no help.

I hope this gets looked at when considered for disabling mentioned here

@slouken slouken added this to the 3.2.0 milestone Oct 7, 2024
@slouken slouken self-assigned this Dec 4, 2024
@icculus
Copy link
Collaborator

icculus commented Dec 4, 2024

We're going to add an #ifdef around these atomics, so if we ever want to build something that checks for allocation counts, we can, but default it to off. All these are doing is incrementing, so it would need a debugger attached to be useful in any case, atm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants