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

Improvements for legacy benchmark tools (closes #1249) #1254

Merged
merged 2 commits into from Jun 18, 2018
Merged

Improvements for legacy benchmark tools (closes #1249) #1254

merged 2 commits into from Jun 18, 2018

Conversation

dinesh-skyach
Copy link

Improvements for legacy benchmark tools #1249

@@ -0,0 +1,53 @@
require 'benchmark/amqp_mock'
require 'ruby-prof'
Copy link

Choose a reason for hiding this comment

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

@dinesh-skyach you must find a way to avoid code duplication
profiler should go in benchmark, improve benchmark usage eventually to configure smaller runs

Gemfile Outdated
@@ -72,6 +72,7 @@ end

group :development do
gem 'annotate', '~> 2.7'
gem 'ruby-prof'
Copy link

Choose a reason for hiding this comment

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

NEVER NEVER NEVER omit VERSION, AND DON'T require uneeded deps

Copy link

@mod mod left a comment

Choose a reason for hiding this comment

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

@dinesh-skyach you misunderstand me,

we need the existing benchmark to be able to support profiling

we can do

# will run regular benchmark
rake benchmark:matching

# Will run the same benchmark code with ruby-prof
# lower number of rounds and count
rake profiling:matching

@dinesh-skyach
Copy link
Author

@mod I didn't get what you mean by "Will run the same benchmark code with ruby-prof". The code is almost the same only the difference is that we need to run the benchmark under Benchmark.benchmark { } block and 'ruby-prof' under RubyProf.profile { } block.

Also for this "rake profiling:matching" currently to run this we use rake benchmark:profiling['matching']. Here you mean to change this to rake profiling:matching.

@yivo yivo changed the title [WIP] #1249 Improvements for legacy benchmark tools Improvements for legacy benchmark tools (closes #1249) Jun 13, 2018
@mod
Copy link

mod commented Jun 13, 2018

you are duplicating code can you make the feature without duplicating the code ?
@dinesh-skyach

@dinesh-skyach
Copy link
Author

@mod we need different files for profiling and benchmark, as they run under different blocks and also as we use 'ruby-prof' gem and this prints very large output because it tracks the recursively called methods. So we save the output in a file.

@mod
Copy link

mod commented Jun 18, 2018

@dinesh-skyach I still think you could have refactored the code in order to get less repeating code

@mod mod merged commit 12d955f into openware:1-8-stable Jun 18, 2018
@dinesh-skyach dinesh-skyach deleted the improvements_for_legacy_benchmark_tools branch December 27, 2018 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants