-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Setup benchmark testing #1393
Setup benchmark testing #1393
Conversation
end | ||
end | ||
end | ||
class Comment < Model; end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we consistently preferred Comment = Class.new(Model)
for empty class definitions.
I'm generally 👍 on this. We definitely need a benchmark. |
43e4e3c
to
1a19b3f
Compare
get action | ||
i += 1 | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I were benchmarking on one branch, I'd put the benchmark code in here. But, because we're also comparing branches, I haven't settled on where I think the benchmarking should happen: in the test, or in the tool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean, but a possible solution would be to make a benchmark
directory in which you clone both branches.
require 'pathname' | ||
require 'shellwords' | ||
require 'English' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Document usage up here
This PR is clearly a must. Let me know if I can help. |
@beauby If you think the current form is good enough to work from, I'd say merge. Lemme add some docs on what |
c8c7212
to
5f78e5e
Compare
Yeah, I am having a hard time getting the interface right to do what i want... Eg find the commit where caching broke B mobile phone
|
# TODO: support /caching/{on,off} | ||
def get_caching | ||
get(url_base + "/caching") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically, the benchmark is a some combination or requests made against the server spawned at that ref. This way, everything can be controlled from the benchmarking file. e.g., if I wanted to run benchmark-ips, I would run that in here, not in the dummy controller. I think this makes it more flexible.
Also, cache toggling is now implemented in the server
+Rails.application.routes.draw do
+ get '/status(/:on)' => 'post#render_cache_status'
+ get '/caching(/:on)' => 'post#render_with_caching_serializer'
+ get '/non_caching(/:on)' => 'post#render_with_non_caching_serializer'
+end
|
||
output = { | ||
label: label, | ||
version: ::ActiveModel::Serializer::VERSION.to_s, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
rails_version: :Rails.version.to_s
9c6c9a6
to
c957411
Compare
@ianks Yeah, you'd stick it in |
@@ -10,7 +10,7 @@ rvm: | |||
- ruby-head | |||
- rbx-2 | |||
|
|||
install: bundle install --path=vendor/bundle --retry=3 --jobs=3 | |||
install: bundle install --path=vendor/bundle --retry=3 --jobs=3 --without bench |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like travis is not working because it uses an old version of bundler that does not handle the --without
parameter as expected. We can either follow travis-ci/travis-ci#3531 (comment) or maybe use --without development bench
?
I just squashed the work, updated the loggers, and removed all the revision running code and extra per tools. TODO:
|
@@ -1,6 +1,7 @@ | |||
inherit_from: .rubocop_todo.yml | |||
|
|||
AllCops: | |||
TargetRubyVersion: 2.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target ruby version!
Adding a benchmak test structure to help contributors to keep track of how their PR will impact overall performance. It enables developers to create test inside of tests/benchmark. This implementation adds a rake task: ```rake benchmark``` that checkout one commit before, run the test of tests/benchmark, then mover back to the last commit and run it again. By comparing the benchmark results between both commits the contributor will notice if and how much his contribution will impact overall performance.
- Setup dummy app files in `test/dummy` - Setup dummy test server `bin/serve_dummy - Note: Serializer caching can be completely disabled by passing in `CACHE_ON=off bin/serve_dummy start` since Serializer#_cache is only set at boot. - run with - ./bin/bench - `bin/bench` etc adapted from ruby-bench-suite - target files are `test/dummy/bm_*.rb`. Just add another to run it. - benchmark cache/no cache - remove rake dependency that loads unnecessary files - remove git gem dependency - Running over revisions to be added in subsequent PR
LGTM |
Followup issue in #1575 |
Based on #872
Update March 8, 2016
PR now contains:
test/dummy
CACHE_ON=off bin/serve_dummy start
since Serializer#_cache is onlyset at boot.
bin/bench
etc adapted from ruby-bench-suitetest/dummy/bm_*.rb
. Just add another to run it.Update Feb 12, 2016
bin/bench
runs files intest/dummy
that start withbm_
. typebin/bench --help
for more on the options and seebm_caching.rb
for an example implementation. Based on the ruby-bench-suiteThe caching slowdown here matches what was described in https://blog.codeship.com/building-a-json-api-with-rails-5/
Original PR description below for posterity
Usage:
bin/bench
with optional arguments<ref1> <ref2>
.<ref1>
defaults to the current branch and<ref2>
defaults to the master branch.bin/branch current
will run on the current branchUsing
bin/bench revisions 792fb8a9053f8db3c562dae4f40907a582dd1720 | tee data.txt
AMS Benchmark tests #832
Adding a benchmak test structure to help contributors to keep track of how
their PR will impact overall performance.
It enables developers to create test inside of tests/benchmark.
This implementation adds a rake task:
rake benchmark
that checkoutone commit before, run the test of tests/benchmark, then mover back to the
last commit and run it again. By comparing the benchmark results between
both commits the contributor will notice if and how much his contribution
will impact overall performance.