Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use separate pixmaps for each edge of a container #3479

Open
Airblader opened this issue Oct 26, 2018 · 26 comments · May be fixed by #5430
Open

Use separate pixmaps for each edge of a container #3479

Airblader opened this issue Oct 26, 2018 · 26 comments · May be fixed by #5430
Labels
4.16 bug help wanted reproducible A bug that has been reviewed and confirmed by a project contributor

Comments

@Airblader
Copy link
Member

I'm submitting a…

[x] Bug
[ ] Feature Request
[ ] Documentation Request
[ ] Other (Please describe in detail)

Current Behavior

i3 uses a single pixmap that covers the entire window.

Expected Behavior

To reduce memory usage, we could use separate pixmaps for the edges (top, bottom, left right) to draw titlebar + borders, and leave the inside of the windows without a (useless) pixmap. See #2742 (comment) for details.

Environment

i3 4.15

@Airblader Airblader added bug missing-log Read the CONTRIBUTING.md file for instructions reproducible A bug that has been reviewed and confirmed by a project contributor 4.15 labels Oct 26, 2018
@Airblader
Copy link
Member Author

One thing to look out for is corners, which need to be assigned to one of the edges. The upper two corners can probably easily be assigned to the top edge, but for the lower corners we might have to be more careful because of things like the split indicator.

@psychon
Copy link
Contributor

psychon commented Oct 26, 2018

Assuming you are fine with depending on cairo's tee surface support (IIRC this is not enabled by default in cairo), you could create a "virtual cairo surface with holes" to draw to:

A tee surface contains a list of other cairo surfaces. Each drawing operation on the tee surface is done to all the underlying surfaces.

For each of the four pixmaps, you create a cairo surface. You use cairo_surface_set_device_offset() to move the (0,0) point of this surface so that all these surfaces have "the right" coordinate system (i.e. the surface for the bottom edge only has its surface-local (0,0) appear at something like (0,height)). Then, you create a tee surface from all of these individual surfaces and just keep your existing code for drawing, but draw to the tee surface.

No idea if this works correctly (I wouldn't be surprised if cairo had bugs in this area, partly because this would explain why tee surfaces are not enabled by default). But if this works, it would be the least invasive change to the existing drawing code. Plus, the existing drawing code itself would not become more complicated.

(The alternative would be to not use a tee surface, but instead run the existing drawing code four times, once for each of the sub-surfaces)

@Airblader
Copy link
Member Author

That's a really interesting concept and would be cool to use, but if it's not enabled by default, we'd at least need a fallback to the current approach. And even then I guess it's only worth the (even if less) effort if it's enabled »somewhere«. :-)

@stapelberg
Copy link
Member

I think we should prioritize this issue. Especially with larger displays (e.g. 8K displays), the memory usage is noticeable. xrestop(1) shows that my i3 process clocks in at 2538396168 pixmap bytes — that’s 2.4 gigabytes!

I’m not 100% sure about this, but I think this is not “just” RAM, but (partially?) GPU RAM, which is much scarcer than regular RAM.

@Airblader Airblader added help wanted 4.16 and removed 4.15 missing-log Read the CONTRIBUTING.md file for instructions labels Feb 15, 2019
@Airblader Airblader pinned this issue Feb 15, 2019
@Airblader
Copy link
Member Author

I've pinned the issue for better visibility. I assume with "prioritize" it you didn't mean that you'll be working on it, so I tagged it as help wanted.

@stapelberg
Copy link
Member

I might be able to spend a little time on it, but don’t want to make any promises. If anyone would race me to it, that’d be appreciated :)

@stapelberg
Copy link
Member

Applied the following quick hack to verify resource usage is actually due to pixmaps:

