diff mbox

ALSA: drivers: make array 'names' const, reduces object code size

Message ID 20171127125851.10076-1-colin.king@canonical.com (mailing list archive)
State New, archived
Headers show

Commit Message

Colin King Nov. 27, 2017, 12:58 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Don't populate array 'names' on the stack but instead make them static.
Makes the object code smaller by 50 bytes:

Before:
   text	   data	    bss	    dec	    hex	filename
  21237	   9192	   1120	  31549	   7b3d	linux/sound/drivers/dummy.o

After:
   text	   data	    bss	    dec	    hex	filename
  21095	   9280	   1120	  31495	   7b07	linux/sound/drivers/dummy.o

(gcc version 7.2.0 x86_64)

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 sound/drivers/dummy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Walter Harms Nov. 27, 2017, 2:55 p.m. UTC | #1 
Am 27.11.2017 13:58, schrieb Colin King:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate array 'names' on the stack but instead make them static.
> Makes the object code smaller by 50 bytes:
> 
> Before:
>    text	   data	    bss	    dec	    hex	filename
>   21237	   9192	   1120	  31549	   7b3d	linux/sound/drivers/dummy.o
> 
> After:
>    text	   data	    bss	    dec	    hex	filename
>   21095	   9280	   1120	  31495	   7b07	linux/sound/drivers/dummy.o
> 
> (gcc version 7.2.0 x86_64)
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  sound/drivers/dummy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index 7b2b1f766b00..69db45bc0197 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_el
>  static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
>  				struct snd_ctl_elem_info *info)
>  {
> -	const char *const names[] = { "None", "CD Player" };
> +	static const char *const names[] = { "None", "CD Player" };
>  
>  	return snd_ctl_enum_info(info, 1, 2, names);
>  }

nitpick: while here
   snd_ctl_enum_info(info, 1, ARRAY_SIZE(names), names);

just my 2 cents,
re,
 wh
Joe Perches Nov. 27, 2017, 3:40 p.m. UTC | #2 
On Mon, 2017-11-27 at 15:55 +0100, walter harms wrote:
> Am 27.11.2017 13:58, schrieb Colin King:
> > From: Colin Ian King <colin.king@canonical.com>
> > Don't populate array 'names' on the stack but instead make them static.
> > Makes the object code smaller by 50 bytes:
[]
> > diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
[]
> > @@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_el
> >  static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
> >  				struct snd_ctl_elem_info *info)
> >  {
> > -	const char *const names[] = { "None", "CD Player" };
> > +	static const char *const names[] = { "None", "CD Player" };
> >  
> >  	return snd_ctl_enum_info(info, 1, 2, names);
> >  }
> 
> nitpick: while here
>    snd_ctl_enum_info(info, 1, ARRAY_SIZE(names), names);
> 
> just my 2 cents,

True, but that seems counter style for most uses of snd_ctl_enum_info

$ git grep -w snd_ctl_enum_info | grep -v ARRAY_SIZE | wc -l
159
$ git grep -w snd_ctl_enum_info | grep ARRAY_SIZE | wc -l
10

but most of those seem to choose variable amounts of a single
array.  Here, ARRAY_SIZE seems better to me too.

For another real nitpick, please prefer "const *" over "const*"

$ git grep -P "\*\s+const\b" | wc -l
7068
$ git grep -P "\*const\b" | wc -l
1801
Walter Harms Nov. 27, 2017, 4 p.m. UTC | #3 
Am 27.11.2017 16:40, schrieb Joe Perches:
> On Mon, 2017-11-27 at 15:55 +0100, walter harms wrote:
>> Am 27.11.2017 13:58, schrieb Colin King:
>>> From: Colin Ian King <colin.king@canonical.com>
>>> Don't populate array 'names' on the stack but instead make them static.
>>> Makes the object code smaller by 50 bytes:
> []
>>> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> []
>>> @@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_el
>>>  static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
>>>  				struct snd_ctl_elem_info *info)
>>>  {
>>> -	const char *const names[] = { "None", "CD Player" };
>>> +	static const char *const names[] = { "None", "CD Player" };
>>>  
>>>  	return snd_ctl_enum_info(info, 1, 2, names);
>>>  }
>>
>> nitpick: while here
>>    snd_ctl_enum_info(info, 1, ARRAY_SIZE(names), names);
>>
>> just my 2 cents,
> 
> True, but that seems counter style for most uses of snd_ctl_enum_info
> 
> $ git grep -w snd_ctl_enum_info | grep -v ARRAY_SIZE | wc -l
> 159
> $ git grep -w snd_ctl_enum_info | grep ARRAY_SIZE | wc -l
> 10
> 
> but most of those seem to choose variable amounts of a single
> array.  Here, ARRAY_SIZE seems better to me too.

I will not argue. I was curious about the magic numbers and
int this case it seems an obvious choice in other cases
it may be wrong i did not and i will not investigate.

re,
 wh

> 
> For another real nitpick, please prefer "const *" over "const*"
> 
> $ git grep -P "\*\s+const\b" | wc -l
> 7068
> $ git grep -P "\*const\b" | wc -l
> 1801
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Takashi Sakamoto Nov. 27, 2017, 5:34 p.m. UTC | #4 
Hi,

