Skip to content
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

Memoize exception_lines in ExceptionPresenter #2743

Merged
merged 1 commit into from Jun 29, 2020
Merged

Conversation

MaxLap
Copy link
Contributor

@MaxLap MaxLap commented Jun 27, 2020

When an exception occurs and is displayed, ExceptionPresenter is used. However, there is an obvious slow amplification doing in it. This PR fixes it.

The method failure_lines uses exception_lines 3 times, which uses exception.message. In some situation, exception.message can be slow (when inspect is called on a big object for example, such as during NoMethodError).
Without caching, this delay, which can become considerable, will be amplified by 3x.

Here is a simple example of an rspec that would be amplified

class A
  def inspect
    sleep(1)
  end
end

describe 'A' do
  it 'works' do
    A.new.no_such_method
  end
end

Instead of taking 1 second, the print of the failure message takes 3 seconds.

I did memoization (@exception_lines ||= ...), but the call result could have been saved and reused in the failure_lines method. Let me know if you prefer that i change it to just start failure_lines with exception_lines = self.exception_lines.

As a side note, I think it would be helpful if the --profile or a new flag would help debug slow failures messages (top 10 of slowest failures to display).

@pirj pirj marked this pull request as draft June 29, 2020 12:47
@pirj
Copy link
Member

pirj commented Jun 29, 2020

Do you mind checking the build failure?

@MaxLap
Copy link
Contributor Author

MaxLap commented Jun 29, 2020

Looking here https://travis-ci.org/github/rspec/rspec-core/branches, i believe the problem is on master, and not a result of my changes.

@JonRowe
Copy link
Member

JonRowe commented Jun 29, 2020

@MaxLap It's not, this branch introduces failures in aggregate failures scenarios but master is failing due to our cross repo checks which in turn are failing due to diff-lcs changes elsewhere.

@JonRowe
Copy link
Member

JonRowe commented Jun 29, 2020

Apologies, I checked again it is failing with the same error. However this cannot be merged until it is green.

@pirj pirj marked this pull request as ready for review June 29, 2020 13:36
@JonRowe
Copy link
Member

JonRowe commented Jun 29, 2020

Master is green if you want to rebase this

The method failure_lines uses exception_lines 3 times, which uses exception.message.
In some situation, exception.message can be slow (when inspect is called on a big object for example).
Without caching, this delay which can become considerable will be amplified by 3x.
@MaxLap
Copy link
Contributor Author

MaxLap commented Jun 29, 2020

All green :)

@JonRowe JonRowe merged commit 8a4fadf into rspec:master Jun 29, 2020
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thanks!
Don't forget to add a changelog entry. No change is too small.

JonRowe added a commit that referenced this pull request Jun 29, 2020
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020
Memoize exception_lines in ExceptionPresenter
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants