Replace expensive seat_get_config() functions with pointer #8269
Conversation
|
I believe this will break if the seat config changes during runtime. (e.g. you reload the config file) The old configs will be freed and replaced with the new ones. This code will continue to reference the stale values. |
Just tested it. This is not the case. Changing seat configs works without having to restart Sway :) |
iguanajuice
force-pushed
the
remove_stuff
branch
from
6465225
to
7e4a5c0
Compare
|
Update: I have been daily driving this patch for 3 weeks now and nothing is out of the ordinary. I think this patch is well tested and would like to see this get merged, thanks. |
|
Gentle ping. |
There was a problem hiding this comment.
I'm still not convinced that this PR is handling lifetimes properly. There are a couple calls to free_seat_config that won't end up nulling or recalculating the pointer that the actual seats have meaning they will start having invalid references.
It might make sense to add a destroy style signal to a seat config, and then have seats listen to this destroy signal, then do the appropriate recalculation then.
Also, please make sure you do a rebase instead of a merge. The commits should carry meaning on its own (see atomic git commits). This should all be squashed.
sway/input/input-manager.c
Outdated
| struct seat_config *seat_config = seat_get_config(seat); | ||
| struct seat_config *seat_config = NULL; | ||
| struct seat_config *sc = NULL; | ||
| for (int i = 0; i < config->seat_configs->length; ++i ) { | ||
| sc = config->seat_configs->items[i]; | ||
| if (strcmp(seat->wlr_seat->name, sc->name) == 0) { | ||
| seat_config = sc; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are we iterating the seat configs here? I thought that the seat->config should be used now.
There was a problem hiding this comment.
Because input_manager_verify_fallback_seat() runs before seat_apply_config() can set the pointer (according to a backtrace).
Attempting to use the pointer now will result in undefined behavior and a "soft crash", hence this is the only time we need to iterate of the seats by name.
I recant this, sway doesn't crash but instead no default (fallback) seat is created and thus no user input.
iguanajuice
force-pushed
the
remove_stuff
branch
from
f410ed3
to
1bbca68
Compare
|
Update: A bug has finally been found. Setting |
First time contributor to Sway here. After studying Sway's codebase for a bit, due to wanting to implement something unrelated, I noticed that the function
seat_get_config()(and it's sister functionseat_get_config_by_name()) are both needlessly expensive and ultimately unneeded.Currently, Sway finds the config related to a seat by looping through all the seats and strcmp'ing their names until a match is found. This is obviously not very efficient.
My change simply replaces these functions with a pointer in
struct sway_seat.Again, first time contributor, so I'm not super familiar with Sway's codebase, but I have tested the following with my patch:
*) seat configurationSo far, no crashes or unexpected behaviour; but I do believe more rigorous testing is needed. I am daily driving this patch.