FreeTDS Development Group

["ZIGLIO Frediano" <Frediano.Ziglio AT vodafone.com>]

> 
> Granted.  I'm not done, but I think this is the right road.  
> 
> I got a little gun-shy after breaking things in a big way.  
> I'll stir things up again after my vacation. ;-)
> 

Hi James!
Have a nice holiday!

...
> 
> I think I understand you.  tds_put_string() tells tds_iconv() 
> to write to its local staging buffer, outbuf.  Then it calls 
> tds_put_n(), which copies the data to the socket's output 
> buffer (TDSSOCKET::out_buf).  That's an extra copy.  When 
> out_buf fills up, tds_put_n() writes the packet and continues.  
> 
> (You care about these efficiencies more than I do.)  
> 
> To avoid calling tds_put_n() and its memcpy(), teach 
> tds_put_string() to write directly to tds->out_buf, calling 
> tds_write_packet() and
> tds_init_write_buf() itself.  See the attached patch, which 
> adds a net of 2 lines of code.  (Note: not compiled.  Just an 
> illustration.)  Also note the IS_TDS7_PLUS test is gone, too, 
> because we might as well have
> tds_iconv() write to tds->out_buf unconditionally.  
> 

Not compiled but surely can produce a infinite loop if network buffer
have few bytes left...
Yes, this produce a performance gain (no copy at all). However every
call to tds_iconv that have to convert large data should copy loop
code... I understood that my code solve only this problem (code loop
only in iconv.c) and no others problems... So an unique function that
use callback is in many case not good.

We are however moving very far away from my main aim: avoid system iconv
dependency using abstract functions.

> > > I assert you can't make read_and_convert() substantially more 
> > > efficient, certainly not via the abstraction you've described. 
> > > Perhaps you can show me I'm wrong about that, or pick 
> something else 
> > > that would benefit more?
> > 
> > IMHO main benefit is you write conversion stuff in a single 
> place. No 
> > more loops spread in read.c, write.c, bcp.c or other...
> 
> I think the number of loops will be limited.  Debugging a 
> callback system
> is no picnic, either.   
> 
> > > A lot of the buffer-to-buffer conversions happen in login.c, and 
> > > those are small predictable strings.
> > 
> > However are dynamic. 0.53 had limitation of 30 characters for these 
> > buffer. Some time later someone required a longer password 
> so we used 
> > dynamic. I agree these fields are limited (128 for 
> hostname, 128 for 
> > database and so on). Perhaps two convert functions like
> >   tds_iconv1(TDSSOCKET*, TDSICONVINFO*, const char* src, 
> size_t src_len,
> > char* dest, size_t* dest_len);
> > for simple conversions
> >   tds_iconv2(TDSSOCKET*, TDSICONVINFO*, TDSREAD* , TDSWRITE* dest);
> > for longer ones
> 
> Remember, I was attracted to "tds_iconv2" in April, too.  It 
> *seems* reasonable to provide a function instead of a buffer. 
>  Having worked over the code, though, I'm convinced it's an 
> illusory gain because 1) there aren't many places to use it, 
> 2) the setup and teardown are significant, and 3) it's a 
> complex abstraction that makes the employment of iconv(3) 
> harder to understand and debug.  The KISS principle tells me, 
> Don't do it.
> 

1) fully agreed
2) I don't think so... if TDSREAD/TDSWRITE will became "popular" in
FreeTDS...
3) However you have a single place of code to debug. Also you fix state
problems. What's state problem? If you are convertiong a string and you
just stop you can alter iconv state causing wrong conversions. The
solution is to reset state for every conversion (but not for every
chunk!!), so calling a function that do the entire conversion fix the
problem...

> 
> My approach removes the buffer copy by adding 2 lines of 
> code, and removing a layer in the call stack.  If you can do 
> better, please show me!
> :-)
> 

Mmmm... are you challenging me ? :-)
I think first we should remove some "FIXME ICONV" from FreeTDS code... I
don't think to have time to code some iconv stuff before you came back
from your holiday so switch off your computer go to vacation and relax
yourself.

freddy77