Issue #3924 has been updated by Yusuke Endoh.


Hello,

2011/5/31 Xavier Shay <xavier-list / rhnh.net>:
> Patches:   https://gist.github.com/58dbd6e72c1a1f47a415

Awesome!


> In addition I have addressed the following concerns from Yusuke:
> - Please use 4 space for indent, with 8 space tab. (Emacs-style)

Your patch still seems to use 8-space (1-tab) indent.

Yours:

+static VALUE
+rb_locate_file(VALUE filename)
+{
+        VALUE full_path = Qnil;
 ~~~~~~~~
 1 tab


Expected:

+static VALUE
+rb_locate_file(VALUE filename)
+{
+    VALUE full_path = Qnil;
 ~~~~
 4 spaces

But okay, this is a minor problem.  We can fix it easily later.


> Yusuke wrote:
> "It is an impossible approach because some extension library can
> modify $LOADED_FEATURES directly by rb_ary_push."
>
> The hooks are only used to keep a cache up to date. If we get a cache-miss, we could just revert to scanning $LOADED_FEATURES again which would pick up any items that were added with rb_ary_push.

Sorry I cannot get your point.

The following program should not load t.rb, should it?

  $"[$".size] = File.expand_path("./t.rb")
  require("./t")

However, this actually load t.rb after your patch is applied:

  $ cat t.rb
  p :loaded

  $ ./ruby -I. -e '
  $"[$".size] = File.expand_path("./t.rb")
  require("./t")
  '
  :loaded

This is considerable incompatiblity, I think.

Of course, you can change LoadedFeaturesProxy to hook Array#[]=.
But you cannot hook rb_ary_push.  This is my concern.


> This would also mean the addition of `lib/enumerator.rb` in my patch would be unnecessary (though IMO probably still a good idea).

Yes, it is a good idea, and will be accepted eventually.
But I'm afraid if it is not the timing to do so.
And we should discuss it separately.


> = Benchmarks
>
> Yusuke I think I mislead you with the benchmarks. I do not expect full_load_path_benchmark.rb to be faster. I was only using it to ensure I didn't make that case any worse. See below for further details.

Okay, I got it.  Sorry for my obtuseness.
However, I don't think that the incompatibility is ignorable.
At least, I have no right to decide it.

Matz and Yugui, what do you think?

-- 
Yusuke Endoh <mame / tsg.ne.jp>
----------------------------------------
Bug #3924: Performance bug (in require?)
http://redmine.ruby-lang.org/issues/3924

Author: Carsten Bormann
Status: Open
Priority: Normal
Assignee: 
Category: core
Target version: 1.9.3
ruby -v: -


=begin
 Running irb < /dev/null in 1.9.2 causes 3016 calls to lstat64.
 
 For instance, there is a sequence of 28 repetitions each of lstat calls to all 6 non-empty path prefixes of /opt/local/lib/ruby1.9/1.9.1/irb.rb -- a total of 170 lstats apparently just to load this file; another set of lstats then occurs later for another 18 (times 6) times.  Clearly, something is running amok in the calling sequence rb_require_safe -> realpath_rec -> lstat.
 
 Another example: Running a simple test with the baretest gem causes 17008 calls to lstat.  According to perftools.rb, 80 % of the 1.2 seconds of CPU is used in Kernel#gem_original_require (and another 12 in GC, some of which may be caused by this).
=end



-- 
http://redmine.ruby-lang.org