Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: JRuby 1.1.5
    • Component/s: Core Classes/Modules
    • Labels:
      None
    • Number of attachments :
      2

      Description

      In response to a report from Raphael Valyi I have discovered some crucial flaws in the constant caching logic; some of them may not be reconcilable with the current design.

      The first is when a value previously cached gets shadowed by new value in an intermediate module or class in the inheritance hierarchy:

      BAR = 1
      class Bar; end
      class Foo < Bar
        def bar; BAR; end
      end
      puts Foo.new.bar
      Bar.const_set(:BAR, 2)
      puts Foo.new.bar
      

      This should print 1, then 2, but it prints 1 and 1.

      The second case is when a previously cached value is shadowed by a new value in an intermediate module or class in the containment hierarchy.

      BAR = 1
      class Bar
        class Foo
          def bar; BAR; end
        end
      end
      puts Bar::Foo.new.bar
      Bar.const_set(:BAR, 2)
      puts Bar::Foo.new.bar
      

      The first case seems like it would be caused by bad generation-twiddling. It could probably be fixed by sorting out when generation-twiddling is not happening and making sure it is cascading properly.

      The second case, however, is a more serious problem. Because we do not currently track cref containment from parent to child, there's no way for an update to a cref constant scope to inform containing scopes of the change. We would need to add a new weak list of contained crefs similar to the subclasses list currently in RubyModule.

      If we can't resolve these problems 1.1.5 will have to ship without constant caching. I'm committing a change to temporarily disable constant caching, and I am adding the tests I describe above.

      1. constant_caching_disabled.patch
        1 kB
        Charles Oliver Nutter
      2. jruby_3091.rb
        0.2 kB
        Christian Seiler

        Activity

        Hide
        Charles Oliver Nutter added a comment -

        Patch to disable constant caching. I'm applying this to trunk while we investigate the problems with the caching logic.

        Show
        Charles Oliver Nutter added a comment - Patch to disable constant caching. I'm applying this to trunk while we investigate the problems with the caching logic.
        Hide
        Charles Oliver Nutter added a comment -

        Some bad news...disabling caching entirely with the given patch didn't solve one of rvalyi's issues, though it did solve the other. So constant caching appears to only be responsible for one of the two problems.

        Show
        Charles Oliver Nutter added a comment - Some bad news...disabling caching entirely with the given patch didn't solve one of rvalyi's issues, though it did solve the other. So constant caching appears to only be responsible for one of the two problems.
        Hide
        Christian Seiler added a comment -

        I have some code which breaks current trunk (r7937), too. Works with r7906 (can't test r7907-r7932 because of 3084). I do some metjod aliasing in an eval. I'll try to dig deeper and come up with a test-case

        Show
        Christian Seiler added a comment - I have some code which breaks current trunk (r7937), too. Works with r7906 (can't test r7907-r7932 because of 3084). I do some metjod aliasing in an eval. I'll try to dig deeper and come up with a test-case
        Hide
        Christian Seiler added a comment -

        The attached code should print "M1 and "M2". MRI and JRuby r7906 behave as expected. R7937 doesn't.

        Show
        Christian Seiler added a comment - The attached code should print "M1 and "M2". MRI and JRuby r7906 behave as expected. R7937 doesn't.
        Hide
        Charles Oliver Nutter added a comment -

        I moved Christian's issue to JRUBY-3093 and resolved it. It was unrelated to constant caching.

        Show
        Charles Oliver Nutter added a comment - I moved Christian's issue to JRUBY-3093 and resolved it. It was unrelated to constant caching.
        Hide
        Charles Oliver Nutter added a comment -

        I'm rather discouraged now. I do not believe it will be possible to get the original passive flushing to work. It's possible to get the hierarchies to flush correctly, with about the same complexity as flushing for method table changes. But the cref/staticscope flushing does not seem to be feasible. In order to get the cref hierarchy to flush, we would need to add two things:

        • A weak list from a class/module to all scopes that reference it
        • A weak list from a scope to all child scopes

        Flushing then is iterating all scopes that depend on this class/module and telling them to flush themselves and all child scopes. But since we cache constants based on the number out of the class where we find it, there's no association between a cached constant and a given cref subhierarchy. So even with all this extra bookkeeping, I'm not sure the existing serial number for constants is even enough to verify the validity of a cached constant value. Disappointing.

        I think the best we can do would be to look at a more global cache tied to something other than the structure of the class/module or cref hierarchies. Ruby 1.9 has a single serial number for all such caches, which could be prone to excess flushing. Marcin suggested a per-name serial number...since constant names are unlikely to be duplicated (much) storing per name would rarely cause cached constants to flush. There is a cost to this above and beyond a global serial number, however, since we'd need to store it in a map. So validating a cached constant would involve at least one hash hit, int retrieval, and comparison.

        I'm open to suggestions on the cref hierarchy, but I don't see it as a solvable problem in the 1.1.5 timeframe.

        Show
        Charles Oliver Nutter added a comment - I'm rather discouraged now. I do not believe it will be possible to get the original passive flushing to work. It's possible to get the hierarchies to flush correctly, with about the same complexity as flushing for method table changes. But the cref/staticscope flushing does not seem to be feasible. In order to get the cref hierarchy to flush, we would need to add two things: A weak list from a class/module to all scopes that reference it A weak list from a scope to all child scopes Flushing then is iterating all scopes that depend on this class/module and telling them to flush themselves and all child scopes. But since we cache constants based on the number out of the class where we find it, there's no association between a cached constant and a given cref subhierarchy. So even with all this extra bookkeeping, I'm not sure the existing serial number for constants is even enough to verify the validity of a cached constant value. Disappointing. I think the best we can do would be to look at a more global cache tied to something other than the structure of the class/module or cref hierarchies. Ruby 1.9 has a single serial number for all such caches, which could be prone to excess flushing. Marcin suggested a per-name serial number...since constant names are unlikely to be duplicated (much) storing per name would rarely cause cached constants to flush. There is a cost to this above and beyond a global serial number, however, since we'd need to store it in a map. So validating a cached constant would involve at least one hash hit, int retrieval, and comparison. I'm open to suggestions on the cref hierarchy, but I don't see it as a solvable problem in the 1.1.5 timeframe.
        Hide
        Charles Oliver Nutter added a comment -

        I have implemented a single global constant generation; I think it's enough for now, and I'm not comfortable trying to get a name-based cache implemented this soon before 1.1.5. This one is guaranteed foolproof and should in most cases still cache just fine.

        It would be worth also monitoring whether the generation even cycles during e.g. Rails execution after the application has completely loaded and warmed up. I'd be surprised if it did.

        Show
        Charles Oliver Nutter added a comment - I have implemented a single global constant generation; I think it's enough for now, and I'm not comfortable trying to get a name-based cache implemented this soon before 1.1.5. This one is guaranteed foolproof and should in most cases still cache just fine. It would be worth also monitoring whether the generation even cycles during e.g. Rails execution after the application has completely loaded and warmed up. I'd be surprised if it did.

          People

          • Assignee:
            Charles Oliver Nutter
            Reporter:
            Charles Oliver Nutter
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: