Invalidate constant cache if included module has constants #795

Merged
merged 2 commits into from Jun 8, 2013

Projects

None yet

3 participants

@charliesome
Contributor

JRuby currently does not invalidate the global constant cache serial number if an included module has constants.

This means that cached constant lookups could return incorrect results.

Here's a little test script demonstrating the incorrect behaviour:

A = 1

class Foo
  def self.go
    A
  end
end

module M
  A = 2
end

puts Foo.go # prints 1

Foo.send(:include, M)

puts Foo.go # should print 2, prints 1
@vendethiel vendethiel and 1 other commented on an outdated diff Jun 8, 2013
test/test_including_module_busts_constant_caches.rb
+
+ module N
+ A = 456
+ end
+
+ class Foo
+ include M
+
+ def self.get
+ A
+ end
+ end
+
+ def test_including_module_busts_constant_caches
+ assert_equal 123, Foo.get
+ Foo.send(:include, M)
@vendethiel
vendethiel Jun 8, 2013

shouldn't that be N ?

@charliesome
charliesome Jun 8, 2013 Contributor

Oops you're right. Good catch!

@headius headius merged commit e6288e4 into jruby:master Jun 8, 2013

1 check passed

default The Travis CI build passed
Details
@headius
Member
headius commented Jun 8, 2013

Good find, thanks!

Some day we'll have to (re)explore a non-global way to invalidate constant caches.

@charliesome charliesome deleted the charliesome:invalidate-constant-cache-if-included-module-has-constants branch Jun 8, 2013
@charliesome
Contributor

@headius I've got branches for Rubinius and MRI implementing constant name cache invalidation. I'd be willing to work towards implementing something similar in JRuby.

@headius
Member
headius commented Jun 8, 2013

We can talk about your strategy, certainly. Is there something I can look at online? I am guessing it is still global, but tries to reduce cache thrashing by splitting it up by constant name?

@charliesome
Contributor

Yep pretty much. Here's the diff of the Rubinius implementation:

charliesome/rubinius@master...constant-name-serial

On 09/06/2013, at 3:16, Charles Oliver Nutter notifications@github.com wrote:

We can talk about your strategy, certainly. Is there something I can look at online? I am guessing it is still global, but tries to reduce cache thrashing by splitting it up by constant name?


Reply to this email directly or view it on GitHub.

@headius
Member
headius commented Jun 9, 2013

Yeah, simple enough...if you'd like to attempt a version for JRuby, it shouldn't be any more difficult than for Rubinius. I still wish it didn't have to be global invalidation, but this is an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment