$LOADED_FEATURES is updated before loading finished

The patch doesn't preserve the intent of the synchronization on loadedFeaturesInternal (see JRUBY-3078), but I think that was obsoleted by the fix for JRUBY-3343 and can be removed.

Here's a RubySpec for this behavior.

Wow, what a quick reply! Thanks.

Yes, the patch is too ad-hoc to apply. LoadService#tryLoadingLibraryOrScript() is called only from LoadService#smartLoad() which is public, and this looks to be called only for loading features specified at command line. I agree with you that removing this lock with warning comment 'MT-unsafe!'.

Do you have any idea about the original issue on loadedFeatures?

Yes, the issue is that although $LOADED_FEATURES should not be updated until loading is finished, circular requires should still be short circuited. On JRuby, there's currently no mechanism to prevent circular requires other than $LOADED_FEATURES. Rubinius accomplishes it by maintaining a separate map of currently loading files; presumably MRI does something similar.

Thanks! I agree with you that circular require is the problem I'm getting with the ad-hoc fix. Let me try to fix it if you've not yet fixed it.

FYI: Here's a branch for this issue: https://github.com/jruby/jruby/commits/require_protection
Still in development.

Now require_protection branch passes ant spec and ant test for me. Can someone review it?
https://github.com/jruby/jruby/compare/c8fedfbc...a0a4fab5 (compare)
https://github.com/jruby/jruby/commits/require_protection (branch)

Nahi, I did a short review of this and did not notice anything wrong. It is a largish set of changes though so it is difficult to know how safe this is. I recommend that you land this on master and if we discover no issues in the next couple of weeks we can consider merging on 1.6 branch for a future 1.6.x release.

Does this preserve the intent of JRUBY-3343? My read is that if two threads require the same file at the same moment, one of them is going to think it's a circular require. The desired behavior from JRUBY-3343 is for one to block until the other completes the require.

Thanks for swift response!

John: No, it doesn't preserve it I think. I'll write a test and try to fix it. Hmm, it must be a challenge of anti-deadlock; coordinating 2 locks for file and the load result. CRuby has GVL so they're free from this problem?

Thomas: I'll merge it to master once I solved above degrade. I didn't think of merging it to 1.6 branch. It might be delayed till another user requesting it.

I fixed the above issue at [require_protection f6555aeb] and merged at [master 447793c4].
But I found that bench/bench_time_require.rb slows down with this merge... I'm investigating it.

Not related to the fix.

Before:

% jruby bench/bench_time_require.rb 
                                    user     system      total        real
require 'time'                  1.093000   0.000000   1.093000 (  1.048000)
require 'time'                  0.302000   0.000000   0.302000 (  0.302000)
require 'time'                  0.162000   0.000000   0.162000 (  0.162000)
Now with rubygems and activerecord
DEPRECATION WARNING: require "activerecord" is deprecated and will be removed in Rails 3. Use require "active_record" instead. (called from require at /home/nahi/git/jruby/lib/ruby/gems/1.8/gems/activerecord-2.3.11/lib/activerecord.rb:32)
                                    user     system      total        real
require 'time'                  5.645000   0.000000   5.645000 (  5.646000)
require 'time'                  3.837000   0.000000   3.837000 (  3.836000)
require 'time'                  3.614000   0.000000   3.614000 (  3.614000)

After:

% jruby bench/bench_time_require.rb
                                    user     system      total        real
require 'time'                  1.102000   0.000000   1.102000 (  1.056000)
require 'time'                  0.248000   0.000000   0.248000 (  0.248000)
require 'time'                  0.129000   0.000000   0.129000 (  0.129000)
Now with rubygems and activerecord
DEPRECATION WARNING: require "activerecord" is deprecated and will be removed in Rails 3. Use require "active_record" instead. (called from require at /home/nahi/git/jruby/lib/ruby/gems/1.8/gems/activerecord-2.3.11/lib/activerecord.rb:32)
                                    user     system      total        real
require 'time'                  5.806000   0.000000   5.806000 (  5.806000)
require 'time'                  3.960000   0.000000   3.960000 (  3.960000)
require 'time'                  3.565000   0.000000   3.565000 (  3.565000)

bench/bench_time_require.rb requires 'activerecord' which is deprecated and only in 2.X version. Changing it to 'active_record' fixes this issue.

% jruby bench/bench_time_require.rb
                                    user     system      total        real
