Reduce memory consumption for soundpacks

[OrenAudeles]

Summary

SUMMARY: Performance "Reduce memory consumption for soundpacks"

Purpose of change

Large soundpacks (see https://discourse.cataclysmdda.org/t/cdda-soundpack/15329 ) can inflate memory consumption by reloading the same sound effect resource multiple times.

Describe the solution

Track the paths and loaded sound resource Mix_Chunks to ensure each sound resource is loaded exactly once. When attempting to load a path first check to see if the path has already been loaded once. If the path has already been loaded, allocate a Mix_Chunk object as a non-allocating copy of the original resource data. If the path has not been loaded, attempt to load it. If successful store the pointer using the resource path, and return the result pointer.
Allocating a new Mix_Chunk as a non-allocating copy allows existing lifetime management of sound_effect objects to remain unchanged, Mix_FreeChunk will not attempt to deallocate the data pointer for non-allocating copies.

By doing this memory consumption is reduced, with reduction becoming more apparent with larger soundpacks with many reused sound resources.

Additional context

In the linked Soundpack there are 2607 sound effect variant paths loaded, 398 unique paths.
From debug.log instrumentation: Count[# of allocated resources] Bytes[# of bytes allocated total]
Existing Implementation: Sound Effects: Count[2607] Bytes[821,454,648]
PR's Implementation: Sound Effects: Count[398] Bytes[173,757,452]

[alanbrady]

I think I understand the gist what's being accomplished here and that's trying to avoid loading the same wav file multiple times for different sounds. Seems like a good idea, but this has problems I think. Seems good, more data to back it up wouldn't hurt.

[alanbrady]

I think I understand the gist what's being accomplished here and that's trying to avoid loading the same wav file multiple times for different sounds. Seems like a good idea, but this has problems I think. Seems good, more data to back it up wouldn't hurt.

[alanbrady]

I think this should be static

[OrenAudeles]

Sure, won't hurt to make it static. I don't think it's strictly necessary to do so (see other file-level global declarations within sdltiles.cpp) but I think it will prevent other files from externing it.

[alanbrady]

Yeah I mean it's actually a nitpick, should have labelled it so. Thanks!

[alanbrady]

I think I'm confused here. You're doing an SDL_malloc to match up with the SDL_free, which sounds right, but then setting nchunk->allocated to 0 so it doesn't free it? I think one of these is wrong.

Now I think about it, doing the SDL_malloc still isn't saving any memory allocs, how is this saving memory? In your test instrumentation your only checking if allocated is null which you set it to. This isn't actually profiling memory, I would like to see some better instrumentation from a tool that was built to do this like valgrind.

[alanbrady]

Ah got it so the SDL_malloc is getting memory for the struct but the allocated field is telling it the abuf is already allocated by someone else, so don't free it with the struct. I understand now. Nevermind I think this is probably good. Some valgrind data to backup your instrumentation would still be nice.

[OrenAudeles]

Instrumentation was testing against allocated so that it didn't count the same memory region multiple times. I should have explained better, either in the PR description or the comments, how Mix_FreeChunk works wrt to allocated.

[alanbrady]

Don't we only need the one unique_chunks.clear() at the end?

[OrenAudeles]

Yes, will remove. I think I put that there then decided to put it at the end because it made more sense, then forgot to remove the first.

[alanbrady]

Thanks. Technically Mix_Chunk is a struct not object but that's bordering on pedantic.

[alanbrady]

Thanks. Technically Mix_Chunk is a struct not object but that's bordering on pedantic.