Skip to content
This repository has been archived by the owner on Apr 17, 2018. It is now read-only.

Commit

Permalink
Update Benchmarking method definitions to use class_eval
Browse files Browse the repository at this point in the history
* This works-around some problems with JRuby 1.4 not being able to
  handle a block with the arguments that were defined.
  • Loading branch information
dkubb committed Mar 11, 2010
1 parent 8d0d33a commit fe94d1a
Showing 1 changed file with 10 additions and 8 deletions.
18 changes: 10 additions & 8 deletions lib/dm-rails/adapters/benchmarking_adapter.rb
Expand Up @@ -13,14 +13,16 @@ def reset_runtime
rt
end

%w(create read update delete).each do |method|

define_method method do |*args, &block|
result = nil
@runtime += Benchmark.ms { result = adapter.send(method, *args, &block) }
result
end

%w[ create read update delete ].each do |method|
class_eval <<-RUBY, __FILE__, __LINE__
def #{method}(*args, &block) # def create(*args, &block)
result = nil # result = nil
@runtime += Benchmark.ms do # @runtime += Benchmark.ms do
result = adapter.#{method}(*args, &block) # result = adapter.create(*args, &block)
end # end
result # result
end # end
RUBY
end

private
Expand Down

12 comments on commit fe94d1a

@mkristian
Copy link
Member

Choose a reason for hiding this comment

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

thanx, that will run with jruby :-)

@sundbp
Copy link

@sundbp sundbp commented on fe94d1a Mar 11, 2010

Choose a reason for hiding this comment

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

btw, I have a patch in the active_support branch of dm-core (that goes with dm-rails) that removes use of objectspace to be jruby friendly: http://github.com/sundbp/dm-core/commit/9dbbd75b0d949341cd912cf106b6709753c95208
Seems like a good idea to incorporate?

@mkristian
Copy link
Member

Choose a reason for hiding this comment

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

@dkubb
ObjectSpace has a significant overhead in jruby: http://kenai.com/projects/jruby/pages/PerformanceTuning#Disabling_ObjectSpace

I do not understand the consequences of sundbp patch but I agree with him the remove the use of ObjectSpace

@sundbp
Copy link

@sundbp sundbp commented on fe94d1a Mar 11, 2010

Choose a reason for hiding this comment

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

I'll admit to being a bit ignorant of the exact impact of the change I made. I've put it in my fork and used it for my use cases of the last month and it seems to work well - can't notice any visible difference on MRI, but no real investigation done.

@dkubb
Copy link
Member Author

@dkubb dkubb commented on fe94d1a Mar 12, 2010

Choose a reason for hiding this comment

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

I agree that I would really like a way to remove the usage of ObjectSpace from that code, but I'm not entirely sure what the impact will be.

What that code does (or so I understand it), is that it allows you to match against the model object without holding a reference to it, preventing circular dependencies, which can cause problems with objects not being GC'd.

Although in this case I don't understand the point of testing self.class against the model. I mean, the string that this creates eventually gets passed down to an instance method, and inlined inside a single method. Since it's being executed within an instance method, by definition, doesn't that mean that self.class would always be equal to or a descendant of the class that defined the hook?

If someone else could validate what I say above to be true, I would gladly apply sundbp's patch.

@sundbp
Copy link

@sundbp sundbp commented on fe94d1a Mar 12, 2010

Choose a reason for hiding this comment

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

I'll try to spend some time over the weekend on understanding the code better and comment.
For the record I'm not sure if I came up with that patch or if I incorporated it from elsewhere, memory a bit fuzzy.

@mkristian
Copy link
Member

Choose a reason for hiding this comment

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

I really would appreciate this, since having ObjectSpace with jruby when it is disabled feels not right and enabling it feels even more wrong. thanx in advance.

@dkubb
Copy link
Member Author

@dkubb dkubb commented on fe94d1a Mar 12, 2010

Choose a reason for hiding this comment

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

I believe that the google app engine guys do something to replace that method in the rails_dm_datastore gem with something equivalent, which might be worth looking into.

@sundbp
Copy link

@sundbp sundbp commented on fe94d1a Mar 12, 2010

Choose a reason for hiding this comment

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

Ah, cool. Found extlib.rb and local_object_space.rb here quite interesting and possibly better solution:
http://github.com/joshsmoore/rails_dm_datastore/tree/master/lib/rails_dm_datastore/

@sundbp
Copy link

@sundbp sundbp commented on fe94d1a Mar 15, 2010

Choose a reason for hiding this comment

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

It seems to me we could adopt the solution the rails_dm_datastore people have put together in dm-core (since moved from extlib) without changing functionality or type of implementation and it'll work for both MRI and jruby (seems rubinius implemented some "missing" objectspace stuff last month but still good idea to avoid it if possible).

Want me to put together a patch vs dm-core (active_support branch) using this sort of implementation?

@mkristian
Copy link
Member

Choose a reason for hiding this comment

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

yes, please - you are the one who has the best background now and I would really appreciate dm-core to be one step more jruby friendly :-)

@sundbp
Copy link

@sundbp sundbp commented on fe94d1a Mar 20, 2010

Choose a reason for hiding this comment

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

I've put it together in: http://github.com/sundbp/dm-core/commits/next/
Sent a pull request.

Please sign in to comment.