From ee0589dbf66d03d35fc2bc9ae4c17fb6531449c5 Mon Sep 17 00:00:00 2001 From: Burke Libbey Date: Tue, 28 Mar 2017 12:39:53 -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 | 8 +++++--- include/ruby/intern.h | 2 +- load.c | 6 +++--- ruby.c | 30 +++++++++++++++++++----------- 4 files changed, 28 insertions(+), 18 deletions(-) diff --git a/file.c b/file.c index c576c85..f620539 100644 --- a/file.c +++ b/file.c @@ -5826,11 +5826,11 @@ rb_find_file_ext_safe(VALUE *filep, const char *const *ext, int safe_level) VALUE rb_find_file(VALUE path) { - return rb_find_file_safe(path, rb_safe_level()); + return rb_find_file_safe(path, rb_safe_level(), 0); } VALUE -rb_find_file_safe(VALUE path, int safe_level) +rb_find_file_safe(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; diff --git a/include/ruby/intern.h b/include/ruby/intern.h index 9b6a8d7..092ff63 100644 --- a/include/ruby/intern.h +++ b/include/ruby/intern.h @@ -456,7 +456,7 @@ VALUE rb_file_s_absolute_path(int, const VALUE *); VALUE rb_file_absolute_path(VALUE, VALUE); VALUE rb_file_dirname(VALUE fname); int rb_find_file_ext_safe(VALUE*, const char* const*, int); -VALUE rb_find_file_safe(VALUE, int); +VALUE rb_find_file_safe(VALUE, int, int); int rb_find_file_ext(VALUE*, const char* const*); VALUE rb_find_file(VALUE); VALUE rb_file_directory_p(VALUE,VALUE); diff --git a/load.c b/load.c index 284ebf2..954d2a5 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(fname, safe_level, 1)) != 0) { ext = strrchr(ftptr = RSTRING_PTR(tmp), '.'); if (!rb_feature_p(ftptr, ext, TRUE, TRUE, &loading) || loading) *path = tmp; @@ -884,7 +884,7 @@ search_required(VALUE fname, volatile VALUE *path, int safe_level) #else rb_str_cat2(tmp, DLEXT); OBJ_FREEZE(tmp); - if ((tmp = rb_find_file_safe(tmp, safe_level)) != 0) { + if ((tmp = rb_find_file_safe(tmp, safe_level, 0)) != 0) { ext = strrchr(ftptr = RSTRING_PTR(tmp), '.'); if (!rb_feature_p(ftptr, ext, FALSE, TRUE, &loading) || loading) *path = tmp; @@ -897,7 +897,7 @@ search_required(VALUE fname, volatile VALUE *path, int safe_level) if (loading) *path = rb_filesystem_str_new_cstr(loading); return 's'; } - if ((tmp = rb_find_file_safe(fname, safe_level)) != 0) { + if ((tmp = rb_find_file_safe(fname, safe_level, 0)) != 0) { ext = strrchr(ftptr = RSTRING_PTR(tmp), '.'); if (!rb_feature_p(ftptr, ext, FALSE, TRUE, &loading) || loading) *path = tmp; diff --git a/ruby.c b/ruby.c index 3f89bb4..0bb3b77 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 = 0; 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,12 @@ 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)); + goto fail; } } rb_update_max_fd(fd); @@ -1934,16 +1934,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 +1954,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 +1987,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.10.1 (Apple Git-78)