Skip to content

Commit

Permalink
Initialize to false
Browse files Browse the repository at this point in the history
  • Loading branch information
@tarcieri
tarcieri committed Mar 28, 2015
1 parent 4df69f7 commit 18a125e
Showing 1 changed file with 2 additions and 0 deletions.
2 changes: 2 additions & 0 deletions lib/celluloid.rb
View file
Expand Up @@ -3,6 +3,8 @@
require 'timeout'
require 'set'

$CELLULOID_DEBUG = false

This comment has been minimized.

Sign in to view

Copy link
@e2

e2 Mar 28, 2015

Contributor

@tarcieri - this should be an environment variable (getenv() probably has even less overhead than a global variable in Ruby anyway...). This also broke the tests on master :(

/cc @digitalextremist

This comment has been minimized.

Sign in to view

Copy link
@tarcieri

tarcieri Mar 28, 2015

Author Member

Using InvokeDynamic and SwitchPoints, JRuby can optimize away code guarded by globals (modifying the global invalidates the SwitchPoint), making this zero-cost on JRuby so long as $CELLULOID_DEBUG is set to false.

getenv, on the other hand, likely requires a system call.

The code guarded by $CELLULOID_DEBUG is in the critical path. This particular optimization was recommended by headius.

This comment has been minimized.

Sign in to view

Copy link
@tarcieri

tarcieri Mar 28, 2015

Author Member

@e2 this is where your tone isn't particularly helpful. When you phrase things like this:

"this should be an environment variable (getenv() probably has even less overhead than a global variable in Ruby anyway...)."

You ignore all of the considerations that have gone into these decisions. Perhaps if you're confused about something and wonder why another alternative wasn't used, you could phrase your concerns in the form of a question.

This comment has been minimized.

Sign in to view

Copy link
@e2

e2 Mar 28, 2015

Contributor

@tarcieri - I was wrong. The reasoning could be mentioned somewhere (in the commit message or with a comment in code).

module Celluloid
# Expose all instance methods as singleton methods
extend self
Expand Down

5 comments on commit 18a125e

@e2
Copy link
Contributor

@e2 e2 commented on 18a125e Mar 28, 2015

Choose a reason for hiding this comment

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

@tarcieri - is there a benchmark for this specifically? (One that relies on e.g. suspend() working quickly?)

If not, what would you suggest? (For general refactoring/changes - not just this case).

The ones included in Celluloid (rake benchmark) show no difference on my setup (jruby-head and jruby-1.7.19) (more often than not, using ENV['CELLULOID_DEBUG'] appeared "faster" than with just $CELLULOID_DEBUG).

The only real difference was that jruby-head was slightly slower overall (consistently).

I benchmarked $CELLULOID_DEBUG == true vs ENV['CELLULOID_DEBUG'] using Benchmark.ips and the difference was: ENV was only 2.0 x slower on jruby-head and only 2.5 x slower on MRI 2.2.0.

What do you think?

@digitalextremist
Copy link
Member

Choose a reason for hiding this comment

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

@e2 two of our weak points right now are test consistency and benchmarks. These two areas are where we've had notoriously low contribution levels. We could use help producing granular and regression-checking oriented tests and benchmarks as you seem to be very well suited for.

@tarcieri
Copy link
Member Author

Choose a reason for hiding this comment

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

The benchmark you want to do is specifically on guarded invocation. Something like this:

require 'benchmark/ips'

$ZOMG_GLOBAL = false

Benchmark.ips do |bm|
  bm.report("env") do
    i = 0
    i += 1 if ENV['NONEXISTENT']
  end

  bm.report("global") do
    i = 0
    i += 1 if $ZOMG_GLOBAL
  end
end

Results:

ruby 2.2.0p0 (2014-12-25 revision 49005) [x86_64-darwin14]

Calculating -------------------------------------
                 env   106.246k i/100ms
              global   137.212k i/100ms
-------------------------------------------------
                 env      3.320M (± 2.4%) i/s -     16.681M
              global      9.209M (± 3.4%) i/s -     46.103M

jruby 1.7.18 (1.9.3p551) 2014-12-22 625381c on Java HotSpot(TM) 64-Bit Server VM 1.7.0_45-b18 +jit [darwin-x86_64]

Calculating -------------------------------------
                 env   186.203k i/100ms
              global   254.957k i/100ms
-------------------------------------------------
                 env      8.963M (± 4.5%) i/s -     44.689M
              global     17.320M (± 5.0%) i/s -     86.175M

@e2
Copy link
Contributor

@e2 e2 commented on 18a125e Mar 29, 2015

Choose a reason for hiding this comment

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

Pretty much my results MRI - 2.8 here (2.5 on my setup) and JRuby - 1.9 here (2.0 on my setup).

Method invocation speed (class with method calling method returning instance variable) seems in between the two, so just in general:

would it be ok to refactor out the global variables + put the logging/debugging elsewhere? Or is speed really critical in this regard? Just asking to get a sense of what needs people have out there. (At this point optimizing JRuby internals would probably bring more benefit).

I believe refactoring out would actually help the JVM with inlining optimizations (based on headius's blog article) if classes like Task don't have the debugging to begin with.

@tarcieri
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't really a fan of how that patch was implemented in the first place and would be open to refactorings

Please sign in to comment.