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: Wed, 30 May 2012 20:39:15 +0000 Message-ID: <20120530203915.GB17661@dcvr.yhbt.net> References: <1338386216-14568-1-git-send-email-funny.falcon@gmail.com> <1338386216-14568-3-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 1338410375 2140 80.91.229.3 (30 May 2012 20:39:35 GMT) X-Complaints-To: usenet@dough.gmane.org NNTP-Posting-Date: Wed, 30 May 2012 20:39:35 +0000 (UTC) To: kgio@librelist.com Original-X-From: kgio@librelist.com Wed May 30 22:39:34 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:163 Archived-At: Received: from zedshaw.xen.prgmr.com ([64.71.167.205]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1SZpg5-0002s4-Jd for gclrkg-kgio@m.gmane.org; Wed, 30 May 2012 22:39:34 +0200 Received: from zedshaw.xen.prgmr.com (localhost [IPv6:::1]) by zedshaw.xen.prgmr.com (Postfix) with ESMTP id 4ECFF21D9BB for ; Wed, 30 May 2012 20:47:26 +0000 (UTC) Sokolov Yura 'funny-falcon wrote: All the issues are pretty minor. I can fix them if you don't have time to fix them, but in case you have other revisions... > +#ifndef HAVE_RB_ARY_SUBSEQ > +static inline VALUE rb_ary_subseq(VALUE ary, long idx, long len) > +{ > + VALUE args[2] = {LONG2FIX(idx), LONG2FIX(len)}; > + return rb_ary_aref(2, args, ary); > +} Lately, I've been favoring naming replacement functions differently and using a macro (I need to fix this in tryopen.c, too :x) like this: static inline VALUE my_ary_subseq(VALUE ary, long idx, long len) { ... } #define rb_ary_subseq(ary,idx,len) my_ary_subseq((ary),(idx),(len)) This makes it easier to debug (especially when I'm debugging with optimizations/disabled). > +struct io_args_v { > + VALUE io; > + VALUE buf; > + VALUE vec_buf; > + struct iovec *vec; > + int iov_cnt; Might be better to make iov_cnt long since you're: * assigning RARRAY_LEN to it * comparing it to iov_max (a long) And only cast iov+cnt to int when calling writev(). > + int something_written; > + int fd; > +}; > +static long trim_writev_buffer(struct io_args_v *a, long n) > +{ > + long i, ary_len = RARRAY_LEN(a->buf), str_len = 0; Use one line for each statement if you're declaring + assigning: long i; long ary_len = RARRAY_LEN(a->buf); long str_len = 0; > + VALUE *elem = RARRAY_PTR(a->buf), str = 0; No need to assign str (and if you did, I prefer NULL to zero unlike the MRI team). I think you can just make str local in each block it's used. Needless assigning to zero can waste space on some compilers. Not sure if it's the case with modern GCC, but I remember it was wasteful in GCC at some point in the last decade. > + for (i = 0; n && i < ary_len; i++, elem++) { > + str = *elem; > + str_len = RSTRING_LEN(str); > + n -= str_len; > + if (n < 0) break; > + } Adding comments for my own benefit: /* all done */ > + if (i == ary_len) { > + assert(n == 0 && "writev system call is broken"); > + a->buf = Qnil; > + return 0; > + } /* partially done, remove fully-written buffers */ > + if (i > 0) { > + a->buf = rb_ary_subseq(a->buf, i, ary_len - i); > + } if (i > 0) a->buf = rb_ary_subseq(a->buf, i, ary_len - i); /* setup+replace partially written buffer */ > + if (n < 0) { > + str = rb_str_subseq(str, str_len + n, -n); > + rb_ary_store(a->buf, 0, str); > + } > --- /dev/null > +++ b/ext/kgio/writev_compat.h > @@ -0,0 +1,57 @@ > +/* > + * this header for supporting strange systems which missing writev > + */ > +#if !defined(HAVE_WRITEV) && !defined(writev) > +#define writev writev_supl > +#define iovec iovec_supl I'd name the replacements and define macros the other way around. > +#define WRITEV_MEMLIMIT (2*1024*1024) I'm not sure if the mem limit is a good idea. writev is meant to be atomic (at least to regular files), and the strings are in memory, anyways, I don't think having an extra copy will users hurt badly. Don't need to worry about mmap()'ed regions in Ruby, either. > +#define IOV_MAX 1024 > + > +struct iovec { > + void *iov_base; > + size_t iov_len; > +}; > + > +static ssize_t writev(int fd, const struct iovec *vec, int iov_cnt) > +{ > + int i; > + long result; writev() result should be ssize_t for consistency. > + size_t total = 0; No need to initialize to zero, since it's obviously assigned below. > + const struct iovec *curv = vec; > + char *buf, *cur; > + > + if (iov_cnt == 0) return 0; > + > + i = 1; > + total = curv->iov_len; > + if ( total < WRITEV_MEMLIMIT ) { > + for (curv++; i < iov_cnt; i++, curv++) { > + size_t next = total + curv->iov_len; > + if ( next > WRITEV_MEMLIMIT ) break; minor: no space after "(" or before ")" > + total = next; > + } > + } > + iov_cnt = i; > + > + if (iov_cnt > 1) { > + cur = buf = (char*)malloc(total); > + if (!buf) rb_memerror(); All Rubies with C API provide xmalloc(), use that instead, especially since you're already using rb_memerror(). Also, never need to cast "void *" result to a different pointer type. > + curv = vec; > + for (i = 0; i < iov_cnt; i++, curv++) { > + memcpy(cur, curv->iov_base, curv->iov_len); > + cur += curv->iov_len; > + } > + } else { > + buf = vec->iov_base; > + } > + > + result = (long)write(fd, buf, total); ditto about result being ssize_t, no cast needed > + if (iov_cnt > 1) { > + free(buf); > + } I suggest preserving errno from write(), free() can call brk/sbrk/munmap that fail and set errno. if (iov_cnt > 1) { int save_errno = errno; free(buf); errno = save_errno; } > + def test_monster_writev_wait_writable > + @wr.instance_variable_set :@nr, 0 > + def @wr.kgio_wait_writable > + @nr += 1 > + IO.select(nil, [self]) > + end > + buf = ["." * 1024] * 1024 * 10 > + buf_size = buf.inject(0){|c, s| c + s.size} > + thr = Thread.new { @wr.kgio_writev(buf) } I had trouble with this test on the last assertion. Adding this here helped: Thread.pass until thr.stop? (Using a dual core machine with plenty of memory for skbs). > + readed = @rd.read(buf_size) > + thr.join > + assert_nil thr.value > + assert_equal buf.join, readed > + assert @wr.instance_variable_get(:@nr) > 0 > + end