diff --git i/src/x.c w/src/x.c
index f643a9b3..a6c8c0e4 100644
--- i/src/x.c
+++ w/src/x.c
@@ -963,6 +963,10 @@ void x_push_node(Con *con) {
             int width = MAX((int32_t)rect.width, 1);
             int height = MAX((int32_t)rect.height, 1);
 
+            if (con->deco_rect.height > 0) {
+                height = con->deco_rect.height;
+            }
+
             xcb_create_pixmap(conn, win_depth, con->frame_buffer.id, con->frame.id, width, height);
             draw_util_surface_init(conn, &(con->frame_buffer), con->frame_buffer.id,
                                    get_visualtype_by_id(get_visualid_by_depth(win_depth)), width, height);

xrestop(1) reports the following for the i3 process:
before: 1950617456 bytes (1950M)
after: 64491680 bytes (64M)

@stapelberg
Copy link
Member

Reported GPU memory usage by nvidia-smi -q -d pids,memory is still pretty high (for the Xorg process), so I’m not sure whether this change actually makes a dent in the sluggishness I can sometimes observe. Will keep running the hack and keep an eye on it.

@Airblader
Copy link
Member Author

@stapelberg Any updates on how much of a difference it would make?

@stapelberg
Copy link
Member

I have run into slowdowns even with the patch, so at least it’s not the primary cause.

We should still do it, but there’s no big urgency, at least not for me.

@Airblader
Copy link
Member Author

OK, then I'll at least remove the pin on the issue for now.

@Airblader Airblader unpinned this issue Mar 30, 2019
@xzfc
Copy link
Contributor

xzfc commented Apr 3, 2019

@psychon #2742 (comment)

Are the borders single-color?

One side may have its own color, everything else is single-color.

That could be drawn just directly to the target window instead of going through a pixmap, but I guess this would make the code slightly more complicated to avoid some visible flickering)

You mean using xcb_poly_fill_rectangle instead of drawing onto cairo surface? Could you elaborate on the causes of flickering and how to avoid it?

@psychon
Copy link
Contributor

psychon commented Apr 3, 2019

You mean using xcb_poly_fill_rectangle instead of drawing onto cairo surface?

Basically, yeah.

For drawing a single color, there will be no flickering. If you drew some background color and then (for example) some text on top of the color, one could briefly see the "only background color"-state. Thus, during repaints, the already-visible text would flicker to the background color.

@rysolv-bot
Copy link

rysolv-bot commented Feb 16, 2021

orestisfl has contributed $100.00 to this issue on Rysolv.

The total bounty is now $100.00. Solve this issue on Rysolv to earn this bounty.

Edit @Airblader: This bounty has been redistributed to this issue from an unclaimed bounty on another PR. We see this as a relatively-straightforward issue to tackle, and expect to save memory, and possibly improve performance, by solving this.

@psychon
Copy link
Contributor

psychon commented Mar 10, 2021

I've pondered a bit about this. Since what was previously one large pixmap ends up as four different ones, something has to "distribute" drawing to these four pixmaps.

  • Option A: Have the drawing code (seems to be x_draw_decoration(), but I am not entirely sure) directly handle distributing the drawing to the four different pixmaps.
    • This would make some not-completely-trivial code more complicated, so I do not really like this / will not try to do this.
  • Option B: Pretend to the drawing code that it still has one big "something" to draw to.
    • Option B1: Use cairo's tee surface. Since this is an experimental cairo feature that is not enabled by default, I guess that this is not actually an option.
    • Option B2a: Add support for the necessary "assemble one large surface from multiple small ones" to draw_util. This requires handling this special case in all the draw util functions and thus seems a bit ugly.
    • Option B2b: Add a new abstraction that sits between draw_util_* and x_draw_decoration() that handles the "assemble multiple surface_t into one large surface that can be drawn to".
    • Option B3: Make the necessary drawing target an argument to x_draw_decoration(). This code could then be called four times, each time drawing to a separate part of the target. Thanks to the support for core fonts, this requires passing some offsets through to everything and results in (IMO) ugly code. Without core fonts, cairo_surface_set_device_offset() could be used to apply some offsets.

Are there some options here that I forgot? Would you go a different route? I do not really understand the code that deals with this enough to properly judge this.

If no one disagrees / has a better suggestion, I will try to implement one of the B2 options and see where that leads.

@Airblader
Copy link
Member Author

The B2* options also seem like the best choice to me. I don't really have a preference between the two.

@stapelberg
Copy link
Member

I agree with @Airblader on this.

Thanks for taking a look!

@rysolv-bot
Copy link

mfurey has contributed $25.00 to this issue on Rysolv.

The total bounty is now $125.00. Solve this issue on Rysolv to earn this bounty.

@buchankn
Copy link

@psychon are you still working this issue? If not, would anyone mind if I looked into it?

@psychon
Copy link
Contributor

psychon commented Feb 13, 2023

@buchankn I'm not sure I ever worked on this and I totally forgot about it. I looked at my local git clone and found a branch that I just pushed: next...psychon:i3:smaller-pixmaps Apparently this tries to do option B2b.

Feel free to start over or to use what you want from that branch. Rebase/squash/whatever as you see fit.

Edit: GitHub notification tells me that I apparently opened a PR with this branch before I forgot about it: #4382 .The PR description should have more information about the state of this.

@buchankn
Copy link

Thanks @psychon!

