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: AS41665 78.109.16.0/20 X-Spam-Status: No, score=-1.8 required=3.0 tests=BAYES_00,RCVD_IN_MSPIKE_BL, RCVD_IN_MSPIKE_ZBI,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 [78.109.23.1]) by dcvr.yhbt.net (Postfix) with ESMTP id 911A320281 for ; Mon, 2 Oct 2017 18:33:55 +0000 (UTC) From: Eric Wong To: spew@80x24.org Subject: [PATCH] Dir.empty? releases GVL Date: Mon, 2 Oct 2017 18:33:39 +0000 Message-Id: <20171002183339.30098-1-e@80x24.org> List-Id: This converts all slow syscalls in the Dir.empty? implementation to release GVL. We avoid unnecessarily GVL release and reacquire for each slow call (opendir, readdir, closedir) and instead only release and acquire the GVL once in the common case. Benchmark results show a small degradation in single-threaded performance: Execution time (sec) name trunk built dir_empty_p 0.689 0.758 Speedup ratio: compare with the result of `trunk' (greater is better) name built dir_empty_p 0.909 * dir.c (rb_gc_for_fd_with_gvl): new function (nogvl_dir_empty_p): ditto (dir_s_empty_p): use new functions to release GVL * benchmark/bm_dir_empty_p.rb: new benchmark --- benchmark/bm_dir_empty_p.rb | 5 ++++ dir.c | 72 ++++++++++++++++++++++++++++++--------------- 2 files changed, 53 insertions(+), 24 deletions(-) create mode 100644 benchmark/bm_dir_empty_p.rb diff --git a/benchmark/bm_dir_empty_p.rb b/benchmark/bm_dir_empty_p.rb new file mode 100644 index 0000000000..8329c757cf --- /dev/null +++ b/benchmark/bm_dir_empty_p.rb @@ -0,0 +1,5 @@ +require 'tmpdir' +max = 100_000 +Dir.mktmpdir('bm_dir_empty_p') do |dir| + max.times { Dir.empty?(dir) } +end diff --git a/dir.c b/dir.c index b7afaec4e0..1412e7c4a8 100644 --- a/dir.c +++ b/dir.c @@ -13,6 +13,7 @@ #include "internal.h" #include "encindex.h" +#include "ruby/thread.h" #include #include @@ -722,6 +723,8 @@ fundamental_encoding_p(rb_encoding *enc) #else # define READDIR(dir, enc) readdir((dir)) #endif + +/* safe to use without GVL */ static int to_be_skipped(const struct dirent *dp) { @@ -2982,6 +2985,46 @@ rb_dir_exists_p(VALUE obj, VALUE fname) return rb_file_directory_p(obj, fname); } +static void * +rb_gc_for_fd_with_gvl(void *ptr) +{ + int *e = ptr; + + return (void *)rb_gc_for_fd(*e); +} + +static void * +nogvl_dir_empty_p(void *ptr) +{ + const char *path = ptr; + DIR *dir = opendir(path); + struct dirent *dp; + VALUE result = Qtrue; + + if (!dir) { + int e = errno; + switch ((int)rb_thread_call_with_gvl(rb_gc_for_fd_with_gvl, &e)) { + default: + dir = opendir(path); + if (dir) break; + e = errno; + /* fall through */ + case 0: + if (e == ENOTDIR) return (void *)Qfalse; + errno = e; /* for rb_sys_fail_path */ + return (void *)Qundef; + } + } + while ((dp = READDIR(dir, NULL)) != NULL) { + if (!to_be_skipped(dp)) { + result = Qfalse; + break; + } + } + closedir(dir); + return (void *)result; +} + /* * call-seq: * Dir.empty?(path_name) -> true or false @@ -2992,9 +3035,7 @@ rb_dir_exists_p(VALUE obj, VALUE fname) static VALUE rb_dir_s_empty_p(VALUE obj, VALUE dirname) { - DIR *dir; - struct dirent *dp; - VALUE result = Qtrue, orig; + VALUE result, orig; const char *path; enum {false_on_notdir = 1}; @@ -3023,28 +3064,11 @@ rb_dir_s_empty_p(VALUE obj, VALUE dirname) } #endif - dir = opendir(path); - if (!dir) { - int e = errno; - switch (rb_gc_for_fd(e)) { - default: - dir = opendir(path); - if (dir) break; - e = errno; - /* fall through */ - case 0: - if (false_on_notdir && e == ENOTDIR) return Qfalse; - rb_syserr_fail_path(e, orig); - } - } - errno = 0; - while ((dp = READDIR(dir, NULL)) != NULL) { - if (!to_be_skipped(dp)) { - result = Qfalse; - break; - } + result = (VALUE)rb_thread_call_without_gvl(nogvl_dir_empty_p, (void *)path, + RUBY_UBF_IO, 0); + if (result == Qundef) { + rb_sys_fail_path(orig); } - closedir(dir); return result; } -- EW