Re: patch to improve performance selecting data

Il giorno mer, 31/03/2010 alle 12.33 +0100, Steve Brown (FICC IT) ha
scritto:
> Hi, I recently did a small test of performance on freetds library
> against Sybase ASE 15.0.3 supplied client-libs on RHEL5.4 x64; and found
> freetds to be significantly slower.
> 
> After running through callgrind, it seemed that tdsdump_log was the
> majority of the difference... spending about 23% of the program time in
> this function even though the logging was turned off... it was basically
> because this function was called 52M times so the call-overhead was
> significant.
> 
> Here is a patch to resolve this issue... it uses a macro to check
> write_dump variable (renamed to tds_write_dump) before making the
> function call.
> 

Applied! I did some minor change in order to avoid ABI change
(tdsdump_log remains tdsdump_log).
I also add an extra patch to use __builtin_expect if available.

> My timing tests (in seconds, smaller is better) are below; averaged over
> 5 runs. "real" time can probably be ignored since it includes the
> elapsed time spent waiting for my Sybase server to respond.  
> 
>  before patch:
> 
> Select a varchar column from a big table: 
> what, Sybase 15.0.3,  freetds-0.82
> time real,  6.3s, 5.8s
> time user, 0.90s, 1.69s (quite a lot slower)
> time sys, 0.50s, 0.47s
> 
> Select varchar,varchar,int columns from a big table: 
> what, Sybase 15.0.3,  freetds-0.82
> time real,  9.1s, 9.8s
> time user, 1.53s, 2.41s (quite a lot slower)
> time sys, 0.89s, 0.98s
> 
> After patch:
> 
> Select a varchar column from a big table: 
> what, Sybase 15.0.3,  freetds-0.82
> time real,  6.3s, 6.2s
> time user, 0.90s, 1.21s (better than unpatched)
> time sys, 0.50s, 0.51
> 
> Select varchar,varchar,int columns from a big table: 
> what, Sybase 15.0.3,  freetds-0.82
> time real,  9.1s, 9.2s
> time user, 1.53s, 1.59s (better than unpatched and almost same speed as
> sybase)
> time sys, 0.89s,  1.06s
> 
> I believe there are other opportunities for speeding  up tds but I have
> not made these as I don't have a good enough understanding of the code,
> nor time to test... E.g:
> 
> 1.
> Caching (if possible) the srctype and desttype variables in
> buffer_transfer_bound_data... I'm not familiar with the code; but it
> would seem sensible to do those calls (desttype =
> _db_get_server_type(..),  srctype = tds_get_conversion_type(..) ) just
> once for the current result-set and store the results inside the
> row->resinfo->columns[i] object.

I think so, we should add some cache fields and check for consistency.
Perhaps even reviewing conversions could help avoiding many switches
with same values.

> 2.
> 	use num->array[0] = 0 instead of memset in tds_get_data()
> although this may be a security concern (not clearing memory).
> 

Yes, mainly for security reasons.

> Steve
> 

Regards
  Frediano