require 'time'                  1.111000   0.000000   1.111000 (  1.069000)
require 'time'                  0.243000   0.000000   0.243000 (  0.243000)
require 'time'                  0.134000   0.000000   0.134000 (  0.134000)
Now with rubygems and activerecord
                                    user     system      total        real
require 'time'                  0.439000   0.000000   0.439000 (  0.439000)
require 'time'                  0.222000   0.000000   0.222000 (  0.222000)
require 'time'                  0.191000   0.000000   0.191000 (  0.191000)

It's not related to the original issue of this ticket but it looks interesting, doesn't it? Length of $LOADED_FEATURES? I'll investigate more and open another ticket if I find something.

I keep this ticket open for a decision; merge to jruby-1_6? As I said before, it might be delayed until another user requests it.

John, at require_protection branch[1], I modified your commit[2] for caseInsensitiveFS for performance. Here's the one[3]. Would you please check it?

[1] https://github.com/jruby/jruby/commits/require_protection
[2] https://github.com/jruby/jruby/commit/a3afa944b9d5c0f4f6c42d491b84c1dc4182e989
[3] https://github.com/jruby/jruby/commit/a315f35068bbf4cfb71402b67d1e2aee663a9fc0

Yeah, you've effectively reverted my change. It would make require 10%+ slower on case insensitive filesystems.

Iterating over the array in a Java style (using Iterator or a for) forces all the RubyStrings in that array to be converted to Java Strings, which is very expensive.

What prompted that part of the change? Can it be avoided?

Thanks for the explanation. Now I've understood the intent of your patch!

Accessing to loadedFeatures directly would cause multi-threaded issue I thought. Your patch depends on the fact that $LOADED_FEATURES is monotonic increase, right?

I'll find another way...

For others: the caseInsensitiveFS change is not merged yet (So master must be OK).

I checked the CRuby 187 behavior on Windows.

>copy con Foo.rb
p :Foo
^Z

>ruby187 -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES"
:Foo
:Foo
:Foo
["enumerator.so", "FOO.rb", "Foo.rb", "foo.rb"]

>ruby192 -I. -rFOO -e "
p $LOADED_FEATURES"
:Foo
[blah, blah, "C:/Users/nahi/Documents/Foo.rb"]

>jruby156 -I. -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES"
:Foo
:Foo
:Foo
["builtin/core_ext/symbol.rb", "enumerator.jar", "FOO.rb", "Foo.rb", "foo.rb"]

>jruby156 -I. --1.9 -rFOO -rFoo -rfoo -e "p $LOADED_FEATURES"
:Foo
[blah, blah, "C:/Users/nahi/Documents/Foo.rb"]

So it might be enough to remove whole caseInsensitiveFS condition for compatibility. JRuby should require it 3 times with 1.8 mode, 1 time with 1.9 mode.

John, can you try the change (remove caseInsensitiveFS block) on your env to measure performance change?

Certainly removing the caseInsensitiveFS branch entirely will be even better than my change... not sure if it can be done, check JRUBY-5320.

I'm not sure that the synchronization on loadedFeatures is necessary at all given the other synchronization changes that have been added since then. Note that iterating over a concurrent collection using an Iterator provides only weak concurrency guarantees – it's probably the same with a `for (Object str : loadedFeaturesInternal)` loop. So my guess is that if synchronization is necessary there, it needs to be an explicit `synchronized(loadedFeatures)` anyway. But as I said, I'm skeptical that it is necessary.

Thanks for confirmation. For case-sensitivity, I think JRUBY-5320 and http://redmine.ruby-lang.org/issues/show/4255 both should be reverted. $LOADED_FEATURES includes Strings given from FS so it should not be an issue even if case-insensible FS. I'm asking this to the assigned CRuby commiter. I'll write again.

For synchronization, using loadedFeatureInternal should be safe. But as you said simple synchronization is better if we should iterate it. Synchronization is needed because we have public LoadService#removeInternalLoadedFeature() which does not share a lock for require. I hope we can drop removeInternalLoadedFeature()...

I merged require_protection branch to master at d54bb64ee61fb46b3ed2836f76c25dc9d9ce1a2d.
That branch also included autoload thread issue(JRUBY-3194), too and I think it's a little big change for maintenance branch.

I close this ticket as 'Fixed in 1.7' but feel free to reopen it if you need this fix for jruby-1_6 as well.