Skip to content

Removed Class Eval and used define_method instead for the SafeBuffer #10600

Merged
merged 2 commits into from May 14, 2013

7 participants

@aditya-kapoor

Removed the string based Class_eval in favor of the define_method in the SafeBuffer class in ActiveSupport::SafeBuffer class.

@aditya-kapoor

@rafaelfranca It is done and it is not breaking anything...:-)

@rafaelfranca
Ruby on Rails member

@aditya-kapoor thank you. But does it fix the issue at #10598? I not seeing the tests

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff May 13, 2013
...t/lib/active_support/core_ext/string/output_safety.rb
@@ -171,18 +171,15 @@ def encode_with(coder)
end
UNSAFE_STRING_METHODS.each do |unsafe_method|
- if 'String'.respond_to?(unsafe_method)
- class_eval <<-EOT, __FILE__, __LINE__ + 1
- def #{unsafe_method}(*args, &block) # def capitalize(*args, &block)
- to_str.#{unsafe_method}(*args, &block) # to_str.capitalize(*args, &block)
- end # end
-
- def #{unsafe_method}!(*args) # def capitalize!(*args)
- @html_safe = false # @html_safe = false
- super # super
- end # end
- EOT
- end
+ if String.new.respond_to?(unsafe_method)
+ define_method(unsafe_method.to_sym) do |*args, &block|
@rafaelfranca
Ruby on Rails member
rafaelfranca added a note May 13, 2013

no need to call to_sym

@rafaelfranca
Ruby on Rails member
rafaelfranca added a note May 14, 2013

Could you remove this to_sym call? Is it necessary?

OK..removing to_sym method :+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca rafaelfranca and 2 others commented on an outdated diff May 13, 2013
...t/lib/active_support/core_ext/string/output_safety.rb
- to_str.#{unsafe_method}(*args, &block) # to_str.capitalize(*args, &block)
- end # end
-
- def #{unsafe_method}!(*args) # def capitalize!(*args)
- @html_safe = false # @html_safe = false
- super # super
- end # end
- EOT
- end
+ if String.new.respond_to?(unsafe_method)
+ define_method(unsafe_method.to_sym) do |*args, &block|
+ to_str.send(unsafe_method, *args, &block)
+ end
+ define_method("#{unsafe_method}!".to_sym) do |*args|
+ @html_safe = false
+ super(*args)
@rafaelfranca
Ruby on Rails member
rafaelfranca added a note May 13, 2013

No need to pass the args

It would throw up an error if we don't pass in the args to the super function

@carlosantoniodasilva
Ruby on Rails member

Yeah, super with no args doesn't work from define_method =/

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

Ok..working on it..

@aditya-kapoor

@rafaelfranca Do we need to write tests for the Code Refactoring also???

@rafaelfranca
Ruby on Rails member

No. I really don't care about this refactoring. I want to fix #10598, so in order to fix #10598 we should have tests to ensure it was fixed.

@rafaelfranca
Ruby on Rails member

I'm unsure if we should accept this pull request since it seems only cosmetic.

@steveklabnik
Ruby on Rails member

I think there are speed benefits, no? Maybe we should benchmark.

@aditya-kapoor

But it do makes the code clean and easy to read...

@rafaelfranca
Ruby on Rails member

Both are fine to me. If we are going to merge this for speed benefits so we have to benchmark

@egilburg

