New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AS::Callbacks remove useless code, improve performance #6351

Merged
merged 1 commit into from May 17, 2012

Conversation

Projects
None yet
6 participants
@bogdan
Contributor

bogdan commented May 16, 2012

This patch is yet another small step to make Callbacks a library of my dream.

Benchmark:
https://gist.github.com/2710476

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

                    user     system      total        real
-----------------------------------------------set_callback
After patch:    0.010000   0.000000   0.010000 (  0.014398)
Before patch:   0.030000   0.000000   0.030000 (  0.031771)

-------------------------------------------define_callbacks
After patch:    0.020000   0.000000   0.020000 (  0.011218)
Before patch:   0.010000   0.000000   0.010000 (  0.011084)

----------------------------------------------run_callbacks
After patch:    0.000000   0.000000   0.000000 (  0.003216)
Before patch:   0.000000   0.000000   0.000000 (  0.003345)

----------------------------------------------skip_callback
After patch:    0.010000   0.000000   0.010000 (  0.012708)
Before patch:   0.020000   0.000000   0.020000 (  0.017077)
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy May 16, 2012

Member

What was the point of undef_method on the runner method? Is it safe to remove?

another small step to make Callbacks a library of my dream.

❤️❤️❤️

Member

jeremy commented May 16, 2012

What was the point of undef_method on the runner method? Is it safe to remove?

another small step to make Callbacks a library of my dream.

❤️❤️❤️

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan May 16, 2012

Contributor

I supposed that no one will ask :), but you did so..
Warning: understanding a story below may seriously drain your today stamina.

Dynamically generated method acts as cache for callback chain code.
Previously this cache was generated on per class basics. So every class had it's own runner method:

-        "_run__#{self.name.hash.abs}__#{kind}__callbacks"
#                        ^ class name

Every time we update callbacks - a cache method should be undefined - what __reset_runner was doing.

(I can assume that this use case is pretty esoteric. This method is only generated after first run of callbacks. So undefine runner can only be valuable if someone changes callbacks after initializaton process finishes somewhere later).

But that is a past.

Now it's generated on per chain basis.

+        name = "_run_callbacks_#{chain.object_id}"

At first it means that when different classes has same chain object - they will share runner method as well.
At second it means that do not need to flush a cache - as when callback chain gets updated it's object_id changes and new callback method will be generated.(which is still pretty esoteric but possible)

Contributor

bogdan commented May 16, 2012

I supposed that no one will ask :), but you did so..
Warning: understanding a story below may seriously drain your today stamina.

Dynamically generated method acts as cache for callback chain code.
Previously this cache was generated on per class basics. So every class had it's own runner method:

-        "_run__#{self.name.hash.abs}__#{kind}__callbacks"
#                        ^ class name

Every time we update callbacks - a cache method should be undefined - what __reset_runner was doing.

(I can assume that this use case is pretty esoteric. This method is only generated after first run of callbacks. So undefine runner can only be valuable if someone changes callbacks after initializaton process finishes somewhere later).

But that is a past.

Now it's generated on per chain basis.

+        name = "_run_callbacks_#{chain.object_id}"

At first it means that when different classes has same chain object - they will share runner method as well.
At second it means that do not need to flush a cache - as when callback chain gets updated it's object_id changes and new callback method will be generated.(which is still pretty esoteric but possible)

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim May 16, 2012

Contributor

So the trade-off here is that we are sharing callback chains between child and parent (good) at the cost that callbacks dynamically updated will be recompiled under a new method without expiring the previous one?

This seems risky, it means that someone doing something crazy (adding and/or removing callbacks in a request lifecycle) will now have a memory leak. Not sure if we should support these cases, but the hole will be there.

Contributor

josevalim commented May 16, 2012

So the trade-off here is that we are sharing callback chains between child and parent (good) at the cost that callbacks dynamically updated will be recompiled under a new method without expiring the previous one?

This seems risky, it means that someone doing something crazy (adding and/or removing callbacks in a request lifecycle) will now have a memory leak. Not sure if we should support these cases, but the hole will be there.

@bogdan

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan May 17, 2012

Contributor

You will get memory leak if you update callbacks in runtime in any case with or without a patch

def create
  User.after_save, if: "some_madness" { puts 'saved' }  
end

This will cause continuous add of conditions into callbacks options hash.

But, the crazy case:

def update
  u = User.first
  u.singleton_class.after_save, if: "some_madness" { puts 'saved' }  
end

(I remember someone mentioned this use case once)
Won't cause memory leak because singleton class will be GCed with User object in both cases as well.

So, that memory leak exists right now. This patch is maybe making leak more but not 10 times more.

Alternatively we can use chain.hash.abs instead of chain.object_id to identify callback runner method name.
This will make less runners to be generated. What do you think?

Contributor

bogdan commented May 17, 2012

You will get memory leak if you update callbacks in runtime in any case with or without a patch

def create
  User.after_save, if: "some_madness" { puts 'saved' }  
end

This will cause continuous add of conditions into callbacks options hash.

But, the crazy case:

def update
  u = User.first
  u.singleton_class.after_save, if: "some_madness" { puts 'saved' }  
end

(I remember someone mentioned this use case once)
Won't cause memory leak because singleton class will be GCed with User object in both cases as well.

So, that memory leak exists right now. This patch is maybe making leak more but not 10 times more.

Alternatively we can use chain.hash.abs instead of chain.object_id to identify callback runner method name.
This will make less runners to be generated. What do you think?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim May 17, 2012

Contributor

