[PATCH xserver 3/4] dix: Convert ResourceClientBits to a variable

[Mark Kettenis]

> From: Adam Jackson <ajax at redhat.com>
> 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 <ajax at redhat.com>
> ---
>  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
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
> 
>