From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS49981 109.236.80.0/20 X-Spam-Status: No, score=-1.8 required=3.0 tests=BAYES_00,RCVD_IN_XBL, RDNS_NONE,SPF_FAIL,SPF_HELO_FAIL,TO_EQ_FM_DOM_SPF_FAIL shortcircuit=no autolearn=no autolearn_force=no version=3.4.0 Received: from 80x24.org (unknown [109.236.90.209]) by dcvr.yhbt.net (Postfix) with ESMTP id 7A2C020372 for ; Thu, 12 Oct 2017 19:12:25 +0000 (UTC) From: Eric Wong To: spew@80x24.org Subject: [PATCH] file.c: apply2files releases GVL Date: Thu, 12 Oct 2017 19:12:18 +0000 Message-Id: <20171012191218.1624-1-e@80x24.org> List-Id: This means File.chmod, File.lchmod, File.chown, File.lchown, File.unlink, and File.utime operations on slow filesystems no longer hold up other threads. The platform-specific utime_failed changes is compile-tested using a new UTIME_EINVAL macro This hurts performance on fast filesystem, but these methods are unlikely to be performance bottlenecks and (IMHO) avoiding pathological slowdowns and stalls are more important. benchmark results: minimum results in each 3 measurements. Execution time (sec) name trunk built file_chmod 0.591 0.801 Speedup ratio: compare with the result of `trunk' (greater is better) name built file_chmod 0.737 * file.c (UTIME_EINVAL): new macro to ease compile-testing * file.c (struct apply_arg): new struct * file.c (no_gvl_apply2files): new function * file.c (apply2files): release GVL * file.c (chmod_internal): adjust for apply2files changes * file.c (lchmod_internal): ditto * file.c (chown_internal): ditto * file.c (lchown_internal): ditto * file.c (utime_failed): ditto * file.c (utime_internal): ditto * file.c (unlink_internal): ditto [ruby-core:83200] [Feature #13996] --- benchmark/bm_file_chmod.rb | 9 +++ file.c | 159 ++++++++++++++++++++++++++++++++------------- 2 files changed, 123 insertions(+), 45 deletions(-) create mode 100644 benchmark/bm_file_chmod.rb diff --git a/benchmark/bm_file_chmod.rb b/benchmark/bm_file_chmod.rb new file mode 100644 index 0000000000..1cd4760c9d --- /dev/null +++ b/benchmark/bm_file_chmod.rb @@ -0,0 +1,9 @@ +# chmod file +require 'tempfile' +max = 200_000 +tmp = Tempfile.new('chmod') +path = tmp.path +max.times do + File.chmod(0777, path) +end +tmp.close! diff --git a/file.c b/file.c index f8b424f2fc..9d014de5b2 100644 --- a/file.c +++ b/file.c @@ -136,6 +136,11 @@ int flock(int, int); # define TO_OSPATH(str) (str) #endif +/* utime may fail if time is out-of-range for the FS [ruby-dev:38277] */ +#if defined DOSISH || defined __CYGWIN__ +# define UTIME_EINVAL +#endif + VALUE rb_cFile; VALUE rb_mFileTest; VALUE rb_cStat; @@ -354,20 +359,87 @@ ignored_char_p(const char *p, const char *e, rb_encoding *enc) #define apply2args(n) (rb_check_arity(argc, n, UNLIMITED_ARGUMENTS), argc-=n) +struct apply_arg { + int i; + int argc; + int errnum; + int (*func)(const char *, void *); + void *arg; + struct { + const char *ptr; + VALUE path; + } fn[1]; /* flexible array */ +}; + +static void * +no_gvl_apply2files(void *ptr) +{ + struct apply_arg *aa = ptr; + + for (aa->i = 0; aa->i < aa->argc; aa->i++) { + if (aa->func(aa->fn[aa->i].ptr, aa->arg) < 0) { + aa->errnum = errno; + break; + } + } + return 0; +} + +#ifdef UTIME_EINVAL +NORETURN(static void utime_failed(struct apply_arg *)); +static int utime_internal(const char *, void *); +#endif + static VALUE -apply2files(void (*func)(const char *, VALUE, void *), int argc, VALUE *argv, void *arg) +apply2files(int (*func)(const char *, void *), int argc, VALUE *argv, void *arg) { - long i; - VALUE path; + VALUE v; + VALUE tmp = Qfalse; + size_t size = sizeof(const char *) + sizeof(VALUE); + const long len = (long)(sizeof(struct apply_arg) + (size * argc) - size); + struct apply_arg *aa = ALLOCV(v, len); + + aa->errnum = 0; + aa->argc = argc; + aa->arg = arg; + aa->func = func; + + /* + * aa is on-stack for small argc, we must ensure paths are marked + * for large argv if aa is on the heap. + */ + if (v) { + tmp = rb_ary_tmp_new(argc); + } + + for (aa->i = 0; aa->i < argc; aa->i++) { + VALUE path = rb_get_path(argv[aa->i]); - for (i=0; ifn[aa->i].ptr = RSTRING_PTR(path); + aa->fn[aa->i].path = path; + + if (tmp != Qfalse) { + rb_ary_push(tmp, path); + } } + rb_thread_call_without_gvl(no_gvl_apply2files, aa, RUBY_UBF_IO, 0); + if (aa->errnum) { +#ifdef UTIME_EINVAL + if (func == utime_internal) { + utime_failed(aa); + } +#endif + rb_syserr_fail_path(aa->errnum, aa->fn[aa->i].path); + } + if (v) { + ALLOCV_END(v); + } + if (tmp != Qfalse) { + rb_ary_clear(tmp); + rb_gc_force_recycle(tmp); + } return LONG2FIX(argc); } @@ -2347,11 +2419,10 @@ rb_file_size(VALUE obj) return OFFT2NUM(st.st_size); } -static void -chmod_internal(const char *path, VALUE pathv, void *mode) +static int +chmod_internal(const char *path, void *mode) { - if (chmod(path, *(int *)mode) < 0) - rb_sys_fail_path(pathv); + return chmod(path, *(int *)mode); } /* @@ -2423,11 +2494,10 @@ rb_file_chmod(VALUE obj, VALUE vmode) } #if defined(HAVE_LCHMOD) -static void -lchmod_internal(const char *path, VALUE pathv, void *mode) +static int +lchmod_internal(const char *path, void *mode) { - if (lchmod(path, (int)(VALUE)mode) < 0) - rb_sys_fail_path(pathv); + return lchmod(path, (int)(VALUE)mode); } /* @@ -2477,12 +2547,11 @@ struct chown_args { rb_gid_t group; }; -static void -chown_internal(const char *path, VALUE pathv, void *arg) +static int +chown_internal(const char *path, void *arg) { struct chown_args *args = arg; - if (chown(path, args->owner, args->group) < 0) - rb_sys_fail_path(pathv); + return chown(path, args->owner, args->group); } /* @@ -2554,12 +2623,11 @@ rb_file_chown(VALUE obj, VALUE owner, VALUE group) } #if defined(HAVE_LCHOWN) -static void -lchown_internal(const char *path, VALUE pathv, void *arg) +static int +lchown_internal(const char *path, void *arg) { struct chown_args *args = arg; - if (lchown(path, args->owner, args->group) < 0) - rb_sys_fail_path(pathv); + return lchown(path, args->owner, args->group); } /* @@ -2593,16 +2661,22 @@ struct utime_args { VALUE atime, mtime; }; -#if defined DOSISH || defined __CYGWIN__ -NORETURN(static void utime_failed(VALUE, const struct timespec *, VALUE, VALUE)); +#ifdef UTIME_EINVAL +NORETURN(static void utime_failed(struct apply_arg *)); static void -utime_failed(VALUE path, const struct timespec *tsp, VALUE atime, VALUE mtime) +utime_failed(struct apply_arg *aa) { - int e = errno; - if (tsp && e == EINVAL) { + int e = aa->errnum; + VALUE path = aa->fn[aa->i].path; + struct utime_args *ua = aa->arg; + + if (ua->tsp && e == EINVAL) { VALUE e[2], a = Qnil, m = Qnil; int d = 0; + VALUE atime = ua->atime; + VALUE mtime = ua->mtime; + if (!NIL_P(atime)) { a = rb_inspect(atime); } @@ -2627,14 +2701,12 @@ utime_failed(VALUE path, const struct timespec *tsp, VALUE atime, VALUE mtime) } rb_syserr_fail_path(e, path); } -#else -#define utime_failed(path, tsp, atime, mtime) rb_sys_fail_path(path) #endif #if defined(HAVE_UTIMES) -static void -utime_internal(const char *path, VALUE pathv, void *arg) +static int +utime_internal(const char *path, void *arg) { struct utime_args *v = arg; const struct timespec *tsp = v->tsp; @@ -2649,9 +2721,9 @@ utime_internal(const char *path, VALUE pathv, void *arg) try_utimensat = 0; goto no_utimensat; } - utime_failed(pathv, tsp, v->atime, v->mtime); + return -1; /* calls utime_failed */ } - return; + return 0; } no_utimensat: #endif @@ -2663,8 +2735,7 @@ no_utimensat: tvbuf[1].tv_usec = (int)(tsp[1].tv_nsec / 1000); tvp = tvbuf; } - if (utimes(path, tvp) < 0) - utime_failed(pathv, tsp, v->atime, v->mtime); + return utimes(path, tvp); } #else @@ -2676,8 +2747,8 @@ struct utimbuf { }; #endif -static void -utime_internal(const char *path, VALUE pathv, void *arg) +static int +utime_internal(const char *path, void *arg) { struct utime_args *v = arg; const struct timespec *tsp = v->tsp; @@ -2687,8 +2758,7 @@ utime_internal(const char *path, VALUE pathv, void *arg) utbuf.modtime = tsp[1].tv_sec; utp = &utbuf; } - if (utime(path, utp) < 0) - utime_failed(pathv, tsp, v->atime, v->mtime); + return utime(path, utp); } #endif @@ -2869,11 +2939,10 @@ rb_readlink(VALUE path, rb_encoding *enc) #define rb_file_s_readlink rb_f_notimplement #endif -static void -unlink_internal(const char *path, VALUE pathv, void *arg) +static int +unlink_internal(const char *path, void *arg) { - if (unlink(path) < 0) - rb_sys_fail_path(pathv); + return unlink(path); } /* -- EW