redding / assert

Assertion style testing framework.
https://github.com/redding/assert
MIT License
10 stars 1 forks source link

prefer using `caller_locations` over `caller` #286

Closed kellyredding closed 6 years ago

kellyredding commented 6 years ago

So I've previously noticed that caller is a bit expensive in processing time and is adding overhead to our test suites. I set out to look into alternatives and squeeze some performance out of running tests in Assert.

In Ruby 2.0+ a new Kernel method, caller_locations, was introduced that is significantly more performant than caller. It also returns an array of location objects instead of an array of Strings.

See:

So, this sets out to switch to using caller_locations instead of caller if in Ruby 2.0+.

First off, I wanted to keep Assert compatible with pre Ruby 2.0 (selfishly b/c we still have some legacy apps I want to continue supporting). I chose to go the polyfill route and monkeypatch in a caller_locations method that behaves similarly enough to the Ruby 2.0+ method. I preferred this approach over some conditional logic as it is easier to remove once I no longer wnat to support pre Ruby 2.0. The polyfill just uses caller under the covers so it continue to return a list of Strings, not objects like the native implementation. I used this link for the method implementation ref: https://www.rubydoc.info/stdlib/core/2.0.0/Kernel%3Acaller_locations.

Next, I just had to go through the implementation and start replacing caller with caller_locations. This was fairly straightforward except for where the caller info was used as a backtrace to custom raised exceptions. Exceptions require backtraces that are arrays of strings so I had to map in those cases. All of this is backwards compatible with pre Ruby 2.0.

Performance-wise, on Assert's test suite on my dev machine:

On one of our medium size apps with 3541 tests (10488 results):

On pre Ruby 2.0, there is no significant performance changes as it is still using caller (under the covers).

So, this doesn't result in a massive performance improvement but it does squeeze out a bit.

@jcredding ready for review.

kellyredding commented 6 years ago

@jcredding so you are good with the polyfill approach - I didn't overlook any concerns there?

jcredding commented 6 years ago

@kellyredding - I think the polyfill is fine in this case. The only concern to me would be you are now defining a method on Kernel so its available in other libraries/apps. We could become dependent on it in another library/app and think everything is good because tests pass but then it errors outside of the test suite. I think this is very unlikely though because of the method; caller is not the most common method and I think we're unlikely to change anything currently using it.