[libxcb,9/7] c_client.py: don't start xcb_parts at offset 2 for no reason

Submitted by Ran Benita on Feb. 25, 2014, 12:11 p.m.

Details

Message ID 1393330297-25151-2-git-send-email-ran234@gmail.com
State Rejected
Headers show

Not browsing as part of any series.

Commit Message

Ran Benita Feb. 25, 2014, 12:11 p.m.
Currently the generated code looks like this:

    struct iovec xcb_parts[4];
    xcb_big_requests_enable_cookie_t xcb_ret;
    xcb_big_requests_enable_request_t xcb_out;

    xcb_parts[2].iov_base = (char *) &xcb_out;
    xcb_parts[2].iov_len = sizeof(xcb_out);
    xcb_parts[3].iov_base = 0;
    xcb_parts[3].iov_len = -xcb_parts[2].iov_len & 3;

    xcb_ret.sequence = xcb_send_request(c, XCB_REQUEST_CHECKED, xcb_parts + 2, &xcb_req);

But there is no reason to allocate e.g. 4 parts and then skip the first
2. Start at 0 instead.

Signed-off-by: Ran Benita <ran234@gmail.com>
---
 src/c_client.py | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/c_client.py b/src/c_client.py
index 402e93f..f2bef2f 100644
--- a/src/c_client.py
+++ b/src/c_client.py
@@ -1934,7 +1934,6 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
                 if field.type.c_need_serialize:
                     # _serialize() keeps track of padding automatically
                     count -= 1
-    dimension = count + 2
 
     _c('{')
     _c('    static const xcb_protocol_request_t xcb_req = {')
@@ -1945,7 +1944,7 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
     _c('    };')
     _c('')
 
-    _c('    struct iovec xcb_parts[%d];', dimension)
+    _c('    struct iovec xcb_parts[%d];', count)
     _c('    %s xcb_ret;', func_cookie)
     _c('    %s xcb_out;', self.c_type)
     if self.c_var_followed_by_fixed_fields:
@@ -1991,12 +1990,12 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
 
     _c('')
     if not self.c_var_followed_by_fixed_fields:
-        _c('    xcb_parts[2].iov_base = (char *) &xcb_out;')
-        _c('    xcb_parts[2].iov_len = sizeof(xcb_out);')
-        _c('    xcb_parts[3].iov_base = 0;')
-        _c('    xcb_parts[3].iov_len = -xcb_parts[2].iov_len & 3;')
+        _c('    xcb_parts[0].iov_base = (char *) &xcb_out;')
+        _c('    xcb_parts[0].iov_len = sizeof(xcb_out);')
+        _c('    xcb_parts[1].iov_base = 0;')
+        _c('    xcb_parts[1].iov_len = -xcb_parts[0].iov_len & 3;')
 
-        count = 4
+        count = 2
 
         for field in param_fields:
             if not field.type.fixed_size():
@@ -2050,15 +2049,14 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
 
     # elif self.c_var_followed_by_fixed_fields:
     else:
-        _c('    xcb_parts[2].iov_base = (char *) &xcb_out;')
+        _c('    xcb_parts[0].iov_base = (char *) &xcb_out;')
         # request header: opcodes + length
-        _c('    xcb_parts[2].iov_len = 2*sizeof(uint8_t) + sizeof(uint16_t);')
-        count += 1
+        _c('    xcb_parts[0].iov_len = 2*sizeof(uint8_t) + sizeof(uint16_t);')
         # call _serialize()
         buffer_var = '&xcb_aux'
         serialize_args = get_serialize_args(self, buffer_var, '&xcb_out', 'serialize')
-        _c('    xcb_parts[%d].iov_len = %s (%s);', count, self.c_serialize_name, serialize_args)
-        _c('    xcb_parts[%d].iov_base = (char *) xcb_aux;', count)
+        _c('    xcb_parts[1].iov_len = %s (%s);', self.c_serialize_name, serialize_args)
+        _c('    xcb_parts[1].iov_base = (char *) xcb_aux;')
         free_calls.append('    free(xcb_aux);')
         # no padding necessary - _serialize() keeps track of padding automatically
 