Yeah, since the correct is to modify the singleton class (you won't be thread safe if you modify the class), I am assuming this fine. /cc @jeremy can we merge?

Contributor

josevalim commented May 17, 2012

Yeah, since the correct is to modify the singleton class (you won't be thread safe if you modify the class), I am assuming this fine. /cc @jeremy can we merge?

@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy May 17, 2012

Member

+1 to merge. Thank you for expending your stamina @bogdan ;)

Member

jeremy commented May 17, 2012

+1 to merge. Thank you for expending your stamina @bogdan ;)

josevalim added a commit that referenced this pull request May 17, 2012

Merge pull request #6351 from bogdan/callbacks
AS::Callbacks remove useless code, improve performance

@josevalim josevalim merged commit ad8b0a4 into rails:master May 17, 2012

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca
Member

rafaelfranca commented May 17, 2012

@spastorino

This comment has been minimized.

Show comment
Hide comment
@spastorino
Member

spastorino commented on 30b31f5 May 17, 2012

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan May 17, 2012

Contributor

Never though object_id could be negative. We should do abs in there.

Contributor

bogdan replied May 17, 2012

Never though object_id could be negative. We should do abs in there.

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus May 17, 2012

Member

@bogdan wouldn't it be better to tr '-' into '_' ? I know that's highly unlikely, but abs could give the same object_id as other object. I don't know what are the chances, though, maybe that's something that we can ignore.

Member

drogus replied May 17, 2012

@bogdan wouldn't it be better to tr '-' into '_' ? I know that's highly unlikely, but abs could give the same object_id as other object. I don't know what are the chances, though, maybe that's something that we can ignore.

This comment has been minimized.

Show comment
Hide comment
@spastorino

spastorino May 17, 2012

Member

Yep when I first saw this I thought the same thing @drogus is mentioning. @bogdan can you provide a PR to fix this? thanks :)

Member

spastorino replied May 17, 2012

Yep when I first saw this I thought the same thing @drogus is mentioning. @bogdan can you provide a PR to fix this? thanks :)

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan May 18, 2012

Contributor
Contributor

bogdan replied May 18, 2012

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim May 18, 2012

Contributor
Contributor

josevalim replied May 18, 2012

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan May 18, 2012

Contributor

Good idea. But if cache will be done in instance variable - this will require to flush the cache in initialize_dup or thing like this so that dup object won't share runner method name

Contributor

bogdan replied May 18, 2012

Good idea. But if cache will be done in instance variable - this will require to flush the cache in initialize_dup or thing like this so that dup object won't share runner method name

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 1, 2012

Member

@bogdan this commit make the Active Model suite to brake sometimes. Reverting this commit all the tests pass.

I used this script to run the suite many times.

With this commit: 50 times, 3 failures
Without this commit: 100 times, 0 failures

Could you investigate?

Member

rafaelfranca replied Jun 1, 2012

@bogdan this commit make the Active Model suite to brake sometimes. Reverting this commit all the tests pass.

I used this script to run the suite many times.

With this commit: 50 times, 3 failures
Without this commit: 100 times, 0 failures

Could you investigate?

This comment has been minimized.

Show comment
Hide comment
@bogdan

bogdan Jun 1, 2012

Contributor

Sure,

Can you share some additional information with me:

  • a failure message
  • what are your thoughts on the reason
  • how did you detect this issue?

Thanks for detecting this. Can't even imagine how hard it could be to detect.

Contributor

bogdan replied Jun 1, 2012

Sure,

Can you share some additional information with me:

  • a failure message
  • what are your thoughts on the reason
  • how did you detect this issue?

Thanks for detecting this. Can't even imagine how hard it could be to detect.

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 1, 2012

Member

These are some examples of broken builds:

  1. http://travis-ci.org/#!/rails/rails/jobs/1498992
  2. http://travis-ci.org/#!/rails/rails/jobs/1496948
  3. http://travis-ci.org/#!/rails/rails/jobs/1489985

These errors started on the same day that we merged this commit, but we don't started to investigate, so I tried to revert this commit today because I thought it was the only reason to make the validations tests break, because we don't changed anything related with validations.

I think that we are getting callbacks name collision, and one of reason to this thought is this failure:

[/home/vagrant/builds/rails/rails/activemodel/lib/active_model/errors.rb:360]:
unexpected invocation: #<ActiveModel::Errors:0x9cbc5c4>.generate_message(:title, :accepted, {})
unsatisfied expectations:
- expected exactly once, not yet invoked: #<ActiveModel::Errors:0x9cbc5c4>.generate_message(:title, :less_than, {:value => 1, :count => 0})
Member

rafaelfranca replied Jun 1, 2012

These are some examples of broken builds:

  1. http://travis-ci.org/#!/rails/rails/jobs/1498992
  2. http://travis-ci.org/#!/rails/rails/jobs/1496948
  3. http://travis-ci.org/#!/rails/rails/jobs/1489985

These errors started on the same day that we merged this commit, but we don't started to investigate, so I tried to revert this commit today because I thought it was the only reason to make the validations tests break, because we don't changed anything related with validations.

I think that we are getting callbacks name collision, and one of reason to this thought is this failure:

[/home/vagrant/builds/rails/rails/activemodel/lib/active_model/errors.rb:360]:
unexpected invocation: #<ActiveModel::Errors:0x9cbc5c4>.generate_message(:title, :accepted, {})
unsatisfied expectations:
- expected exactly once, not yet invoked: #<ActiveModel::Errors:0x9cbc5c4>.generate_message(:title, :less_than, {:value => 1, :count => 0})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment