[PATCH] dix: Reshuffle ScreenRec to pack holes

[Adam Jackson]

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>