[ruby-core:36647] Re: [Ruby 1.9 - Bug #3924] Performance bug (in require?)

On 1/06/11 12:24 AM, Yusuke Endoh wrote:
>> 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.
Sorry I misunderstood what you were asking for. Too long working with 
ruby 2 spaces, I have forgotten what tabbing looks like :)

After other things are resolved I will go through and ensure all of 
load.c is formatted correctly.

>
>
>> 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.
I was thinking of the case where non-file features are pushed to the 
array (such as enumerator.so and almagamite).
     rb_ary_push($LOADED_FEATURES, 'enumerator.so')
     require 'enumerator'

Perhaps a scan of the load path to cover this case woudn't be too bad? 
(Not in my patch, just thinking out loud).

This does not cover your case though so perhaps not a great idea:
     rb_ary_push($LOADED_FEATURES, File.expand_path("./t"))
     require "./t"

Two other options that maybe you will like:
1) store some metadata against entries in $LOADED_FEATURES to indicate 
whether they have been cached. rb_ary_push will not set this, so a quick 
scan can be made of $LOADED_FEATURES on require to see if a recache is 
needed
2) On require, recache if the count of $LOADED_FEATURES does not match 
the count of the cache. This isn't 100% correct --- you could rb_ary_pop 
then rb_ary push to work around it --- but perhaps if more acceptable?

I think #2 if the limitation is acceptable. Items added by rb_ary_push 
would be located on the filesystem if possible (such as "./t" in your 
example), other wise left as is (such as "enumerator.so").

>
>> 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.
ok

Thanks,
Xavier