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

On 30/05/11 11:55 PM, Yusuke ENDOH wrote:
> Hello,
>
> 2011/5/30 Xavier Shay<[e-mail]>:
>>> - Please try to minimize a patch.
>>>    - It would be good to use the existing code as possible as you can.
>> I realise this would be ideal, but I can't figure out how it works, or whether it would be compatible with the new algorithm.
>
> Indeed, load.c is very complex, but I believe that this is because
> load.c is an accumulation of bad know-hows in the long history.
> So we should not throw it away without understanding, I think.
This is a fair point. Perhaps a good first step would be trying to 
refactor it with better variable and method names so that the intent of 
the code can reveal itself.

>
>
>>> - It is slightly disturbing to define a new class with Array inherited.
>>>    Is it really needed?  Is there no alternative approach?
>> I agree and wish there was a good alternative. Unfortunately this is necessary because $LOADED_FEATURES is treated like an array by much existing code (particularly tests), but we need it as a case-insensitive hash for the new algorithm. Perhaps the hash could be exposed directly as a class (like CaseInsensitiveHash), but this means more code, is potentially more complicated, and means we are really changing the type of $LOADED_FEATURES. Thoughts?
>
> Aha, I understand why the inheritance is used.  It is to hook the
> array modification, right?
correct

> It is an impossible approach because some extension library can
> modify $LOADED_FEATURES directly by rb_ary_push.
> I found amalgalite to do so actually:
>
> http://www.google.com/codesearch/p?hl=ja#MyfZfO3_1cw/ext/amalgalite/amalgalite3_requires_bootstrap.c&q=LOADED_FEATURES%20lang:c%20rb_ary_push&l=151
>
>> rb_ary_push( rb_gv_get( "$LOADED_FEATURES" ), require_name );
>
It seems that amalgalite is the only extension to do this in that 
search. While I realise that doesn't cover everything, it seems the 
impact is perhaps small? Would it be possible to instead work with the 
maintainer to fix, and publicize this change. It seems and odd API to be 
supporting.

Xavier