[PATCH] dix: Reshuffle ScreenRec to pack holes
Adam Jackson
ajax at nwnk.net
Wed May 19 13:04:14 PDT 2010
On Wed, 2010-05-19 at 12:23 -0700, Jamey Sharp wrote:
> On Wed, May 19, 2010 at 11:31 AM, Adam Jackson <ajax at redhat.com> wrote:
> > typedef struct _Screen {
> > int myNum; /* index of this instance in Screens[] */
> > - ATOM id;
> > + unsigned int rgf; /* array of flags; she's -- HUNGARIAN */
>
> Wow, I just wondered what that was, and the answer is dumb. I'm not
> sure it should be moved away from GCperDepth. At least it's safe to
> shrink it to int, or even short, as long as MAXFORMATS continues to be
> 8. In fact, I think you can legally declare it "unsigned rgf :
> MAXFORMATS + 1;", and throw it in the unused_flags if you want.
> Alternatively, use one of the unused flag bits in each GC to mark
> whether it's free for reuse, and drop this rgf field entirely.
That (using a GC bit) is not a terrible idea at all.
> How awful would it be to kill the GCperDepth pool entirely, and let
> GetScratchGC always just delegate to CreateScratchGC?
I suspect that these days it mostly just amortizes malloc overhead for
scratch GC creation. We really don't push GC state down to the device
until the very last second. I'd want to find a rendering path that
really hammers scratch GCs to find out though.
> > + unsigned long whitePixel, blackPixel;
> > + unsigned int rootVisual;
> > + unsigned int defColormap;
>
> Seems to me these should be declared Pixel, VisualID, and Colormap,
> respectively, which are all CARD32. That should save another 8 bytes
> on 64-bit, I think?
Weeeellll...
I dunno, the whole blackPixel/whitePixel thing is an anachronism. The
idea is that on monochrome framebuffers it's not especially obvious
whether 1 or 0 is black. But, anymore, I really can't justify giving a
damn. 0 is black, move on.
Given _that_, we could just rip it out of ScreenRec entirely.
The other two, sure.
> > + unsigned char rootDepth;
> > + unsigned backingStoreSupport:1;
> > + unsigned saveUnderSupport:1;
> > + unsigned unused_flags:22;
>
> Why is this a 24-bit bitfield? Is GCC packing it together with the
> 1-byte rootDepth?
Yep. Not the most obvious thing. Could probably be:
unsigned rootDepth : 5;
unsigned backingStoreSupport : 1;
unsigned saveUnderSupport : 1;
unsigned unused_flags : 17;
It's really just an optimization to store it in the ScreenRec, you could
equivalently do:
int
RootDepth(ScreenPtr s)
{
return WindowTable[s->myNum]->drawable.depth;
}
I doubt there's any measurable performance impact from doing that.
- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-devel/attachments/20100519/b0adc298/attachment.pgp>
More information about the xorg-devel
mailing list