dnkl/foot
41
2.0k
Fork
You've already forked foot
243

Implement damage rectangle optimization #2340

Closed
lilyyy411 wants to merge 2 commits from lilyyy411/foot:master into master
pull from: lilyyy411/foot:master
merge into: dnkl:master
dnkl:master
dnkl:osc-5522
dnkl:sixel-heap-buffer-overflow
dnkl:releases/1.27
dnkl:releases/1.26
dnkl:releases/1.25
dnkl:releases/1.24
dnkl:multi-cursor
dnkl:releases/1.23
dnkl:pixman-16f-2
dnkl:releases/1.22
dnkl:releases/1.21
dnkl:releases/1.20
dnkl:releases/1.19
dnkl:releases/1.18
dnkl:releases/1.17
dnkl:releases/1.16
dnkl:releases/1.15
dnkl:releases/1.14
dnkl:releases/1.13
dnkl:releases/1.12
dnkl:releases/1.11
dnkl:releases/1.10
dnkl:releases/1.9
dnkl:releases/1.8
dnkl:releases/1.7
dnkl:releases/1.6
dnkl:releases/1.5
dnkl:releases/1.4
dnkl:releases/1.3
dnkl:releases/1.2
dnkl:releases/1.1
dnkl:releases/1.0
First-time contributor
Copy link

This PR fixes #2338 and implements basic damage rectangle optimization leveraging the fact that pixman does a pretty good chunk of the work already. This is a somewhat naive implementation that could theoretically be more fine-tuned and optimized with custom data structures, but honestly, it's good enough for any practical purposes. Heck, even not doing any additional optimization was already good enough for practical purposes.

The algorithm used here is quite simple: it just collects all the damage for each row, consolidates the row's damage into some number of rectangles (to reduce the number of small "bad" rectangles
that would otherwise cause frequent and bad merges) and then adds the new rectangle to the worker's damage region. If the number of rectangles in the damage buffer exceeds some per-worker limit, then it merges each rectangle in the worker damage buffer with its neighbor.

As a bonus, benchmarks on my machine show substantial improvements for most of vtebench's scrolling benchmarks at high numbers of lines/cols and minimal effect on small resolutions.

This PR fixes #2338 and implements basic damage rectangle optimization leveraging the fact that pixman does a pretty good chunk of the work already. This is a somewhat naive implementation that could theoretically be more fine-tuned and optimized with custom data structures, but honestly, it's good enough for any practical purposes. Heck, even not doing any additional optimization was already good enough for practical purposes. The algorithm used here is quite simple: it just collects all the damage for each row, consolidates the row's damage into some number of rectangles (to reduce the number of small "bad" rectangles that would otherwise cause frequent and bad merges) and then adds the new rectangle to the worker's damage region. If the number of rectangles in the damage buffer exceeds some per-worker limit, then it merges each rectangle in the worker damage buffer with its neighbor. As a bonus, benchmarks on my machine show substantial improvements for most of vtebench's scrolling benchmarks at high numbers of lines/cols and minimal effect on small resolutions.
Fix formatting
Some checks are pending
builds.sr.ht/freebsd-x64 Job completed
ci/woodpecker/pr/woodpecker Pipeline was successful
ci/woodpecker/pull_request_closed/woodpecker Pipeline is pending approval
5317300015
Owner
Copy link

As a bonus, benchmarks on my machine show substantial improvements for most of vtebench's scrolling benchmarks at high numbers of lines/cols and minimal effect on small resolutions.

Since you seem to have it all setup, can you post the benchmark results here, for a "normal" resolution, as well as high resolution.

> As a bonus, benchmarks on my machine show substantial improvements for most of vtebench's scrolling benchmarks at high numbers of lines/cols and minimal effect on small resolutions. Since you seem to have it all setup, can you post the benchmark results here, for a "normal" resolution, as well as high resolution.
@ -1212,0 +1268,4 @@
* end up causing unnecessary remerging and less accurate damage rects.
*
* What makes this possible is the fact that pixman stores the rectangles
* as a sorted array.
Owner
Copy link

Is this guaranteed in any way (e.g. documented) or is it a pixman implementation detail?

We might want to (in debug builds) assert they truly are sorted.

Is this guaranteed in any way (e.g. documented) or is it a pixman implementation detail? We might want to (in debug builds) assert they truly are sorted.
Author
First-time contributor
Copy link

I'm pretty sure that this should be guaranteed. The thing is that Pixman itself really does not like to document anything outside of the original source code, but here in the Pixman source code, you can see how they talk about the storage representation. Either way, the code is still sound and won't cause undefined behavior if it isn't the case; it'd just be less efficient.

