config/output: Store output configs sequentially

[kennylevinsen]

The current output configuration system merges configurations such that only one configuration per match is stored. This is a simplification of a previous design and has the benefit that a minimal number of output configurations are retained, but to correctly merge and supersede configurations it is required that an identifier can be resolved from a connector, which leads to differences in how output configurations are interpreted based on whether a display is currently connected or not.

Instead, append all new output configurations to the list as they are made, and remove old output configurations once all their settings have been overwritten by newer configurations.

Based on ideas from #5629

Fixes: #5632

[pedrocr]

I haven't looked at the code but since I was CCed I'll just give one comment on the approach described. Unbounded growth in output configs is at best a theoretical concern. Output configs are very small. The small amount of code I added to do that was just to avoid even theoretical pathological cases. That's why it's so simple, it's more than enough to prevent any meaningful memory usage. Meanwhile the merge code was complex and had at least one bug, while also making it very hard to add features such as glob patterns.

Adding complexity to better reduce memory usage is unlikely to be a good tradeoff. The PR languished without a response but not for that problem not being fixed.

[kennylevinsen]

I wrote this draft PR just as a quick experiment to see how a solution would work, and as it was entirely trivial I don't see any reason to consider allowing any unbounded growth.

Btw, I consider the general idea yours, and do prefer proper attribution, whether you want to pick up your old PR with this as input, want a particular tag on commits here, etc.

Output configs are very small.

Output configs reference things like color transforms which are currently roughly half a megabyte each, and without cleanup these would always leak - someone toggling between the upcoming HDR feature to view HDR content and an ICC profile (which are mutually exclusive due to ICC profile limitations) to have pleasant and/or color accurate SDR content would hit this.

On the smaller side, someone scripting background image changes every five minutes, either for aesthetics or to minimize OLED burn-in, would easily accumulate thousands of output configurations per month. Even just having your machine go idle on average 20 times a day would accumulate over a thousand output configurations a month, and kanshi would just add to the list.

Adding complexity to better reduce memory usage is unlikely to be a good tradeoff.

If I removed all memory deallocations in sway it's unlikely I'll ever experience an out of memory condition from it, and yet we would always consider a missing deallocation a bug no matter how small the leak is. Allowing a working dataset to grow unbounded is worse than a missing free.

Meanwhile the merge code was complex

Note that the output configuration merge code was rewritten and massively simplified some time back, so while it still has the same known issues, it is now quite simple.

[pedrocr]

Feel free to use as much or as little code as is helpful. Don't worry about attribution at all. I stopped working on sway when I finally figured out the kinds of things I was interested in weren't welcome, so I just kept my local fork for the things I wanted. But I left the PR there in case anyone wanted to use it.

As for memory growth, the original PR is not optimal but it's not unbounded either. The code throws away all the old output configs that have been replaced by a new one that touches the same set of things. I calculated it at the time and it was something like 1MB per output in the absolutely pathological case. If the output config can now be much larger that limit has grown but it's still a theoretical case and is definitely not unbounded.

someone toggling between the upcoming HDR feature to view HDR content and an ICC profile (which are mutually exclusive due to ICC profile limitations) to have pleasant and/or color accurate SDR content would hit this.

On the smaller side, someone scripting background image changes every five minutes, either for aesthetics or to minimize OLED burn-in, would easily accumulate thousands of output configurations per month.

Neither of these would be a problem. Since they're the same output lines touching the same thing but superseding them the code already does a delete of the old line on insert of the new one:

https://github.com/swaywm/sway/pull/5629/files#diff-66d6f9399c138857218e684475e604f3710684dd9deb896b66bf0ec2cacd149fR201-R210

To actually have a pathological case with the original code you need to make output commands that each do a different combination of changes between all the options so that none of them directly supersede the others. That's where merging does compress things and the supersedure comparison does not. And even that is not unbounded and basically requires some kind of adversarial interaction to happen. And if that's what you're defending against someone can just spam output lines with different output names that can't be merged anyway.

Maybe you're thinking of the exact compare on the swaybg_cmd logic. That's only there to not cause flicker when the background command hasn't changed at all:

https://github.com/swaywm/sway/pull/5629/files#diff-66d6f9399c138857218e684475e604f3710684dd9deb896b66bf0ec2cacd149fR597-R602

[kennylevinsen]

I wasn't arguing that your PR didn't contain any consideration to mitigate unbounded growth to avoid the pathological examples above, I was just answering that unbounded growth from an append-only approach is not a theoretical problem and is indeed worth addressing (as you also did to an extent).

The approach here is conceptually similar, just implemented as a very minor change to the current output configuration system and generalized to any field combination instead of only exact matches.

[kennylevinsen]

Still depends on #8687

[pedrocr]

There was no chance of unbounded growth in the PR and the examples you gave had no growth at all.

There's a simple design to make it optimal without any merge logic. Split up internally every output command into multiple appended commands to the list each with one individual change. Then the same replacement logic would be enough to be as good as doing merging as any time the same setting gets changed in two places only one gets saved. That may even be slightly less code and also very straightforward.

[kennylevinsen]

There's a simple design to make it optimal without any merge logic.

This PR doesn't rely on merge logic (other than when reading the config, but that's also true for your old PR), it just appends new configs like your PR.

Old matching configs just have fields cleared as they are set by new configs (what supersede_output_config does), and once they have no more fields set the config is removed - this is different than your approach, but handles more cases with roughly the same (maybe slightly fewer?) lines of code.

[kennylevinsen]

I calculated it at the time and it was something like 1MB per output in the absolutely pathological case

Not that it matters, but the upper bound of that PR is 2**21 - 1 output configs per config name (wildcard, connector or identifier), each of which is 184 bytes. That's 368 MiB in just the structs, and if the subset that had color transforms set used ICC profile color transforms, that's another 400 GB per config name.

(This PR has an upper bound of 21 output configs per config name, and each field is stored only once per config name, so all in all just the half meg from color transform.)

[pedrocr]

Not that it matters, but the upper bound of that PR is 2**21 - 1

That PR only had 17 bits, so I assume you are calculating it with new things that happened since. But that's not correct either because the interface doesn't allow you to fill in the struct with anything you want. You're limited to the combinations of settings that are possible with output commands which are very few.

Either way, like we discussed on IRC this is just quire bike sheddy. I've run this code for years now with no issues and any other version would be fine too. Glob matching is very useful and it's too bad it didn't get done.

[kennylevinsen]

But that's not correct either because the interface doesn't allow you to fill in the struct with anything you want. You're limited to the combinations of settings that are possible with output commands which are very few.

Note that all output subcommands within an output XYZ { ... } block accumulate their changes to a single struct output_config that is stored at the end of the output command block, so all combinations are valid and possible.

[pedrocr]

Not all are possible still because you can't set width without height and stuff like that. But I didn't know it was accumulating. Can you even exercise that bracket syntax from a script? If that was the problem that part of the parsing could be done without accumulation to get back to very few combinations.

This is not a real problem with getting glob matching to work, there are plenty of ways to do it and there were never any actual objections left to the original PR, the discussion just petered out.