@@ -2067,7 +2065,7 @@  def _c_request_helper(self, name, cookie_type, void, regular, aux=False, reply_f
         if field.isfd:
             _c('    xcb_send_fd(c, %s);', field.c_field_name)
 
-    _c('    xcb_ret.sequence = xcb_send_request(c, %s, xcb_parts + 2, &xcb_req);', func_flags)
+    _c('    xcb_ret.sequence = xcb_send_request(c, %s, xcb_parts, &xcb_req);', func_flags)
 
     # free dyn. all. data, if any
     for f in free_calls:

Comments

On Tue, Feb 25, 2014 at 02:11:36PM +0200, Ran Benita wrote:
> Currently the generated code looks like this:
> 
>     struct iovec xcb_parts[4];
>     xcb_big_requests_enable_cookie_t xcb_ret;
>     xcb_big_requests_enable_request_t xcb_out;
> 
>     xcb_parts[2].iov_base = (char *) &xcb_out;
>     xcb_parts[2].iov_len = sizeof(xcb_out);
>     xcb_parts[3].iov_base = 0;
>     xcb_parts[3].iov_len = -xcb_parts[2].iov_len & 3;
> 
>     xcb_ret.sequence = xcb_send_request(c, XCB_REQUEST_CHECKED, xcb_parts + 2, &xcb_req);
> 
> But there is no reason to allocate e.g. 4 parts and then skip the first
> 2. Start at 0 instead.

Take a look at the implementation of xcb_send_request: if it needs to
send extra data before the request, it will use the iovec entries before
the passed-in pointer.  Yes, that's part of the API.  It really ought to
be more clearly documented.

- Josh Triplett
On Tue, Feb 25, 2014 at 05:34:27AM -0800, Josh Triplett wrote:
> On Tue, Feb 25, 2014 at 02:11:36PM +0200, Ran Benita wrote:
> > Currently the generated code looks like this:
> > 
> >     struct iovec xcb_parts[4];
> >     xcb_big_requests_enable_cookie_t xcb_ret;
> >     xcb_big_requests_enable_request_t xcb_out;
> > 
> >     xcb_parts[2].iov_base = (char *) &xcb_out;
> >     xcb_parts[2].iov_len = sizeof(xcb_out);
> >     xcb_parts[3].iov_base = 0;
> >     xcb_parts[3].iov_len = -xcb_parts[2].iov_len & 3;
> > 
> >     xcb_ret.sequence = xcb_send_request(c, XCB_REQUEST_CHECKED, xcb_parts + 2, &xcb_req);
> > 
> > But there is no reason to allocate e.g. 4 parts and then skip the first
> > 2. Start at 0 instead.
> 
> Take a look at the implementation of xcb_send_request: if it needs to
> send extra data before the request, it will use the iovec entries before
> the passed-in pointer.  Yes, that's part of the API.  It really ought to
> be more clearly documented.

Oh, that's easy to miss. I need to add big requests to my testings.
Thanks for catching this, and thanks Uli for documenting it.

Ran

> - Josh Triplett
On 25.02.2014 21:08, Ran Benita wrote:
> On Tue, Feb 25, 2014 at 05:34:27AM -0800, Josh Triplett wrote:
>> On Tue, Feb 25, 2014 at 02:11:36PM +0200, Ran Benita wrote:
>>> Currently the generated code looks like this:
>>>
>>>     struct iovec xcb_parts[4];
>>>     xcb_big_requests_enable_cookie_t xcb_ret;
>>>     xcb_big_requests_enable_request_t xcb_out;
>>>
>>>     xcb_parts[2].iov_base = (char *) &xcb_out;
>>>     xcb_parts[2].iov_len = sizeof(xcb_out);
>>>     xcb_parts[3].iov_base = 0;
>>>     xcb_parts[3].iov_len = -xcb_parts[2].iov_len & 3;
>>>
>>>     xcb_ret.sequence = xcb_send_request(c, XCB_REQUEST_CHECKED, xcb_parts + 2, &xcb_req);
>>>
>>> But there is no reason to allocate e.g. 4 parts and then skip the first
>>> 2. Start at 0 instead.
>>
>> Take a look at the implementation of xcb_send_request: if it needs to
>> send extra data before the request, it will use the iovec entries before
>> the passed-in pointer.  Yes, that's part of the API.  It really ought to
>> be more clearly documented.
> 
> Oh, that's easy to miss. I need to add big requests to my testings.
> Thanks for catching this, and thanks Uli for documenting it.

Not just big requests. One entry can be used by xcb_send_request() if it needs
big requests, another entry might be used in send_request() if the output buffer
is full and needs to be flushed. I would say that it's virtually impossible for
any code to hit both these cases at the same time accidentally.

I would say that the later condition is rather hard to hit on purpose without
ending up with some blatantly slow code (Anyone feels like sending NoOperation
requests of size 2^16 - something small?).

Uli