Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: JRuby 1.6.6, JRuby 1.6.7
    • Fix Version/s: JRuby 1.6.8
    • Labels:
      None
    • Environment:
      not OS related. We reproduced this on a Mac OSX Snow Leopard
    • Testcase included:
      yes
    • Number of attachments :
      2

      Description

      With the d9426515f8497c635d3aeebdc8d12c6105bf6ab8 changeset we see about 20-25% drop in performance for a specific case where objects extend a module via active_support/core_ext/object/metaclass.

      We have a ruby test script is attached to allow you to reproduce the problem. We ran our test script against 1.6.7 and 1.6.7 with the d94265 changeset removed and saw 20% performance improvement.

      Attached files:

      • autoload_bug.zip: a test script to reproduce performance issue, run test.rb
      • debug.patch: there are some debug output patched to 1.6.7 branch, and if you built a complete jar with this patch, and run the test, you should see that class C is never mark as autoloaded.

        Activity

        Hide
        Li Xiao added a comment -

        Cool, thanks for looking into this.

        Show
        Li Xiao added a comment - Cool, thanks for looking into this.
        Hide
        Hiroshi Nakamura added a comment -

        Let me give it a try.

        Show
        Hiroshi Nakamura added a comment - Let me give it a try.
        Hide
        Hiroshi Nakamura added a comment -
        diff --git a/src/org/jruby/RubyKernel.java b/src/org/jruby/RubyKernel.java
        index 241aded..a4c3708 100644
        --- a/src/org/jruby/RubyKernel.java
        +++ b/src/org/jruby/RubyKernel.java
        @@ -195,10 +195,13 @@ public class RubyKernel {
                     }
         
                     public void load(Ruby runtime) {
        -                if (runtime.getLoadService().lockAndRequire(file())) {
        -                    // Do not finish autoloading by cyclic autoload 
        -                    module.finishAutoload(baseName);
        -                }
        +                runtime.getLoadService().lockAndRequire(file());
        +                // From jruby 1.7 it calls finishAutoload if the above require
        +                // was NOT a cyclic autoload. But since there's no cyclic
        +                // detection in jruby-1_6, when lockAndRequire returns from
        +                // LoadService it means that autoload is finished. So it's safe
        +                // to call finishAutoload here.
        +                module.finishAutoload(baseName);
                     }
                 });
                 return runtime.getNil();
        

        Can someone try this patch?

        Show
        Hiroshi Nakamura added a comment - diff --git a/src/org/jruby/RubyKernel.java b/src/org/jruby/RubyKernel.java index 241aded..a4c3708 100644 --- a/src/org/jruby/RubyKernel.java +++ b/src/org/jruby/RubyKernel.java @@ -195,10 +195,13 @@ public class RubyKernel { } public void load(Ruby runtime) { - if (runtime.getLoadService().lockAndRequire(file())) { - // Do not finish autoloading by cyclic autoload - module.finishAutoload(baseName); - } + runtime.getLoadService().lockAndRequire(file()); + // From jruby 1.7 it calls finishAutoload if the above require + // was NOT a cyclic autoload. But since there's no cyclic + // detection in jruby-1_6, when lockAndRequire returns from + // LoadService it means that autoload is finished. So it's safe + // to call finishAutoload here. + module.finishAutoload(baseName); } }); return runtime.getNil(); Can someone try this patch?
        Hide
        Babar added a comment - - edited

        Hi Hiroshi,

        Your performance patch is OK with my thread safe Rails 3.2 application.
        Thanks you !

        Does this mean that JRuby 1.6.8 will be released ?

        Show
        Babar added a comment - - edited Hi Hiroshi, Your performance patch is OK with my thread safe Rails 3.2 application. Thanks you ! Does this mean that JRuby 1.6.8 will be released ?
        Hide
        Hiroshi Nakamura added a comment -

        I've just committed to jruby-1_6 branch.

        commit 465bc21af418ba7b874c3c7f6444ea4a3ca58a91
        Author: Hiroshi Nakamura <nahi@ruby-lang.org>
        Date:   Mon Apr 16 22:01:56 2012 +0900
        
            JRUBY-6580: Fix performance drop with threadsafe autoload
            
            In jruby-1_6 branch, end of autoload require was not detected correctly.
            It caused unnecesarry locking for constant access even after autoload
            finished.
        

        Babar, thanks for the confirmation. I think Tom will push 1.6.8 at some point but I'm not sure about the date. For a while, please use 1.6.8.dev at http://ci.jruby.org/snapshots/release/ after the commit reach there.

        Show
        Hiroshi Nakamura added a comment - I've just committed to jruby-1_6 branch. commit 465bc21af418ba7b874c3c7f6444ea4a3ca58a91 Author: Hiroshi Nakamura <nahi@ruby-lang.org> Date: Mon Apr 16 22:01:56 2012 +0900 JRUBY-6580: Fix performance drop with threadsafe autoload In jruby-1_6 branch, end of autoload require was not detected correctly. It caused unnecesarry locking for constant access even after autoload finished. Babar, thanks for the confirmation. I think Tom will push 1.6.8 at some point but I'm not sure about the date. For a while, please use 1.6.8.dev at http://ci.jruby.org/snapshots/release/ after the commit reach there.

          People

          • Assignee:
            Hiroshi Nakamura
            Reporter:
            Li Xiao
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: