From 594cf720b08a54aabbc1d2ce561fdb5b28bbba0b Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Mon, 29 May 2017 14:40:34 -0400 Subject: [PATCH] Don't open file twice when specified by absolute path. When invoking `require '/a.rb'` (i.e. via an absolute path), ruby generates this sequence of syscalls: open /a.rb fstat64 /a.rb close /a.rb open /a.rb fstat64 /a.rb fstat64 /a.rb read /a.rb close /a.rb It is apparent that the only inherently necessary members of this sequence are: open /a.rb fstat64 /a.rb read /a.rb close /a.rb (the fstat64 isn't *obviously* necessary, but it does serve a purpose and probably shouldn't be removed). The first open/fstat64/close is used to check whether the file is loadable. This is important when scanning the `$LOAD_PATH`, since it is used to determine when a file has been found. However, when we've already unambiguously identified a file before invoking `require`, this serves no inherent purpose, since we can move whatever work is happening as a result of that `fstat64` into the second open/close sequence. This change bypasses the first open/fstat64/close in the case of an absolute path to `require`. It also removes one of the doubled-up `fstat64` calls later in the sequence. As a result, the number of syscalls to require a file changes: * From 8 to 4 when specified by absolute path; * From 8 to 7 otherwise. In future work, it would be possible to re-use the file descriptor opened while searching the `$LOAD_PATH` without the close/open sequence, but this would cause some ugly layering issues. --- file.c | 20 +++++++++++++++++--- internal.h | 1 + load.c | 2 +- ruby.c | 31 ++++++++++++++++++++----------- 4 files changed, 39 insertions(+), 15 deletions(-) diff --git a/file.c b/file.c index c576c85c0a..c26793765a 100644 --- a/file.c +++ b/file.c @@ -5829,8 +5829,8 @@ rb_find_file(VALUE path) return rb_find_file_safe(path, rb_safe_level()); } -VALUE -rb_find_file_safe(VALUE path, int safe_level) +static VALUE +rb_find_file_safe_internal(VALUE path, int safe_level, int defer_load_check) { VALUE tmp, load_path; const char *f = StringValueCStr(path); @@ -5850,7 +5850,9 @@ rb_find_file_safe(VALUE path, int safe_level) if (safe_level >= 1 && !fpath_check(path)) { rb_raise(rb_eSecurityError, "loading from unsafe path %s", f); } - if (!rb_file_load_ok(f)) return 0; + if (!defer_load_check && !rb_file_load_ok(f)) { + return 0; + } if (!expanded) path = copy_path_class(file_expand_path_1(path), path); return path; @@ -5886,6 +5888,18 @@ rb_find_file_safe(VALUE path, int safe_level) return copy_path_class(tmp, path); } +VALUE +rb_find_file_safe(VALUE path, int safe_level) +{ + return rb_find_file_safe_internal(path, safe_level, 0); +} + +VALUE +rb_find_file_safe_with_defer_load_check(VALUE path, int safe_level) +{ + return rb_find_file_safe_internal(path, safe_level, 1); +} + static void define_filetest_function(const char *name, VALUE (*func)(ANYARGS), int argc) { diff --git a/internal.h b/internal.h index e3d5dd8fb7..a54fd9e537 100644 --- a/internal.h +++ b/internal.h @@ -1139,6 +1139,7 @@ void rb_file_const(const char*, VALUE); int rb_file_load_ok(const char *); VALUE rb_file_expand_path_fast(VALUE, VALUE); VALUE rb_file_expand_path_internal(VALUE, VALUE, int, int, VALUE); +VALUE rb_find_file_safe_with_defer_load_check(VALUE path, int safe_level); VALUE rb_get_path_check_to_string(VALUE, int); VALUE rb_get_path_check_convert(VALUE, VALUE, int); void Init_File(void); diff --git a/load.c b/load.c index 284ebf2e48..01c3f25713 100644 --- a/load.c +++ b/load.c @@ -859,7 +859,7 @@ search_required(VALUE fname, volatile VALUE *path, int safe_level) if (loading) *path = rb_filesystem_str_new_cstr(loading); return 'r'; } - if ((tmp = rb_find_file_safe(fname, safe_level)) != 0) { + if ((tmp = rb_find_file_safe_with_defer_load_check(fname, safe_level)) != 0) { ext = strrchr(ftptr = RSTRING_PTR(tmp), '.'); if (!rb_feature_p(ftptr, ext, TRUE, TRUE, &loading) || loading) *path = tmp; diff --git a/ruby.c b/ruby.c index 3f89bb4ae6..c48ab7ed30 100644 --- a/ruby.c +++ b/ruby.c @@ -1883,18 +1883,18 @@ load_file_internal(VALUE argp_v) } static VALUE -open_load_file(VALUE fname_v, int *xflag) +open_load_file(VALUE fname_v, int *xflag, int script) { const char *fname = StringValueCStr(fname_v); long flen = RSTRING_LEN(fname_v); VALUE f; - int e; + int e = 0; + int fd = -1; if (flen == 1 && fname[0] == '-') { f = rb_stdin; } else { - int fd; /* open(2) may block if fname is point to FIFO and it's empty. Let's use O_NONBLOCK. */ #if defined O_NONBLOCK && HAVE_FCNTL && !(O_NONBLOCK & O_ACCMODE) @@ -1920,12 +1920,13 @@ open_load_file(VALUE fname_v, int *xflag) #endif if ((fd = rb_cloexec_open(fname, mode, 0)) < 0) { - int e = errno; + e = errno; if (!rb_gc_for_fd(e)) { - rb_load_fail(fname_v, strerror(e)); + goto fail; } if ((fd = rb_cloexec_open(fname, mode, 0)) < 0) { - rb_load_fail(fname_v, strerror(errno)); + e = errno; + goto fail; } } rb_update_max_fd(fd); @@ -1934,16 +1935,14 @@ open_load_file(VALUE fname_v, int *xflag) /* disabling O_NONBLOCK */ if (fcntl(fd, F_SETFL, 0) < 0) { e = errno; - (void)close(fd); - rb_load_fail(fname_v, strerror(e)); + goto fail; } #endif e = ruby_is_fd_loadable(fd); if (!e) { e = errno; - (void)close(fd); - rb_load_fail(fname_v, strerror(e)); + goto fail; } f = rb_io_fdopen(fd, mode, fname); @@ -1956,6 +1955,16 @@ open_load_file(VALUE fname_v, int *xflag) } } return f; +fail: + if (fd >= 0) (void)close(fd); + if (script) { + /* when we reach this from `$ ruby bad.rb`, show the reason */ + rb_load_fail(fname_v, strerror(e)); + } else { + /* when called from require/load/etc., just show: + * "cannot load such file", same as `load_failed`. */ + rb_load_fail(fname_v, "cannot load such file"); + } } static VALUE @@ -1979,7 +1988,7 @@ load_file(VALUE parser, VALUE fname, int script, ruby_cmdline_options_t *opt) arg.script = script; arg.opt = opt; arg.xflag = 0; - arg.f = open_load_file(rb_str_encode_ospath(fname), &arg.xflag); + arg.f = open_load_file(rb_str_encode_ospath(fname), &arg.xflag, script); return (NODE *)rb_ensure(load_file_internal, (VALUE)&arg, restore_load_file, (VALUE)&arg); } -- 2.11.0 (Apple Git-81)