Re: svn commit: r1907634 - /apr/apr/trunk/include/apr_buckets.h

[Eric Covener]

> This is true, but I see only a low risk of making backports difficult for 
> structures that seem to be set and haven't been touched
> for a long time. And even if we need to touch them we can still backport. It 
> is just more work then.

+1

Re: svn commit: r1907634 - /apr/apr/trunk/include/apr_buckets.h

[Ruediger Pluem]

On 2/14/23 7:28 PM, Christophe JAILLET wrote:
> Le 14/02/2023 à 11:51, Ivan Zhakov via dev a écrit :
>> On Tue, 14 Feb 2023 at 10:58, > > wrote:
>>
>>     Author: jailletc36
>>     Date: Tue Feb 14 07:56:50 2023
>>     New Revision: 1907634
>>
>>     URL: http://svn.apache.org/viewvc?rev=1907634=rev
>>     
>>     Log:
>>     Re-order the fields of 'struct apr_bucket_file' to avoid a hole and
>>     some padding.
>>
>>     Before the patch, pahole states that:
>>
>>     struct apr_bucket_file {
>>          apr_bucket_refcount        refcount;             /*     0   
>>   4 */
>>
>>          /* XXX 4 bytes hole, try to pack */
>>
>>          apr_file_t *               fd;                   /*     8   
>>   8 */
>>          apr_pool_t *               readpool;             /*    16   
>>   8 */
>>          int                        can_mmap;             /*    24   
>>   4 */
>>
>>          /* XXX 4 bytes hole, try to pack */
>>
>>          apr_size_t                 read_size;            /*    32   
>>   8 */
>>
>>          /* size: 40, cachelines: 1, members: 5 */
>>          /* sum members: 32, holes: 2, sum holes: 8 */
>>          /* last cacheline: 40 bytes */
>>     };
>>
>> Nice, but I'd like to note that this change breaks ABI and can be released 
>> only in APR 2.0.
>>
> 
> Sure.
> 
> I was just wondering if such small optimizations make sense or not.
> It potentially makes backport of some other patches more difficult and breaks 
> ABI.

This is true, but I see only a low risk of making backports difficult for 
structures that seem to be set and haven't been touched
for a long time. And even if we need to touch them we can still backport. It is 
just more work then.

> 
> So, does it worth the effort?

I would say yes at least for structures that seem to be set and stable.

> 
> I've spotted a few of them in APR, and would make sure that such patches are 
> welcomed before committing other things.

As always, really appreciate your look on these gory details.

Regards

Rüdiger

Re: svn commit: r1907634 - /apr/apr/trunk/include/apr_buckets.h

[Christophe JAILLET]

Le 14/02/2023 à 11:51, Ivan Zhakov via dev a écrit :
On Tue, 14 Feb 2023 at 10:58, > wrote:


Author: jailletc36
Date: Tue Feb 14 07:56:50 2023
New Revision: 1907634

URL: http://svn.apache.org/viewvc?rev=1907634=rev

Log:
Re-order the fields of 'struct apr_bucket_file' to avoid a hole and
some padding.

Before the patch, pahole states that:

struct apr_bucket_file {
         apr_bucket_refcount        refcount;             /*     0 
    4 */


         /* XXX 4 bytes hole, try to pack */

         apr_file_t *               fd;                   /*     8 
    8 */
         apr_pool_t *               readpool;             /*    16 
    8 */
         int                        can_mmap;             /*    24 
    4 */


         /* XXX 4 bytes hole, try to pack */

         apr_size_t                 read_size;            /*    32 
    8 */


         /* size: 40, cachelines: 1, members: 5 */
         /* sum members: 32, holes: 2, sum holes: 8 */
         /* last cacheline: 40 bytes */
};

Nice, but I'd like to note that this change breaks ABI and can be 
released only in APR 2.0.




Sure.

I was just wondering if such small optimizations make sense or not.
It potentially makes backport of some other patches more difficult and 
breaks ABI.


So, does it worth the effort?

I've spotted a few of them in APR, and would make sure that such patches 
are welcomed before committing other things.


CJ




--
Ivan Zhakov

Re: svn commit: r1907634 - /apr/apr/trunk/include/apr_buckets.h

[Ivan Zhakov via dev]

On Tue, 14 Feb 2023 at 10:58,  wrote:

> Author: jailletc36
> Date: Tue Feb 14 07:56:50 2023
> New Revision: 1907634
>
> URL: http://svn.apache.org/viewvc?rev=1907634=rev
> Log:
> Re-order the fields of 'struct apr_bucket_file' to avoid a hole and some
> padding.
>
> Before the patch, pahole states that:
>
> struct apr_bucket_file {
> apr_bucket_refcountrefcount; /* 0 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> apr_file_t *   fd;   /* 8 8 */
> apr_pool_t *   readpool; /*16 8 */
> intcan_mmap; /*24 4 */
>
> /* XXX 4 bytes hole, try to pack */
>
> apr_size_t read_size;/*32 8 */
>
> /* size: 40, cachelines: 1, members: 5 */
> /* sum members: 32, holes: 2, sum holes: 8 */
> /* last cacheline: 40 bytes */
> };
>
> Nice, but I'd like to note that this change breaks ABI and can be released
only in APR 2.0.



-- 
Ivan Zhakov