I'm pretty sure that this should be guaranteed. The thing is that Pixman itself really does not like to document anything outside of the original source code, but here in the [Pixman source code](<https://gitlab.freedesktop.org/pixman/pixman/-/blob/master/pixman/pixman-region.c?ref_type=heads#L121-168>), you can see how they talk about the storage representation. Either way, the code is still *sound* and won't cause undefined behavior if it isn't the case; it'd just be less efficient.
Owner
Copy link

Thanks, that's good enough for me

Thanks, that's good enough for me
dnkl marked this conversation as resolved
Author
First-time contributor
Copy link

Sorry for the delay. Here are the bench results from vtebench.

"Regular" resolution = 213 columns x 63 lines
"High" resolution = 640 columns x 240 lines

Hardware/Software

  • Device: 83K7 IdeaPad Slim 3 15ARP10
  • OS: Linux Mint trixie/sid (xia) x86_64
  • Linux kernel: 6.17.0-1023-oem
  • WM: niri 26.04
  • CPU: AMD Ryzen 5 7535HS, 12 cores, 4.6GHz, charging + performance mode
  • Background CPU usage: <1%

Low Res (all times in ms)

reg

Benchmark Original (average) Original (deviation) Mine (average) Mine (deviation) %change (negative = speedup)
cursor_motion 19.98 +-1.31 20.68 +-1.31 +3.5
dense_cells 64.58 +-3.68 63.78 +-3.98 -1.2
light_cells 13.40 +-1.31 8.90 +-0.87 -33.6
medium_cells 10.97 +-7.03 10.40 +-5.87 -5.2
scrolling 179.96 +-2.99 180.41 +-2.14 +0.3
scrolling_bottom_region 177.86 +-1.77 178.04 +-2.15 +0.1
scrolling_bottom_small_region 176.58 +-1.77 177.09 +-1.67 +0.3
scrolling_fullscreen 8.24 +-1.09 9.94 +-1.20 +20.6
scrolling_top_region 176.42 +-2.5 177.79 +-4.73 +0.8
sync_medium_cells 30.56 +-11.08 29.91 +-9.50 -2.12
unicode 11.79 +-14.23 11.97 +-11.71 +1.5

Conclusions

Mostly insignificant changes, mild regressions, mostly within noise interval.

High Res (all times in ms)

high

Benchmark Original (average) Original (deviation) Mine (average) Mine (deviation) %change (negative = speedup)
cursor_motion 646.06 +-18.36 613.59 +-11.51 -5.0
dense_cells 1021.2 +-40.72 1009.80 +-21.68 -1.1
light_cells 78.12 +-7.06 74.65 +-6.61 -4.4
medium_cells 18.96 +-2.63 14.21 +-2.49 -25.1
scrolling 288.14 +-9.24 262.26 +-4.08 -8.9
scrolling_bottom_region 287.94 +-4.54 227.55 +-2.43 -21.0
scrolling_bottom_small_region 233.53 +-5.48 231.05 +-6.45 -1.1
scrolling_fullscreen 28.13 +-5.28 26.31 +-4.96 -6.5
scrolling_top_region 293.03 +-6.78 312.66 +-11.65 +6.7
scrolling_top_small_region 260.26 +-6.96 225.58 +-7.41 -13.5
sync_medium_cells 30.29 +-4.53 29.19 +-3.00 -3.6
unicode 23.98 +-5.12 21.28 +-29.79 -11.3

Conclusions

Mostly significant performance improvements and lower variation in some benchmarks.

However, there are some regressions that stick out

  • The 6.7% regression in scrolling_top_region.
  • The absolutely bonkers change in standard variation for the unicode one.
