[PATCH 2/2] (g)unzip: Optimize inflate_codes()

[Rob Landley]

On Monday 08 February 2010 16:04:40 Joakim Tjernlund wrote:
> > Joakim Tjernlund/Transmode wrote on 2010/02/08 22:52:11:
> > > Rob Landley <rob at landley.net> wrote on 2010/02/08 21:26:44:
> > > > On Monday 08 February 2010 11:55:16 Joakim Tjernlund wrote:
> > > > > Ported the recent optimization from the Linux kernel.
> > > > > This will not perform as god as the kernel version as the
> > > > > code structure in busybox is different and I had to adopt
> > > > > the optimization to it.
> > > > >
> > > > > This has seen very little testing and is a RFC only at this point.
> > > > > The inflate speed increase in the kernel was 12-15% on ppc.
> > > > >
> > > > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund at transmode.se>
> > > >
> > > > That looks like a fairly significant size increase.  Is it big enough
> > > > to justify a config option?  (This is why "make bloatcheck"
> > > > exists...)
> > >
> > > function                                             old     new  
> > > delta inflate_codes                                        624     819 
> > >   +195 .rodata                                           131741  131755
> > >     +14
> > > -----------------------------------------------------------------------
> > >------- (add/remove: 0/0 grow/shrink: 2/0 up/down: 209/0)            
> > > Total: 209 bytes text    data     bss     dec     hex filename
> > >  705967    2137    9072  717176   af178 busybox_old
> > >  706176    2137    9072  717385   af249 busybox_unstripped
> >
> > Left some debug in, the correct bloatcheck is:
> > function                                             old     new   delta
> > inflate_codes                                        624     791    +167
> > -------------------------------------------------------------------------
> >----- (add/remove: 0/0 grow/shrink: 1/0 up/down: 167/0)             Total:
> > 167 bytes text      data       bss       dec       hex   filename
> >  705967      2137      9072    717176     af178   busybox_old
> >  706134      2137      9072    717343     af21f   busybox_unstripped
>
> And yet some:
> function                                             old     new   delta
> inflate_codes                                        624     728    +104
> ---------------------------------------------------------------------------
>--- (add/remove: 0/0 grow/shrink: 1/0 up/down: 104/0)             Total: 104
> bytes text	   data	    bss	    dec	    hex	filename
>  705967	   2137	   9072	 717176	  af178	busybox_old
>  706071	   2137	   9072	 717280	  af1e0	busybox_unstripped
>
> I don't think this should be a config option considering the increased risk
> of bugs with two variants. Inflate bugs can be pretty serious to your data.

For ~100 bytes I agree, I just wanted to make sure we'd evaluated it.  It 
seems pretty reasonable, and 104 bytes is better than 209. :)

The 12-15% speedup mentioned was on powerpc, what platform was the size 
increase measured on?  (Alas, the one thing QEMU is _not_ good for is 
performance testing.  The performance characteristics of the emulator and the 
real hardware are different enough that micro-optimizing for one can actually 
slow the other down...)

By the way, does our gzip code have an "upstream" we follow?  I.E. would it be 
easy to fairly closely mirror the kernel's inflate/deflate code so we can track 
any future changes more easily?  For example, the kernel version diverged 
significantly from both zlib and gzip, and the kernel version is gplv2 (only).  
We want to make sure _not_ to blindly copy changes from gnu gzip because gzip 
1.4 is GPLv3.

Several busybox things have an upstream from the kernel.  (bloat-o-meter and 
kconfig come to mind.)  Might be nice to document that somewhere if keeping in 
sync is considered a plus...

>    Jocke

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds