> From: Adam Jackson <[email protected]> > Date: Tue, 3 May 2016 11:24:36 -0400 > > This turns out to be a remarkably hot function in XID lookup. Consider > the expansion of CLIENT_ID. We start with: > > #define RESOURCE_CLIENT_BITS ResourceClientBits() > #define RESOURCE_AND_CLIENT_COUNT 29 > #define CLIENTOFFSET (RESOURCE_AND_CLIENT_COUNT - RESOURCE_CLIENT_BITS) > > So that's effectively: > > #define CLIENTOFFSET (29 - RCB()) > > Then: > > #define RESOURCE_CLIENT_MASK \ > ((1 << RESOURCE_CLIENT_BITS) - 1) << CLIENTOFFSET > > Is effectively: > > #define RESOURCE_CLIENT_MASK (((1 << RCB()) - 1) << (29 - RCB()) > > And: > > #define CLIENT_BITS(id) ((id) & RESOURCE_CLIENT_MASK) > #define CLIENT_ID(id) ((int)(CLIENT_BITS(id) >> CLIENTOFFSET)) > > Is: > > #define CLIENT_ID(id) \ > ((id) & (((1 << RCB()) - 1) << (29 - RCB()))) >> (29 - RCB()) > > Since ResourceClientBits isn't marked __attribute__((pure)), the > optimizer isn't allowed to CSE those function calls, since it doesn't > know they don't have side effects. And, worse, ResourceClientBits > doesn't just return an integer, it computes the ilog2 afresh every time. > > Instead we can just compute the value we need at argv parse. > > v2: Ensure ResourceClientBits is initialized even when we don't pass > -maxclients (Alan Coopersmith)
Hmm. This is marked as _X_EXPORT, so presumably part of the driver ABI. Exporting variables like this is generally a bad idea, at least for ELF DSOs. Copy relocations and all that. So isn't it better to keep this as a function call? You'd still make it return a precalculated value instead of doing the ilog2 on each call. Marking it as a "pure" function in addition to that should fine too. Cheers, Mark > Signed-off-by: Adam Jackson <[email protected]> > --- > dix/resource.c | 23 ----------------------- > include/resource.h | 4 ++-- > os/osinit.c | 1 + > os/utils.c | 13 +++++++++++++ > 4 files changed, 16 insertions(+), 25 deletions(-) > > diff --git a/dix/resource.c b/dix/resource.c > index ad71b24..f06bf58 100644 > --- a/dix/resource.c > +++ b/dix/resource.c > @@ -600,29 +600,6 @@ CreateNewResourceClass(void) > > static ClientResourceRec clientTable[MAXCLIENTS]; > > -static unsigned int > -ilog2(int val) > -{ > - int bits; > - > - if (val <= 0) > - return 0; > - for (bits = 0; val != 0; bits++) > - val >>= 1; > - return bits - 1; > -} > - > -/***************** > - * ResourceClientBits > - * Returns the client bit offset in the client + resources ID field > - *****************/ > - > -unsigned int > -ResourceClientBits(void) > -{ > - return (ilog2(LimitClients)); > -} > - > /***************** > * InitClientResources > * When a new client is created, call this to allocate space > diff --git a/include/resource.h b/include/resource.h > index 5871a4c..3cce6c6 100644 > --- a/include/resource.h > +++ b/include/resource.h > @@ -85,10 +85,10 @@ typedef uint32_t RESTYPE; > #define RT_LASTPREDEF ((RESTYPE)9) > #define RT_NONE ((RESTYPE)0) > > -extern _X_EXPORT unsigned int ResourceClientBits(void); > +extern _X_EXPORT unsigned int ResourceClientBits; > /* bits and fields within a resource id */ > #define RESOURCE_AND_CLIENT_COUNT 29 /* 29 bits for XIDs */ > -#define RESOURCE_CLIENT_BITS ResourceClientBits() /* client field > offset */ > +#define RESOURCE_CLIENT_BITS ResourceClientBits /* client field > offset */ > #define CLIENTOFFSET (RESOURCE_AND_CLIENT_COUNT - RESOURCE_CLIENT_BITS) > /* resource field */ > #define RESOURCE_ID_MASK ((1 << CLIENTOFFSET) - 1) > diff --git a/os/osinit.c b/os/osinit.c > index 54b39a0..92aced3 100644 > --- a/os/osinit.c > +++ b/os/osinit.c > @@ -88,6 +88,7 @@ int limitNoFile = -1; > > /* The actual user defined max number of clients */ > int LimitClients = LIMITCLIENTS; > +unsigned int ResourceClientBits; > > static OsSigWrapperPtr OsSigWrapper = NULL; > > diff --git a/os/utils.c b/os/utils.c > index e48d9f8..8b81d39 100644 > --- a/os/utils.c > +++ b/os/utils.c > @@ -513,6 +513,18 @@ AdjustWaitForDelay(void *waitTime, unsigned long > newdelay) > } > } > > +static unsigned int > +ilog2(int val) > +{ > + int bits; > + > + if (val <= 0) > + return 0; > + for (bits = 0; val != 0; bits++) > + val >>= 1; > + return bits - 1; > +} > + > void > UseMsg(void) > { > @@ -1061,6 +1073,7 @@ ProcessCommandLine(int argc, char *argv[]) > FatalError("Unrecognized option: %s\n", argv[i]); > } > } > + ResourceClientBits = ilog2(LimitClients); > } > > /* Implement a simple-minded font authorization scheme. The authorization > -- > 2.7.4 > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel > > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