Tenderlove wrote (http://tenderlovemaking.com/2013/03/03/dynamic_method_definitions.html) a good post on the advantages of define_method over eval with regard to performance. But even other than performance, define_method usually (this case included) involves cleaner and more readable code (e.g. less need for the now-removed comments(*) which were needed previously to easily understand what the method is doing), plus easier to maintain (editors have better time with indenting, syntax highlighting, etc). Plus things like JRuby sometimes are more stable on well-defined dynamic constructs such as define_method, as opposed to carte blanche evals.

IMHO, that the cumulative benefits expands beyond a "cosmetic" change into a bona fide Refactor, and should be welcome in the absense of reasons to not accept it (e.g. evidence of a noticeable performance loss)

(*) I'd still keep the comments though, but this is beside the point.

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff May 14, 2013
...t/lib/active_support/core_ext/string/output_safety.rb
- if 'String'.respond_to?(unsafe_method)
- class_eval <<-EOT, __FILE__, __LINE__ + 1
- def #{unsafe_method}(*args, &block) # def capitalize(*args, &block)
- to_str.#{unsafe_method}(*args, &block) # to_str.capitalize(*args, &block)
- end # end
-
- def #{unsafe_method}!(*args) # def capitalize!(*args)
- @html_safe = false # @html_safe = false
- super # super
- end # end
- EOT
- end
+ if String.new.respond_to?(unsafe_method)
+ define_method(unsafe_method.to_sym) do |*args, &block|
+ to_str.send(unsafe_method, *args, &block)
+ end
@rafaelfranca
Ruby on Rails member
rafaelfranca added a note May 14, 2013

Missing blank line after this

@rafaelfranca
Ruby on Rails member
rafaelfranca added a note May 14, 2013

A blank line here would make the things clear

Where??? between two define_methods???

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

@rafaelfranca @steveklabnik @egilburg I have done the benchmarking thing and here is the gist for it...
https://gist.github.com/aditya-kapoor/5574044

When I ran it on my system, the following where the results for it....

                      user     system      total        real

Define Method 0.610000 0.060000 0.670000 ( 0.677310)
Class Eval 4.950000 0.350000 5.300000 ( 5.315905)

@rafaelfranca
Ruby on Rails member

OMG!!!!!!!!!!!!!!! Thank you

@aditya-kapoor

@rafaelfranca The changes have suggested by you have been added...when would you merge it???

@bogdan
bogdan commented May 14, 2013

@aditya-kapoor you are benchmarking definition time but not runtime. It is different and we should be more focused on runtime. It will be much slower for define_method.
@tenderlove 's article seems fit heavy methods: the case when method calculations is much slower than . or send operation.

Example from ActiveRecord:
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/attribute_methods/read.rb#L53
Still using eval because the performance win for trivial methods is significant because User#name can be called kazillion times during single request.

I am not sure for this particular case e.g. how slow a method defined with define_method will be comparing to downcase operation?

We definitely need to benchmark runtime before we merge it.

@aditya-kapoor

@bogdan I have done the benchmarking for the runtime as well as the define time. You can refer to the gist https://gist.github.com/aditya-kapoor/5574044 and here are the results which were run on my system

                         user     system      total        real

Define Method 0.640000 0.040000 0.680000 ( 0.680650)
Calculating -------------------------------------
Call using define Method

68814 i/100ms

Call using define Method
2324646.0 (±0.7%) i/s - 11629566 in 5.002975s
user system total real
Class Eval 4.970000 0.330000 5.300000 ( 5.309989)
Calculating -------------------------------------
Call using class eval

68715 i/100ms

Call using class eval
2308585.6 (±0.6%) i/s - 11544120 in 5.000716s

As you can see from the results above, there is hardly any difference in the runtime. If you have refered to the article written by @tenderlove this is the one area where the define_method is slightly slower than the class_eval...

The overall time ( defination time + calling time) for define_method is much more than the overall time for class_eval...correct me if I am wrong...

Thanks

@aditya-kapoor

@rafaelfranca @steveklabnik @bogdan @egilburg any words on the benchmarking data???

@rafaelfranca rafaelfranca merged commit 8ce3c1e into rails:master May 14, 2013
@rafaelfranca
Ruby on Rails member

Thank you

@bogdan
bogdan commented May 15, 2013

@aditya-kapoor your last benchmark just benchmarking a String instead of benchmarking two versions of SafeBuffer.

Benchmark.ips do |x|
  x.report("Call using define Method") { "String".capitalize }
end

Benchmark.ips do |x|
  x.report("Call using class eval") { "String".capitalize }
end

The true benchmark:
https://gist.github.com/5582062

Shows arround 26% performance lose.
If this is reasonable cost for code beauty than go for it.
But I doubt this a lot.

#    > git checkout  069ea45; diffbench bench.rb
#
#    Running benchmark with current working tree
#    Checkout HEAD^
#    Running benchmark with HEAD^
#    Checkout to previous HEAD again
#    
#                        user     system      total        real
#    ----------------------------------capitalize in safe buffer
#    After patch:    0.010000   0.000000   0.010000 (  0.009733)
#    Before patch:   0.010000   0.000000   0.010000 (  0.007702)
#    Improvement: -26%
@aditya-kapoor

@bogdan Have you benchmarked for the definition time and memory count....define_method is 793% faster than the class_eval...what difference it would make it one thing is defined about 8 times faster is only 20 % slower than another thing which is 8 times slower and only 20% faster than the first...

All I am saying that the cumulative effect of both definition time + running time for the define_method is better than that for the class_eval....This thing is also stated in the @tenderlove 's article if you have closely examined his results...The define_method is also much better than class_eval in terms of memory consumption and instruction count...

@bogdan
bogdan commented May 15, 2013

If I would have to run: load 'output_safety.rb' 1000 times in my app I would agree with you.
But more likely I will run: ActiveSupport::SafeBuffer#capitalize more often.
So definition time makes zero sense to me. Even if you win that much it will be lost during 10 minutes a running app.

But I agree with memory usage concerns of class_eval, but I still prefer performance over memory consumption.
As I said before: Only for tiny methods like this class_eval is better.

@aditya-kapoor

@bogdan So again, it all depends on the usage and memory consumption but many modern Rubyists are preferring the define_method over the class_eval due to these factors only...If I go by your results, the difference is not so much mind boggling and I guess it is reasonable with respect to Rails...

@thedarkone

@aditya-kapoor how are using Rails if speeding up require "activesupport/lib/active_support/core_ext/string/output_safety.rb" is more important to you than the speed of your template rendering?

@aditya-kapoor

@thedarkone It is about the overall performance that matters to me and I guess this is the way it should be....

@rafaelfranca
Ruby on Rails member

Run time rendering is more important on this case. Reverting.

@rafaelfranca rafaelfranca added a commit that referenced this pull request May 15, 2013
@rafaelfranca rafaelfranca Revert "Merge pull request #10600 from aditya-kapoor/code_refactor"
This reverts commit 8ce3c1e, reversing
changes made to f93da57.

Reason: It slow down the running time.

require "diffbench"
load 'output_safety.rb'

N = 10000
b = ActiveSupport::SafeBuffer.new("hello world")
DiffBench.bm do
  report "capitalize in safe buffer" do
    N.times do
      b.capitalize
    end
  end
end

> git checkout  069ea45; diffbench bench.rb;
diffbench bench.rb;diffbench
bench.rb;diffbench bench.rb;diffbench
bench.rb;diffbench bench.rb;diffbench
bench.rb;

Running benchmark with current working tree
Checkout HEAD^
Running benchmark with HEAD^
Checkout to previous HEAD again

                    user     system      total
                    real
----------------------------------capitalize
in safe buffer
After patch:    0.010000   0.000000   0.010000
(  0.009733)
Before patch:   0.010000   0.000000   0.010000
(  0.007702)
Improvement: -26%

Running benchmark with current working tree
Checkout HEAD^
Running benchmark with HEAD^
Checkout to previous HEAD again

                    user     system      total
                    real
----------------------------------capitalize
in safe buffer
After patch:    0.010000   0.000000   0.010000
(  0.009768)
Before patch:   0.010000   0.000000   0.010000
(  0.007896)
Improvement: -24%

Running benchmark with current working tree
Checkout HEAD^
Running benchmark with HEAD^
Checkout to previous HEAD again

                    user     system      total
                    real
----------------------------------capitalize
in safe buffer
After patch:    0.010000   0.000000   0.010000
(  0.009938)
Before patch:   0.010000   0.000000   0.010000
(  0.007768)
Improvement: -28%

Running benchmark with current working tree
Checkout HEAD^
Running benchmark with HEAD^
Checkout to previous HEAD again

                    user     system      total
                    real
----------------------------------capitalize
in safe buffer
After patch:    0.010000   0.000000   0.010000
(  0.010001)
Before patch:   0.010000   0.000000   0.010000
(  0.007873)
Improvement: -27%

Running benchmark with current working tree
Checkout HEAD^
Running benchmark with HEAD^
Checkout to previous HEAD again

                    user     system      total
                    real
----------------------------------capitalize
in safe buffer
After patch:    0.010000   0.000000   0.010000
(  0.009670)
Before patch:   0.010000   0.000000   0.010000
(  0.007800)
Improvement: -24%

Running benchmark with current working tree
Checkout HEAD^
Running benchmark with HEAD^
Checkout to previous HEAD again

                    user     system      total
                    real
----------------------------------capitalize
in safe buffer
After patch:    0.010000   0.000000   0.010000
(  0.009949)
Before patch:   0.010000   0.000000   0.010000
(  0.007752)
Improvement: -28%
ed738f7
@tenderlove tenderlove added a commit to tenderlove/rails that referenced this pull request May 21, 2013
@tenderlove tenderlove Merge branch 'master' into stmt
* master: (330 commits)
  plugin new missing license spec
  let Ruby do the is_a check for us
  Mocha 0.14.0 was released with MT5 support. Switch back to gem
  Fix named routing regression from 3.2.13
  Revert "just call the class method since we know the callbacks are stored at the"
  test refactor
  Add more data to AR::UnknownAttributeError
  Raise when multiple included blocks are defined
  Revert "Integration tests support the OPTIONS http method"
  restore whitespace in Gemfile between sqlite3 and sprockets
  Revert "Add the options method to action_controller testcase."
  Check if APP_RAKEFILE is defined
  Fix detection of engine in rake db:load_config Broken by d1d7c86
  Remove trailing line break
  tiny types should only be integers when the length is <= 1. fixes #10620
  add failing test exposing mysql adapter tinyint bug
  require things we need
  Revert "Merge pull request #10600 from aditya-kapoor/code_refactor"
  just call the class method since we know the callbacks are stored at the class level
  this variable is used, so we don't have to use double assignments
  ...
f796ed6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.