Sorry for the delay. Here are the bench results from vtebench. "Regular" resolution = 213 columns x 63 lines "High" resolution = 640 columns x 240 lines # Hardware/Software - Device: 83K7 IdeaPad Slim 3 15ARP10 - OS: Linux Mint trixie/sid (xia) x86_64 - Linux kernel: 6.17.0-1023-oem - WM: niri 26.04 - CPU: AMD Ryzen 5 7535HS, 12 cores, 4.6GHz, charging + performance mode - Background CPU usage: <1% # Low Res (all times in ms) ![reg](/attachments/1d48cd5c-6661-4442-854a-d6c7758ae58c) | Benchmark | Original (average) | Original (deviation) | Mine (average) | Mine (deviation) | %change (negative = speedup) | |-------------------------------|--------------------|----------------------|----------------|------------------|------------------------------| | cursor_motion | 19.98 | +-1.31 | 20.68 | +-1.31 | +3.5 | | dense_cells | 64.58 | +-3.68 | 63.78 | +-3.98 | -1.2 | | light_cells | 13.40 | +-1.31 | 8.90 | +-0.87 | -33.6 | | medium_cells | 10.97 | +-7.03 | 10.40 | +-5.87 | -5.2 | | scrolling | 179.96 | +-2.99 | 180.41 | +-2.14 | +0.3 | | scrolling_bottom_region | 177.86 | +-1.77 | 178.04 | +-2.15 | +0.1 | | scrolling_bottom_small_region | 176.58 | +-1.77 | 177.09 | +-1.67 | +0.3 | | scrolling_fullscreen | 8.24 | +-1.09 | 9.94 | +-1.20 | +20.6 | | scrolling_top_region | 176.42 | +-2.5 | 177.79 | +-4.73 | +0.8 | | sync_medium_cells | 30.56 | +-11.08 | 29.91 | +-9.50 | -2.12 | | unicode | 11.79 | +-14.23 | 11.97 | +-11.71 | +1.5 | ## Conclusions Mostly insignificant changes, mild regressions, mostly within noise interval. # High Res (all times in ms) ![high](/attachments/18fd9ba8-a7da-42eb-8b31-c751c4e68711) | Benchmark | Original (average) | Original (deviation) | Mine (average) | Mine (deviation) | %change (negative = speedup) | |-------------------------------|--------------------|----------------------|----------------|------------------|------------------------------| | cursor_motion | 646.06 | +-18.36 | 613.59 | +-11.51 | -5.0 | | dense_cells | 1021.2 | +-40.72 | 1009.80 | +-21.68 | -1.1 | | light_cells | 78.12 | +-7.06 | 74.65 | +-6.61 | -4.4 | | medium_cells | 18.96 | +-2.63 | 14.21 | +-2.49 | -25.1 | | scrolling | 288.14 | +-9.24 | 262.26 | +-4.08 | -8.9 | | scrolling_bottom_region | 287.94 | +-4.54 | 227.55 | +-2.43 | -21.0 | | scrolling_bottom_small_region | 233.53 | +-5.48 | 231.05 | +-6.45 | -1.1 | | scrolling_fullscreen | 28.13 | +-5.28 | 26.31 | +-4.96 | -6.5 | | scrolling_top_region | 293.03 | +-6.78 | 312.66 | +-11.65 | +6.7 | | scrolling_top_small_region | 260.26 | +-6.96 | 225.58 | +-7.41 | -13.5 | | sync_medium_cells | 30.29 | +-4.53 | 29.19 | +-3.00 | -3.6 | | unicode | 23.98 | +-5.12 | 21.28 | +-29.79 | -11.3 | ## Conclusions Mostly significant performance improvements and lower variation in some benchmarks. However, there are some regressions that stick out - The 6.7% regression in `scrolling_top_region`. - The absolutely bonkers change in standard variation for the unicode one.
31 KiB
30 KiB
Author
First-time contributor
Copy link

All benchmarks were done with pgo enabled

All benchmarks were done with pgo enabled
Owner
Copy link

I've been holding back on this, because I hesitate about adding this much complexity.

Since your use case is fairly exotic, I don't think it's something we should optimize for. We should however not crash or hang. Would something much, much simpler work for you? I'm thinking we could check the damage rect count right at the end, before we call wl_surface_damage_buffer(), and just damage the entire window in a single rect if the number of rectangles are above a certain threshold.

I've been holding back on this, because I hesitate about adding this much complexity. Since your use case is fairly exotic, I don't think it's something we should optimize for. We _should_ however **not** crash or hang. Would something much, much simpler work for you? I'm thinking we could check the damage rect count right at the end, before we call `wl_surface_damage_buffer()`, and just damage the entire window in a single rect if the number of rectangles are above a certain threshold.
Author
First-time contributor
Copy link

Yeah sure do the simpler thing. I got a bit carried away with my silly schemes.

Yeah sure do the simpler thing. I got a bit carried away with my silly schemes.
lilyyy411 closed this pull request 2026年06月21日 04:27:40 +02:00
Some checks are pending
builds.sr.ht/freebsd-x64 Job completed
ci/woodpecker/pr/woodpecker Pipeline was successful
ci/woodpecker/pull_request_closed/woodpecker Pipeline is pending approval

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
dnkl/foot!2340
Reference in a new issue
dnkl/foot
No description provided.
Delete branch "lilyyy411/foot:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?