From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS6939 64.71.128.0/18 X-Spam-Status: No, score=-2.0 required=3.0 tests=AWL,BAYES_00, MSGID_FROM_MTA_HEADER shortcircuit=no autolearn=unavailable version=3.3.2 Path: news.gmane.org!not-for-mail From: Eric Wong Newsgroups: gmane.comp.lang.ruby.kgio.general Subject: Re: [PATCH 3/3] add `#kgio_writev` and `#kgio_trywritev` Date: Thu, 31 May 2012 14:19:23 -0700 Message-ID: <20120531211923.GA21892@dcvr.yhbt.net> References: <20120530203915.GB17661@dcvr.yhbt.net> <1338454833-11754-1-git-send-email-funny.falcon@gmail.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Trace: dough.gmane.org 1338499188 24704 80.91.229.3 (31 May 2012 21:19:48 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Thu, 31 May 2012 21:19:48 +0000 (UTC) To: kgio@librelist.com Original-X-From: kgio@librelist.com Thu May 31 23:19:47 2012 Return-path: Envelope-to: gclrkg-kgio@m.gmane.org List-Archive: List-Help: List-Id: List-Post: List-Subscribe: List-Unsubscribe: Precedence: list Original-Sender: kgio@librelist.com Xref: news.gmane.org gmane.comp.lang.ruby.kgio.general:169 Archived-At: Received: from zedshaw.xen.prgmr.com ([64.71.167.205]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1SaCmV-0006eZ-LK for gclrkg-kgio@m.gmane.org; Thu, 31 May 2012 23:19:44 +0200 Received: from zedshaw.xen.prgmr.com (localhost [IPv6:::1]) by zedshaw.xen.prgmr.com (Postfix) with ESMTP id 4E8F921DD1C for ; Thu, 31 May 2012 21:27:38 +0000 (UTC) Sokolov Yura 'funny-falcon wrote: > +/* it seems that socket buffer is rarely exceed 1MB */ See other email about LFN. On the other hand, 1M may be a good size anyways to fit into the CPU cache. Buffer size tuning is hard so I usually avoid it :) > +#define WRITEV_MEMLIMIT (1024*1024) > +#define WRITEV_IMPL_THRESHOLD 512 I would like to see data/examples/benchmarks for how you arrived at the WRITEV_IMPL_THRESHOLD. This patch looks pretty good. More comments below. > +static unsigned int iov_max = 1024; /* this could be overriden in init */ > + > +struct io_args_v { > + VALUE io; > + VALUE buf; > + VALUE vec_buf; > + struct iovec *vec; > + unsigned long iov_cnt; > + size_t iov_len; The "iov_len" name confused me, I think something with "total" is better. Perhaps "total_len"? > +static void prepare_writev(struct io_args_v *a, VALUE io, VALUE ary) > +{ > + long vec_cnt; > + static struct iovec stub = {NULL, 0}; > + a->io = io; > + a->fd = my_fileno(io); > + a->something_written = 0; > + > + /* rb_ary_subseq will not copy array unless it modified */ > + a->buf = rb_ary_subseq(ary, 0, RARRAY_LEN(ary)); > + > + a->vec_buf = rb_str_new(0, 0); > + a->vec = &stub; Can we just leave a->vec unset? It's always set in fill_iovec anyways. Static variables inside library function smells funny. > +static void fill_iovec(struct io_args_v *a) > +{ > + unsigned long i; > + struct iovec *curvec; > + > + a->iov_cnt = RARRAY_LEN(a->buf); > + a->iov_len = 0; > + if (a->iov_cnt == 0) return; > + if (a->iov_cnt > iov_max) a->iov_cnt = iov_max; > + rb_str_resize(a->vec_buf, sizeof(struct iovec) * a->iov_cnt); > + curvec = a->vec = (struct iovec*)RSTRING_PTR(a->vec_buf); > + /* lets limit total memory to write, > + * but always take first string */ > + next_len = a->iov_len + str_len; > + if (i && next_len > WRITEV_MEMLIMIT) { > + a->iov_cnt = i; > + break; Use hard tabs like the rest of the file > +static VALUE my_writev(VALUE io, VALUE str, int io_wait) > +{ > + struct io_args_v a; > + long n; > + > + prepare_writev(&a, io, str); > + set_nonblocking(a.fd); > + > + do { > + fill_iovec(&a); > +#ifdef HAVE_WRITEV > + /* for big strings use library function */ > + if (a.iov_len / WRITEV_IMPL_THRESHOLD > a.iov_cnt) > + n = (long)writev(a.fd, a.vec, a.iov_cnt); > + else > +#endif > + n = (long)custom_writev(a.fd, a.vec, a.iov_cnt, a.iov_len); Please no. CPP conditionals inside functions is confusing, but CPP conditionals mixing with C conditionals makes my eyes bleed. How about something like this? #ifdef HAVE_WRITEV static ssize_t real_writev(int fd, const struct iovec *vec, int cnt, size_t total) { return writev(fd, vec, cnt); } #else # define real_writev(fd,vec,cnt,total) custom_writev((fd),(vec),(cnt),(total)) #endif