Share graphics context globally #4376
Conversation
libi3/draw_util.c
Outdated
| static struct { | ||
| uint8_t depth; | ||
| xcb_gcontext_t gc; | ||
| } gc_cache[2] = { |
There was a problem hiding this comment.
I feel like in i3 we are sticking too much in primitive data structures even though we can take advantage of more advanced ones, e.g. this could have been implemented more easily with glib's hashtable and glib is already a dependency.
I am not really requesting that you re-write this, just an observation. You can ignore this comment.
There was a problem hiding this comment.
Yeah, I didn't use much glib so far. I quickly looked at the link and then decided to go with You can ignore this comment. 😅
|
I rebased this on I am not entirely sure, but the following comments now seems outdated: if (surface->owns_gc) {
/* NOTE: This function is also called on uninitialised surface_t instances.
* The x11 error from xcb_free_gc(conn, XCB_NONE) is silently ignored
* elsewhere.
*/
xcb_free_gc(conn, surface->gc);
}
However, this is just a guess and I don't have the time right now to dig deeper, so I am just mentioning this here and keeping things otherwise the way they already were. |
psychon
force-pushed
the
gc_cache
branch
3 times, most recently
from
6ad3b66
to
cb38b26
Compare
Instead of creating a graphics context for every surface_t, this commit adds a cache that allows to "remember" up to two GCs. Thus, the code uses less GCs. When a GC from the cache can be used, this also gets rid of a round-trip to the X11 server. Both of these are tiny, insignificant savings, but so what? Since GCs are per-depth, this code needs access to get_visual_depth(). To avoid a code duplication, this function is moved to libi3. Fixes: i3#3478 Signed-off-by: Uli Schlachter <psychon@znc.in>
| } | ||
|
|
||
| xcb_gcontext_t gc = xcb_generate_id(conn); | ||
| xcb_void_cookie_t gc_cookie = xcb_create_gc_checked(conn, gc, drawable, 0, NULL); |
There was a problem hiding this comment.
| xcb_void_cookie_t gc_cookie = xcb_create_gc_checked(conn, gc, drawable, 0, NULL); | |
| /* The drawable is only used to get the root and depth, thus the gc is not | |
| * tied to the drawable and it can be re-used from different drawables. */ | |
| xcb_void_cookie_t gc_cookie = xcb_create_gc_checked(conn, gc, drawable, 0, NULL); |
I came up with this while trying to understand why this is safe to do e.g. when creating a gc from a drawable that is later freed. If my comment is correct, maybe we can include it for further context to readers?
There was a problem hiding this comment.
Done.
"Definitive answer" seems to come from the X11 protocol reference manual: https://www.x.org/releases/X11R7.6/doc/xproto/x11protocol.html#requests:CreateGC
The gcontext can be used with any destination drawable having the same root and depth as the specified drawable [...]
Signed-off-by: Uli Schlachter <psychon@znc.in>
Instead of creating a graphics context for every surface_t, this commit
adds a cache that allows to "remember" up to two GCs. Thus, the code
uses less GCs. When a GC from the cache can be used, this also gets rid
of a round-trip to the X11 server. Both of these are tiny, insignificant
savings, but so what?
Fixes: #3478
Signed-off-by: Uli Schlachter psychon@znc.in
This adds a copy&paste duplicate of
get_visual_depth()fromsrc/xcb.cto `libi3. If you tell me what to do, I can clean this up again, but I was not really sure what the best option here is.I tried testing the cache overflow by trying to create a window with a depth other than 24 or 32, but my X11 server does not offer any such visuals:
Also, initially I planned to use a
SLISTfor this and have the cache unlimited. After briefly staring at the macros, I went with a statically sized solution instead. Thus, there is now the possibility that a GC cannot be cached and is thus still owned by thesurface_t.