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

[ci skip] Add benchmark your code section to contributing to ruby on rails guide. #15347

Merged
merged 1 commit into from May 26, 2014

Conversation

@JuanitoFatas
Copy link
Contributor

JuanitoFatas commented May 26, 2014

I have seen many times that maintainers want people to use this gem to give
a benchmark report. It would be nice to add it and refer to it later.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented May 26, 2014

Seems good to me. @zzak WDYT about the gramar?

@rafaelfranca rafaelfranca added the docs label May 26, 2014
@JuanitoFatas

This comment has been minimized.

Copy link
Contributor Author

JuanitoFatas commented May 26, 2014

screenshot 2014-05-27 03 55 48

@JuanitoFatas

This comment has been minimized.

Copy link
Contributor Author

JuanitoFatas commented May 26, 2014

I use some words from Mr @tenderlove's comments on #15272 (comment).

@chancancode

This comment has been minimized.

Copy link
Member

chancancode commented May 26, 2014

👍 on the idea. Would need to expand more on the introduction paragraph to explain "why". Things like you should only do this if it's in fact a hot path and a bottleneck, readability and understandability are also important, etc etc.

But this is a good start. We can even do those after we merge his.

@zzak
zzak reviewed May 26, 2014
View changes
guides/source/contributing_to_ruby_on_rails.md Outdated
@@ -215,6 +215,40 @@ Rails follows a simple set of coding style conventions:

The above are guidelines - please use your best judgment in using them.

### Benchmark Your Code

Please use [benchmark-ips](https://github.com/evanphx/benchmark-ips) Gem to

This comment has been minimized.

Copy link
@zzak

zzak May 26, 2014

Member

Please use the [benchmark-ips] gem to

This comment has been minimized.

Copy link
@JuanitoFatas

JuanitoFatas May 26, 2014

Author Contributor

Thanks! Updated.

@zzak
zzak reviewed May 26, 2014
View changes
guides/source/contributing_to_ruby_on_rails.md Outdated

Please use [benchmark-ips](https://github.com/evanphx/benchmark-ips) Gem to
create a performance report. If the change was from a bottleneck you ran into,
please share the benchmarks if you could.

This comment has been minimized.

Copy link
@zzak

zzak May 26, 2014

Member

I would change this whole paragraph to say:

If your change has an impact on the performance of Rails, please use the [benchmark-ips] gem to provide benchmark results for comparison.

This comment has been minimized.

Copy link
@JuanitoFatas

JuanitoFatas May 26, 2014

Author Contributor

Updated! Thank you.

@zzak
zzak reviewed May 26, 2014
View changes
guides/source/contributing_to_ruby_on_rails.md Outdated
create a performance report. If the change was from a bottleneck you ran into,
please share the benchmarks if you could.

Example of benchmark-ips:

This comment has been minimized.

Copy link
@zzak

zzak May 26, 2014

Member

Here's an example of using benchmark-ips:

This comment has been minimized.

Copy link
@JuanitoFatas

JuanitoFatas May 26, 2014

Author Contributor

Updated, thanks.

@zzak
zzak reviewed May 26, 2014
View changes
guides/source/contributing_to_ruby_on_rails.md Outdated
end
```

will create a report like:

This comment has been minimized.

Copy link
@zzak

zzak May 26, 2014

Member

This will generate a report with the following information:

This comment has been minimized.

Copy link
@JuanitoFatas

JuanitoFatas May 26, 2014

Author Contributor

Updated! Thanks a lot.

@zzak
zzak reviewed May 26, 2014
View changes
guides/source/contributing_to_ruby_on_rails.md Outdated
per second, the faster something is. A fun thing about benchmark/ips is that you
don't need to guess about how many times you need to run your code, benchmark/ips
automatically figures out how many times to run the code to get interesting data.
No more guessing at random iteration counts!

This comment has been minimized.

Copy link
@zzak

zzak May 26, 2014

Member

I would rewrite this section as such:

Benchmark/ips will report the number of iterations per second for a given block of code. When analyzing the results, notice the percent of standard deviation which tells us how spread out our measurements are from the average. A high standard deviation could indicate the results having too much variability.
One benefit to using this method is benchmark-ips automatically determines the data points for testing our code, so we can focus on the results instead of guessing iteration counts as we do with the traditional Benchmark library.

This comment has been minimized.

Copy link
@fxn

fxn May 26, 2014

Member

This is Rails documentation, the last paragraph would maybe work in an enthusiastic blog post about benchmark/ips, but wording and tone do not seem appropriate here.

We do not need to repeat the documentation of the library in our own documentation, and we do not need to sell it. Just say something really short and to the point.

This comment has been minimized.

Copy link
@zzak

zzak May 26, 2014

Member

@fxn good point, I think this paragraph would be better suited in the README of benchmark/ips, we should just say: Please see the benchmark/ips [README] for more information.

This comment has been minimized.

Copy link
@chancancode

chancancode May 26, 2014

Member

The practical aspect here is that we've had people submitting benchmark reports that are clearly a result of misunderstanding/misuse of the benchmark/ips library. If we think that's a common enough problem we can perhaps have a template for that in here?

This comment has been minimized.

Copy link
@zzak

zzak May 26, 2014

Member

@JuanitoFatas Could you change this to my suggestion above (just link to the readme)?

@chancancode I'm +1 on a benchmark report template

This comment has been minimized.

Copy link
@JuanitoFatas

JuanitoFatas May 26, 2014

Author Contributor

@zzak link added. Please see below.

This comment has been minimized.

Copy link
@zzak

zzak May 26, 2014

Member

@JuanitoFatas please rm the above paragraph

This comment has been minimized.

Copy link
@JuanitoFatas

JuanitoFatas May 26, 2014

Author Contributor

rmed. 😢

@zzak

This comment has been minimized.

Copy link
Member

zzak commented May 26, 2014

@JuanitoFatas Please apply my suggestions, squash, and I will commit.

zzak added a commit to zzak/benchmark-ips that referenced this pull request May 26, 2014
@JuanitoFatas

This comment has been minimized.

Copy link
Contributor Author

JuanitoFatas commented May 26, 2014

@zzak I have applied your suggestions. Thanks! 👍
@rafaelfranca @fxn @chancancode Thanks for the feedback. Now we have a starting point if someone wants to expand this further.

@zzak

This comment has been minimized.

Copy link
Member

zzak commented May 26, 2014

IMO we could expand on this further in the performance testing guide ;)

…rails guide.

I have seen many times that maintainers want people to use this gem to give
a benchmark report. It would be nice to add it and refer to it later.
zzak added a commit that referenced this pull request May 26, 2014
…-to-ror

[ci skip] Add benchmark your code section to contributing to ruby on rails guide.
@zzak zzak merged commit 5af6120 into rails:master May 26, 2014
@JuanitoFatas JuanitoFatas deleted the JuanitoFatas:benchmark-in-contributing-to-ror branch May 26, 2014
@zzak

This comment has been minimized.

Copy link
Member

zzak commented May 26, 2014

@JuanitoFatas Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.