On Nov 27 2017 21:58, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate array 'names' on the stack but instead make them static.
> Makes the object code smaller by 50 bytes:
> 
> Before:
>     text	   data	    bss	    dec	    hex	filename
>    21237	   9192	   1120	  31549	   7b3d	linux/sound/drivers/dummy.o
> 
> After:
>     text	   data	    bss	    dec	    hex	filename
>    21095	   9280	   1120	  31495	   7b07	linux/sound/drivers/dummy.o
> 
> (gcc version 7.2.0 x86_64)
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   sound/drivers/dummy.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index 7b2b1f766b00..69db45bc0197 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_el
>   static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
>   				struct snd_ctl_elem_info *info)
>   {
> -	const char *const names[] = { "None", "CD Player" };
> +	static const char *const names[] = { "None", "CD Player" };
>   
>   	return snd_ctl_enum_info(info, 1, 2, names);
>   }

Total size of snd-dummy.ko increases but this patch has an advantage to 
have the symbol in read-only section. This looks good to me.

Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Another issue is addressed by the others, but here I focus on the 
original intention of this patch.


Thanks

Takashi Sakamoto
Takashi Iwai Nov. 27, 2017, 7:51 p.m. UTC | #5 
On Mon, 27 Nov 2017 13:58:51 +0100,
Colin King wrote:
> 
> From: Colin Ian King <colin.king@canonical.com>
> 
> Don't populate array 'names' on the stack but instead make them static.
> Makes the object code smaller by 50 bytes:
> 
> Before:
>    text	   data	    bss	    dec	    hex	filename
>   21237	   9192	   1120	  31549	   7b3d	linux/sound/drivers/dummy.o
> 
> After:
>    text	   data	    bss	    dec	    hex	filename
>   21095	   9280	   1120	  31495	   7b07	linux/sound/drivers/dummy.o
> 
> (gcc version 7.2.0 x86_64)
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Applied, thanks.


Takashi
Takashi Iwai Nov. 27, 2017, 7:51 p.m. UTC | #6 
On Mon, 27 Nov 2017 18:34:17 +0100,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Nov 27 2017 21:58, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > Don't populate array 'names' on the stack but instead make them static.
> > Makes the object code smaller by 50 bytes:
> >
> > Before:
> >     text	   data	    bss	    dec	    hex	filename
> >    21237	   9192	   1120	  31549	   7b3d	linux/sound/drivers/dummy.o
> >
> > After:
> >     text	   data	    bss	    dec	    hex	filename
> >    21095	   9280	   1120	  31495	   7b07	linux/sound/drivers/dummy.o
> >
> > (gcc version 7.2.0 x86_64)
> >
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >   sound/drivers/dummy.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> > index 7b2b1f766b00..69db45bc0197 100644
> > --- a/sound/drivers/dummy.c
> > +++ b/sound/drivers/dummy.c
> > @@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_el
> >   static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
> >   				struct snd_ctl_elem_info *info)
> >   {
> > -	const char *const names[] = { "None", "CD Player" };
> > +	static const char *const names[] = { "None", "CD Player" };
> >     	return snd_ctl_enum_info(info, 1, 2, names);
> >   }
> 
> Total size of snd-dummy.ko increases but this patch has an advantage
> to have the symbol in read-only section.

The total size decreases :)


Takashi
Takashi Sakamoto Nov. 28, 2017, 10:39 a.m. UTC | #7 
On Nov 28 2017 04:51, Takashi Iwai wrote:
> On Mon, 27 Nov 2017 18:34:17 +0100,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On Nov 27 2017 21:58, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Don't populate array 'names' on the stack but instead make them static.
>>> Makes the object code smaller by 50 bytes:
>>>
>>> Before:
>>>      text	   data	    bss	    dec	    hex	filename
>>>     21237	   9192	   1120	  31549	   7b3d	linux/sound/drivers/dummy.o
>>>
>>> After:
>>>      text	   data	    bss	    dec	    hex	filename
>>>     21095	   9280	   1120	  31495	   7b07	linux/sound/drivers/dummy.o
>>>
>>> (gcc version 7.2.0 x86_64)
>>>
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>    sound/drivers/dummy.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
>>> index 7b2b1f766b00..69db45bc0197 100644
>>> --- a/sound/drivers/dummy.c
>>> +++ b/sound/drivers/dummy.c
>>> @@ -830,7 +830,7 @@ static int snd_dummy_capsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_el
>>>    static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
>>>    				struct snd_ctl_elem_info *info)
>>>    {
>>> -	const char *const names[] = { "None", "CD Player" };
>>> +	static const char *const names[] = { "None", "CD Player" };
>>>      	return snd_ctl_enum_info(info, 1, 2, names);
>>>    }
>>
>> Total size of snd-dummy.ko increases but this patch has an advantage
>> to have the symbol in read-only section.
> 
> The total size decreases :)

Ah, yes. The state of my local build tree is not good to calculate it... 
In fact, the relocatable object decreases its size when built with a 
header package from Ubuntu repository.


Thanks

Takashi Sakamoto
diff mbox

Patch

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 7b2b1f766b00..69db45bc0197 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -830,7 +830,7 @@  static int snd_dummy_capsrc_put(struct snd_kcontrol *kcontrol, struct snd_ctl_el
 static int snd_dummy_iobox_info(struct snd_kcontrol *kcontrol,
 				struct snd_ctl_elem_info *info)
 {
-	const char *const names[] = { "None", "CD Player" };
+	static const char *const names[] = { "None", "CD Player" };
 
 	return snd_ctl_enum_info(info, 1, 2, names);
 }