Skip to content

Named based constant cache invalidation#798

Merged
8 commits merged intojruby:masterfrom
haileys:constant-name-cache
Jun 11, 2013
Merged

Named based constant cache invalidation#798
8 commits merged intojruby:masterfrom
haileys:constant-name-cache

Conversation

@haileys
Copy link
Contributor

@haileys haileys commented Jun 11, 2013

Hi!

I've implemented named based constant cache invalidation in JRuby.

In my implementation, I've added a hashtable mapping constant names to Invalidator objects on the Ruby class. The lookup of the Invalidator object is cached in fields of certain classes (example) - I've tried to make this threadsafe, but I'd like my approach verified because I've probably made a mistake somewhere.

I used this script to benchmark the performance improvement of this patch:

require "benchmark"

class A
  Foo = 123

  class B
    class C
      class D
        class E
          def self.get_foo
            Foo
          end
        end
      end
    end
  end
end

Bar = 456

5.times do
  puts Benchmark.measure {
    1_000_000.times do
      Object.send(:remove_const, :Bar)
      Object.const_set(:Bar, 456)
      A::B::C::D::E.get_foo
    end
  }
end

ruby-2.0.0-p0:

  0.930000   0.000000   0.930000 (  0.935408)
  0.940000   0.000000   0.940000 (  0.937916)
  0.970000   0.000000   0.970000 (  0.967160)
  0.940000   0.000000   0.940000 (  0.937358)
  0.930000   0.000000   0.930000 (  0.931662)

JRuby master:

  1.250000   0.010000   1.260000 (  0.865000)
  0.700000   0.000000   0.700000 (  0.670000)
  0.700000   0.010000   0.710000 (  0.686000)
  0.670000   0.000000   0.670000 (  0.677000)
  0.680000   0.000000   0.680000 (  0.679000)

JRuby constant-name-cache:

  1.250000   0.010000   1.260000 (  0.794000)
  0.620000   0.000000   0.620000 (  0.589000)
  0.580000   0.010000   0.590000 (  0.572000)
  0.570000   0.000000   0.570000 (  0.568000)
  0.570000   0.010000   0.580000 (  0.570000)

✨

@headius
Copy link
Member

headius commented Jun 11, 2013

Mostly good suggestions from @wmeissner. Summing up some points:

  1. ConcurrentHashMap would be good. It will be mostly read, so we can set it to concurrency level of 1 and it won't be too big then.
  2. When a new invalidator is created, appropriate atomic operations must be performed to guarantee exactly one is ever created. If two get created, you could see threads stomp on each other and someone cache an invalidator that will never be invalidated. The synchronized modifier on getConstantValidator will do this, but we don't want to synchronize on every read either. CHM.putIfAbsent should do the trick. (Note: This may apply to your Rubinius patch as well).
  3. I don't think the invalidator fields need to be volatile; worst case is they get set by multiple threads, but as long as (2) holds, they'll just end up with the same invalidator anyway. And CHM will guarantee volatility of the invalidator state (happens-before).

We can attempt to make these changes for you, or if you like you can try them yourself. They're not major, and the patch looks good otherwise.

@haileys
Copy link
Contributor Author

haileys commented Jun 11, 2013

@wmeissner @headius Thanks for the fantastic feedback!

I've gone ahead and implemented your suggestions, also I've removed the synchronization from the invalidator accessor methods on the AST and IR classes because I figure if multiple threads race on this, the worst that can happen is invalidator is set twice. Let me know what you think.

@headius
Copy link
Member

headius commented Jun 11, 2013

Looks good! I'll get this merged and play with it a bit.

One note about the benchmark: We don't handle caching Colon2 as well as we could, so there could be considerably more improvement if we did that. Your improvement here could make the entire :: chain optimize away to nothing and never invalidate...so that's motivation for me to make another attempt.

@ghost ghost merged commit 79dad3e into jruby:master Jun 11, 2013
@headius
Copy link
Member

headius commented Jun 11, 2013

Merged in 79dad3e. Thanks! Will have to try improving the perf of Colon2 logic in the compiler now.

This pull request was closed.
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.

2 participants

@haileys @headius