[Xcb] [PATCH 1/2] Don't declare xcb_req as static

[Adam Jackson]

Doing so forces the compiler to allocate storage for the symbol in
.data, which means 24 bytes of dirty data and a relocation per request
function.

text data bss dec hex filename
88888 7136 8 96032 17720 src/.libs/libxcb-glx.so.before
92432 680 8 93120 16bc0 src/.libs/libxcb-glx.so.after

Signed-off-by: Adam Jackson <***@redhat.com>
---
src/c_client.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/c_client.py b/src/c_client.py
index 57de3fb..3b61f35 100644
--- a/src/c_client.py
+++ b/src/c_client.py
@@ -2319,7 +2319,7 @@ def _c_request_helper(self, name, void, regular, aux=False, reply_fds=False):
dimension = count + 2

_c('{')
- _c(' static const xcb_protocol_request_t xcb_req = {')
+ _c(' const xcb_protocol_request_t xcb_req = {')
_c(' .count = %d,', count)
_c(' .ext = %s,', func_ext_global)
_c(' .opcode = %s,', self.c_request_name.upper())

[Adam Jackson]

Signed-off-by: Adam Jackson <***@redhat.com>
---
src/xcb_conn.c | 2 +-
src/xcb_out.c | 4 ++--
src/xcb_util.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/xcb_conn.c b/src/xcb_conn.c
index 7d09637..6632d90 100644
--- a/src/xcb_conn.c
+++ b/src/xcb_conn.c
@@ -125,7 +125,7 @@ static int set_fd_flags(const int fd)

static int write_setup(xcb_connection_t *c, xcb_auth_info_t *auth_info)
{
- static const char pad[3];
+ const char pad[3];
xcb_setup_request_t out;
struct iovec parts[6];
int count = 0;
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 3601a5f..dc51ae0 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -70,7 +70,7 @@ static inline void send_request(xcb_connection_t *c, int isvoid, enum workaround

static void send_sync(xcb_connection_t *c)
{
- static const union {
+ const union {
struct {
uint8_t major;
uint8_t pad;
@@ -237,7 +237,7 @@ uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iove

if(!(flags & XCB_REQUEST_RAW))
{
- static const char pad[3];
+ const char pad[3];
unsigned int i;
uint16_t shortlen = 0;
size_t longlen = 0;
diff --git a/src/xcb_util.c b/src/xcb_util.c
index a3357ef..d5605da 100644
--- a/src/xcb_util.c
+++ b/src/xcb_util.c
@@ -227,9 +227,9 @@ static int _xcb_open(const char *host, char *protocol, const int display)
{
int fd;
#ifdef __hpux
- static const char unix_base[] = "/usr/spool/sockets/X11/";
+ const char unix_base[] = "/usr/spool/sockets/X11/";
#else
- static const char unix_base[] = "/tmp/.X11-unix/X";
+ const char unix_base[] = "/tmp/.X11-unix/X";
#endif
const char *base = unix_base;
size_t filelen;

[Eric Anholt]

Post by Adam Jackson
---
src/xcb_conn.c | 2 +-
src/xcb_out.c | 4 ++--
src/xcb_util.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/xcb_conn.c b/src/xcb_conn.c
index 7d09637..6632d90 100644
--- a/src/xcb_conn.c
+++ b/src/xcb_conn.c
@@ -125,7 +125,7 @@ static int set_fd_flags(const int fd)
static int write_setup(xcb_connection_t *c, xcb_auth_info_t *auth_info)
{
- static const char pad[3];
+ const char pad[3];
xcb_setup_request_t out;
struct iovec parts[6];
int count = 0;

Isn't this taking pad from being implicitly zeroed to being stack
garbage? I think you'd need " = { 0 }"

Post by Adam Jackson
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 3601a5f..dc51ae0 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -70,7 +70,7 @@ static inline void send_request(xcb_connection_t *c, int isvoid, enum workaround
static void send_sync(xcb_connection_t *c)
{
- static const union {
+ const union {
struct {
uint8_t major;
uint8_t pad;

Definitely a good one. This union should compile to an immediate
constant in the code.

Post by Adam Jackson
@@ -237,7 +237,7 @@ uint64_t xcb_send_request_with_fds64(xcb_connection_t *c, int flags, struct iove
if(!(flags & XCB_REQUEST_RAW))
{
- static const char pad[3];
+ const char pad[3];
unsigned int i;
uint16_t shortlen = 0;
size_t longlen = 0;

Same stack concern.

Post by Adam Jackson
diff --git a/src/xcb_util.c b/src/xcb_util.c
index a3357ef..d5605da 100644
--- a/src/xcb_util.c
+++ b/src/xcb_util.c
@@ -227,9 +227,9 @@ static int _xcb_open(const char *host, char *protocol, const int display)
{
int fd;
#ifdef __hpux
- static const char unix_base[] = "/usr/spool/sockets/X11/";
+ const char unix_base[] = "/usr/spool/sockets/X11/";
#else
- static const char unix_base[] = "/tmp/.X11-unix/X";
+ const char unix_base[] = "/tmp/.X11-unix/X";
#endif

Also good.

[Eric Anholt]

Post by Adam Jackson
Doing so forces the compiler to allocate storage for the symbol in
.data, which means 24 bytes of dirty data and a relocation per request
function.
text data bss dec hex filename
88888 7136 8 96032 17720 src/.libs/libxcb-glx.so.before
92432 680 8 93120 16bc0 src/.libs/libxcb-glx.so.after

Increasing the executed-code size by 4% here makes me nervous -- done
any performance comparison before and after? I think that delta will
get buried under the overhead of xcb_send_request, but it would be nice
to be sure.

Is the problem just the relocations in that data? Would pulling the
req->ext out of xcb_protocol_request and passing it as another parameter
of a new xcb_send_request variant also fix it, while letting us keep the
remainder of the xcb_protocol_request_t static?

[Keith Packard]

Post by Eric Anholt
Increasing the executed-code size by 4% here makes me nervous

Overall memory usage in the system will be less as you get rid of the
data segment duplication in every process.

Post by Eric Anholt
Is the problem just the relocations in that data?

Yes, even if the xcb_extension_t is also const, you've got a relocation
to deal with.

Post by Eric Anholt
Would pulling the req->ext out of xcb_protocol_request and passing it
as another parameter of a new xcb_send_request variant also fix it,
while letting us keep the remainder of the xcb_protocol_request_t
static?

Yes, it looks like that should work. I note that there's also a global
mutex lock for every extension request which would be nice to eliminate
at some point :-)

[Keith Packard]

Post by Adam Jackson
Doing so forces the compiler to allocate storage for the symbol in
.data, which means 24 bytes of dirty data and a relocation per request
function.

Yeah, no way to make it actually constant as it has a pointer to the
xcb_extension_t.

Reviewed-by: Keith Packard <***@keithp.com>