Proliferation & performance of SDL_properties

[willvale]

I noticed that some SDL3 API areas (e.g. IO, processes) make quite heavy use of the relatively new SDL_properties mechanism. Since properties are heap allocted, multi-thread safe and use fine grained locking this is causing a lot of memory allocations and mutex locks vs. a more traditional implementation.

E.g. if I call SDL_IOFromFile, it performs these operations before returning, plus whatever else is involved in the hashtable manipulation (which I didn't instrument).

SDL_malloc
SDL_malloc
SDL_LockMutex
SDL_LockMutex
SDL_free
SDL_LockMutex
SDL_free
SDL_calloc
SDL_malloc
SDL_calloc
SDL_calloc
SDL_calloc
SDL_calloc
SDL_calloc
SDL_calloc
SDL_LockMutex
SDL_malloc

Obviously opening files is a relatively heavyweight operation, but this still feels excessive to me for something which always happens within a single API call on a single thread. (NB: This is using custom heap on my side, and I (after the edit!) didn't instrument the locks required to synchronise access to that, so these should all be from the SDL side.)

I can see that a bag of properties is nice for one-time things like device configuration, and solves some problems neatly, but it would be good if it stayed away from things which happen more than once or twice in a game session.

E.g. for IO, having a fixed-size "file system handle" space a la SDL2 would solve this with much lower overhead. And for SDL_CreateProcessWithProperties, an optional parameters structure could do the same thing?

Thanks for listening!

[willvale]

For context, using some properties with SDL_CreateProcessWithProperties needs the following

	SDL_PropertiesID props = SDL_CreateProperties();
	SDL_SetPointerProperty(props, SDL_PROP_PROCESS_CREATE_ARGS_POINTER, params.begin());
	SDL_SetNumberProperty(props, SDL_PROP_PROCESS_CREATE_STDIN_NUMBER, SDL_PROCESS_STDIO_NULL);
	SDL_SetNumberProperty(props, SDL_PROP_PROCESS_CREATE_STDOUT_NUMBER, SDL_PROCESS_STDIO_NULL);
	SDL_SetNumberProperty(props, SDL_PROP_PROCESS_CREATE_STDERR_NUMBER, SDL_PROCESS_STDIO_NULL);
	SDL_SetBooleanProperty(props, SDL_PROP_PROCESS_CREATE_BACKGROUND_BOOLEAN, true);

	// Create process
	Process *result = nullptr;
	if (SDL_Process *handle = SDL_CreateProcessWithProperties(props))

SDL_calloc
SDL_calloc
SDL_calloc
SDL_calloc
SDL_calloc
SDL_LockMutex
SDL_malloc
SDL_calloc
SDL_LockMutex
SDL_malloc
SDL_calloc
SDL_LockMutex
SDL_malloc
SDL_calloc
SDL_LockMutex
SDL_malloc
SDL_calloc
SDL_LockMutex
SDL_free
SDL_calloc
SDL_LockMutex
SDL_malloc
SDL_LockMutex
SDL_LockMutex
SDL_calloc
SDL_LockMutex
SDL_calloc
SDL_calloc
SDL_calloc
SDL_calloc
SDL_calloc
SDL_LockMutex
SDL_malloc
SDL_LockMutex
SDL_LockMutex
SDL_LockMutex
SDL_LockMutex
SDL_LockMutex
SDL_LockMutex
SDL_LockMutex
SDL_LockMutex
SDL_LockMutex
SDL_malloc
SDL_calloc
SDL_malloc
SDL_malloc
SDL_malloc
SDL_LockMutex
SDL_LockMutex
SDL_free
SDL_LockMutex
SDL_free
SDL_malloc
SDL_malloc
SDL_malloc
SDL_LockMutex
SDL_LockMutex
SDL_free
SDL_LockMutex
SDL_free
SDL_calloc
SDL_LockMutex
SDL_malloc
SDL_LockMutex
SDL_free
SDL_LockMutex
SDL_free
SDL_LockMutex
SDL_free

[slouken]

I'm tossing this to @icculus on this milestone for consideration, but this might bump to 3.6.

[willvale]

Thanks! No rush from my end.

I haven't contributed anything here before but if there's a way I can help out I'd be happy to do that.

[icculus]

I need to dig into the implementation to see if there are obvious wins we can pick up.

I suspect if nothing else, it could be worth dirtying up some internal code so things like IOFromFile, etc, doesn't have to go through a properties set...instead we build out a stack-allocated struct that everything funnels through instead of building out an SDL_Properties.

But also: most places we use properties are expected to be a little heavyweight...if you think managing properties to call SDL_CreateProcessWithProperties is expensive, wait until you see what creating a process costs! :)

Jokes aside, improving this would be nice, so I'll take a look.

[icculus]

Hmm, maybe we add an API:

const SDL_PropertiesKeyValuePair keys_and_vals[] = {
    // last field would probably be a void*, to hold pointers, numbers, and bools, and likely would need an explicit cast.
    // maybe just explicitly Uint64? I don't know.
    { SDL_PROP_TYPE_POINTER, SDL_PROP_PROCESS_CREATE_ARGS_POINTER, params.begin(); },
    { SDL_PROP_TYPE_NUMBER, SDL_PROP_PROCESS_CREATE_STDIN_NUMBER, SDL_PROCESS_STDIO_NULL },
    { SDL_PROP_TYPE_NUMBER, SDL_PROP_PROCESS_CREATE_STDOUT_NUMBER, SDL_PROCESS_STDIO_NULL },
    { SDL_PROP_TYPE_NUMBER, SDL_PROP_PROCESS_CREATE_STDERR_NUMBER, SDL_PROCESS_STDIO_NULL },
    { SDL_PROP_TYPE_BOOLEAN, SDL_PROP_PROCESS_CREATE_BACKGROUND_BOOLEAN, true }
};

SDL_PropertiesID props = SDL_CreateStaticProperties(keys_and_vals[], SDL_arraysize(keys_and_vals));

// Create process
Process *result = nullptr;
if (SDL_Process *handle = SDL_CreateProcessWithProperties(props))

A static properties creates no hashtable, and expects keys_and_vals to remain valid while it exists. It will not allow changes to the property set, and lookups are a simple iteration over keys until we find it, under the assumption most of these would have <= 5 items. This would still have mallocs and locks to manage an SDL_PropertiesID, but dramatically less.

This might be a terrible idea, but it would solve the common use case of "I made an SDL_Properties with 2 items to call SDL_CreateWidgetWithProperties() and then immediately destroyed it".

[willvale]

That's an interesting idea, and would definitely be more efficient while still staying within the property bag API - assuming that reading from the static properties is reasonably efficient.

I'm just not sure if the properties are buying much more than a parameter struct in these relatively simple cases - that would make more more compact code. I guess the dirtying you're referring to is having e.g. platform specific properties leak up into generic code?

IMO the most important thing to look at is the internal properties in IO - that's likely the highest-frequency thing.

[madebr]

I think we don't have to worry about a properties object being destroyed from under you while it is in use: SDL borrows it during the Create and Get function calls.

Would a SDL_MakePropertiesReadOnly(SDLPropertiesID) call also work? This wiuld be a one-way operation. As an owner of the object, you can only destroy the object after that call.

This would keep the performance identical for bigger maps, and would only make SetProperty calls fail.

[icculus]

I took a run at my idea; if this turns out to be a terrible idea, no foul at just closing the PR unmerged, though.

[icculus]

Would a SDL_MakePropertiesReadOnly(SDLPropertiesID) call also work?

It could save on locking once the data is ready, but there's still a giant flurry of malloc and LockMutex calls to build up the properties in the first place.

[slouken]

Bumping this to 3.6

[icculus]

One last note before I move on to other 3.4.0 stuff: I just realized that SDL_IOFromFile isn't a thing that funnels into a generic SDL_CreateIOStreamWithProperties() function. There the problem is that it creates an internal properties set for implementation-specific stuff (the FILE*, win32 HANDLE, etc), in case the caller wants it.

So we were talking about different things here. That would probably be solvable with a hint to say "don't set properties on this SDL_IOStream at creation time, I don't want them," which is likely the common use-case, too.

[willvale]

@icculus that last thing would be perfect for me, ideally as an opt-in "please give me properties" to suit the common case. Thanks!

I'll have a go with CreateTemporaryProperties and see how that works for the process example.

[kennethrapp]

Would it be feasible to have an option to preallocate space for the hashmap, similar to how tidwall/hashmap.c works?

[icculus]

Right now we grow as necessary (preallocating 4 buckets), which is probably the right thing to do for SDL_Properties, since they tend to only have a handful of items anyhow...but our underlying hashtable interface already does allow preallocation of arbitrary sizes at creation time, fwiw.