Bug 2029 - libXRes does not give storage used for window background pixmaps
libXRes does not give storage used for window background pixmaps
Status: CLOSED FIXED
Product: xorg
Classification: Unclassified
Component: Lib/other
unspecified
x86 (IA32) Linux (All)
: high major
Assigned To: Jim Gettys
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-12-07 08:37 UTC by Matthew Allum
Modified: 2012-07-02 14:34 UTC (History)
6 users (show)

See Also:


Attachments
Fix for Xres's QueryClientPixmapBytes to include window background pixels (1.38 KB, patch)
2004-12-07 15:10 UTC, Matthew Allum
no flags Details | Splinter Review
Second attempt at patch for fixing xres for window background pixmaps (1.97 KB, patch)
2004-12-08 15:34 UTC, Matthew Allum
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Allum 2004-12-07 08:37:43 UTC
If you set a background pixmap of a window, more memory is used by the server
than a window without a background pixmap. 

One would expect XResQueryClientPixmapBytes() to include this extra window
pixmap memory but it doesn't. 

I think its actually impossible to get at this background pixmap memory usage
via XRes and therefore XRes could be quite misleading for some applications.
Comment 1 Jim Gettys 2004-12-07 13:22:51 UTC
Seems like a bug in the server; it clearly should account for the memory somehow.
Comment 2 Matthew Allum 2004-12-07 14:52:20 UTC
XResQueryClientPixmapBytes(), server side, ends up calling
FindClientResourcesByType() with RT_PIXMAP which fire a callback which
accumalates used bytes via

(pix->devKind * pix->drawable.height)

for each pixmap. ( Is that even right - should it not take into account
pix->width and pix->bitsPerPixel ? )

Im thinking I could add another call to FindClientResourcesByType() but with
RT_WINDOW and calculate additional bytes via passed win->background.pixmap ?

Does that sound sane ?
Comment 3 Matthew Allum 2004-12-07 15:10:10 UTC
Created attachment 1491 [details] [review]
Fix for Xres's QueryClientPixmapBytes to include window background pixels

Attached is a patch to kdrive's Xext/Xres.c which implements my suggestion.

It appears to work fine for me, with xrestop giving more real values.

Im still a little worried about the 'pix->devKind * pix->drawable.height'
byte calculation ( Im just doing the same thing for Win background pixmaps ),
though I *am* getting realistic values - approx ~1M per 640x480x24bpp win
backg\round pixmap.

If this is good, this should  be applied to Xorg Xext/Xres.c too.
Comment 4 Jim Gettys 2004-12-07 18:15:39 UTC
Shouldn't this computation take into account the depth of the window?

If only 16 bits, you get 2 bytes/pixel; if 32, 4, and so on...
Comment 5 Matthew Allum 2004-12-08 03:29:46 UTC
Yes thats what Im saying. 

Does something like;

pix->drawable.width * pix->drawable.height * (pix->drawable.bitsPerPixel>>3)

seem like a more realistic bytes calculation ?

Im confused to as why it would be pix->drawable.height * pix->devKind currently
and give what look like good values.
  
Comment 6 Keith Packard 2004-12-08 12:35:43 UTC
This patch will double-count any window background pixmaps which still have
resource table entries.

And, the usage of 'devKind' presumes a specific DDX architecture which will not
be accurate on xfree86 DDXen where the window background is in off-screen memory.

The computation involving height/width/bpp is probably closer to the real number.
Comment 7 Jim Gettys 2004-12-08 12:46:14 UTC
Keithp and I have been chatting about this...

This isn't correct, and XRes currently does not report sensible sizes.

As Xres walks the resource database, it needs to take the size of objects it
discovers and divide by the refcount, so that the right fraction is attributed
to the client (note this will sum to one).


Comment 8 Matthew Allum 2004-12-08 15:34:45 UTC
Created attachment 1498 [details] [review]
Second attempt at patch for fixing xres for window background pixmaps

This patch improves things via;

 - assuming pixmap memory usage via width * height * bpp>>3
 - Taking into account refcnt for a pixmap shared among multiple clients.

Im not sure what keith means when he says 'double-count any window background
pixmaps which still have resource table entries'. Is that the refcnt thing ?
If not could you explain a little more ?
Comment 9 Keith Packard 2004-12-08 18:16:01 UTC
I meant that if a window *and* an X resource pointed at a pixmap, you'd get
dunned for the pixmap twice -- once for the Pixmap ID and once for the background.

Dividing by the reference count should produce a more accurate answer for the
total client usage.  Note that this means you should also count pixmap memory
usage by GC tiles and stipples so that references from there are correctly
accounted for.  Oh, and you'll want to count composite pixmaps too, probably
accounting those against the top level window referencing them.

We should figure out if this is what we want the extension to do, or if we want
(instead) to provide some more comprehensive reporting mechanism to the client.
Comment 10 Matthew Allum 2004-12-09 08:55:44 UTC
Also should probably include window border pixmaps ( though I cant even imagine
what kind of evil would use that ).

Keith, you'll have to give me a little clue on getting at composite pixmaps.
What are your thoughts on what the extension 'should do' ?  
Comment 11 Matthew Allum 2006-07-31 09:50:53 UTC
On mr stones request, I've commited an improved version of this patch ( handling
pixmap borders and GC stipple / tile pixmaps ).