Skip to content

Invalidate constant cache if included module has constants#795

Merged
headius merged 2 commits intojruby:masterfrom
haileys:invalidate-constant-cache-if-included-module-has-constants
Jun 8, 2013
Merged

Invalidate constant cache if included module has constants#795
headius merged 2 commits intojruby:masterfrom
haileys:invalidate-constant-cache-if-included-module-has-constants

Conversation

@haileys
Copy link
Contributor

@haileys haileys commented Jun 8, 2013

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 reviewed Jun 8, 2013
View reviewed changes
test/test_including_module_busts_constant_caches.rb Outdated
Copy link

@vendethiel vendethiel Jun 8, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't that be N ?

Copy link
Contributor Author

@haileys haileys Jun 8, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops you're right. Good catch!

headius added a commit that referenced this pull request Jun 8, 2013
@headius
Merge pull request #795 from charliesome/invalidate-constant-cache-if…
e6288e4 
…-included-module-has-constants

Invalidate constant cache if included module has constants
@headius headius merged commit e6288e4 into jruby:master Jun 8, 2013
@headius
Copy link
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.

@haileys haileys deleted the invalidate-constant-cache-if-included-module-has-constants branch June 8, 2013 16:16
@haileys
Copy link
Contributor Author

haileys commented Jun 8, 2013

@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
Copy link
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?

@haileys
Copy link
Contributor Author

haileys commented Jun 8, 2013

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

haileys/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
Copy link
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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

JRuby 1.7.5

Development

Successfully merging this pull request may close these issues.

3 participants

@haileys @headius @vendethiel