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

Remove broken conflict benchmarks #446

Merged
merged 2 commits into from
Mar 16, 2020
Merged

Conversation

dylanahsmith
Copy link
Contributor

Problem

I was trying to run our benchmarks, since I wanted to make sure recent refactors didn't cause a performance regression. However, it looks like they got broken because they were doing some monkey patching of the private resolve_cache_miss method that got removed in the refactor.

Running dev benchmark-cpu resulted in this error in the output

FetchEmbedConflictRunner:             /Users/dylansmith/src/identity_cache/performance/cache_runner.rb:114:in `method': undefined method `resolve_cache_miss' for class `#<Class:0x007fcf86b92460>' (NameError)
	from /Users/dylansmith/src/identity_cache/performance/cache_runner.rb:114:in `prepare'
	from ./performance/cpu.rb:14:in `run'
	from ./performance/cpu.rb:27:in `block in benchmark'
	from ./performance/cpu.rb:25:in `each'
	from ./performance/cpu.rb:25:in `benchmark'
	from ./performance/cpu.rb:36:in `bmbm'
	from ./performance/cpu.rb:44:in `<main>'

Solution

I just removed these conflict benchmarks, since the only difference they have is that they cause the cas or add memcached operations to fail, but identity cache doesn't have any code conditional on whether or not these operations succeed. As such, it was only really benchmarking dependencies of identity cache.

Comment on lines -39 to -42
matrix:
exclude:
- rvm: 2.2.6
gemfile: gemfiles/Gemfile.rails-edge
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This started overriding the above exclude under the jobs: key

@fbogsany
Copy link
Member

The benchmarks were originally added to compare perf with CAS against the corresponding Miss results. See #154 (comment) for discussion. They're not really useful as regression tests.

@dylanahsmith dylanahsmith merged commit ff8c02e into master Mar 16, 2020
@dylanahsmith dylanahsmith deleted the remove-conflict-benchmarks branch March 16, 2020 23:32
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems March 16, 2020 23:39 Inactive
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.

3 participants