I have a working prototype that fixes this issue, but I still have more testing/commenting/cleanup to do.

When I run the unmodified i3, I'm seeing i3 memory increase by about 35MB for every new workspace I open. With the fix, i don't see very much of a memory increase at all for each new workspace.

I do, however, see some memory increase when I open a new container in a stacked layout, so I want to investigate that a little too.

@buchankn
Copy link

buchankn commented Feb 14, 2023

I thought about the memory increase in stacked and tabbed layouts a little, and had a "duh" moment. I assume that the extra memory is caused by all the windows that are created and stacked underneath the visible one.

Doing anything to decrease that memory usage is outside the scope of the issue, so I'm not worrying about that for now.

@stapelberg
Copy link
Member

I assume that the extra memory is caused by all the windows that are created and stacked underneath the visible one.

It should be easy to confirm this by temporarily starting a different window manager and arranging the windows in the same position.

I can say that you are right in terms of windows being stacked. The X server is smart enough to not use extra framebuffer memory for stacked-but-invisible windows (hence the whole Expose event concept), but I don’t know about non-framebuffer memory.

I tried changing the implementation to unmap windows that aren’t visible, but that turned out to be infeasible: #1966 (comment)

@buchankn
Copy link

That's a good point that the X server won't use extra frame buffer memory for stacked windows. I'm curious now where this memory is being used, so I think I'll dig a little more. I already turned off BackingStore, which didn't have any affect on memory usage.

Next step is to follow your suggestion to run on another WM and look at memory usage, or maybe I'll write a bare bones cairo xcb application that creates a couple overlapping windows, without a window manager, just to characterize memory usage.

@buchankn
Copy link

buchankn commented Feb 18, 2023

Ok, I wrote a simple cairo xcb application that just opened a hello world window that filled half the screen, paused for 10 seconds, opened a second window right above it, paused for 10 seconds, and removed them one at a time.

When I ran this without a DE or WM (just X11 with an xterm), xrestop showed no pixmap bytes added for my application. I ran the same application in my prototype i3 (separate pixmaps for window borders) stacked mode, and only saw very minimal pixmap bytes added to i3, about what I'd expect from just the border pixmaps around the window.

I also ran an xterm in i3 stacked mode, and still saw very minimal pixmap bytes added to i3, just like I saw with my minimal application.

This is the i3 pixmap usage after each added xterm:

3023k
4500k
6021k
7539k

So, each xterm added about 1500k of pixmap memory, which seems reasonable for the window top decoration on a 4k monitor.

Then, I ran xfce4-terminal in i3 stacked mode, and saw about the equivalent of a whole window of pixmap bytes added to i3. Every xfce4-terminal window I added to the stack added another whole window of pixmap bytes to i3 (and almost the same amount of pixmaps was reported in xrestop for xfce4-terminal).

However, when I run with the unpatched i3 in stacked mode, xfce4-terminal seems to add twice as many pixmap bytes to i3 as my prototype version that uses separate pixmaps for the window edges. So my patched i3 version is decreasing the pixmap memory used.

I'm starting to wonder if xrestop is incorrectly adding the pixmap bytes of the re-parented window to the i3 pixmap byte count. Something to do with how i3 re-parents windows confusing xrestop, perhaps?

buchankn added a commit to buchankn/i3 that referenced this issue Feb 18, 2023
@buchankn
Copy link

buchankn commented Feb 18, 2023

@Airblader mentioned being careful of the corners, because of the split indicator. What test cases would you recommend to ensure that corners are being drawn correctly? I have an implementation that draws each side of the window border selectively (eg, it doesn't attempt to redraw all sides when you only want to draw one side). In contrast, @psychon was working an implementation that would always call the draw_util... functions for each side, for each side drawn. Eg, when the left side was drawn, the draw function was called 4 times, for each side.

For reference, I have the 'prototype' implementation here: link

I'm still testing it, but so far I haven't found a case where things are not drawn correctly on the corners. If there is an issue with it, I can switch to drawing all the side each time, but I wanted to avoid unnecessary calls/processing if it was possible.

buchankn added a commit to buchankn/i3 that referenced this issue Feb 22, 2023
buchankn added a commit to buchankn/i3 that referenced this issue Feb 23, 2023
buchankn added a commit to buchankn/i3 that referenced this issue Feb 23, 2023
buchankn added a commit to buchankn/i3 that referenced this issue Feb 23, 2023
buchankn added a commit to buchankn/i3 that referenced this issue Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.16 bug help wanted reproducible A bug that has been reviewed and confirmed by a project contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants