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.

Sign up for GitHub

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

Jump to bottom

AMS Benchmark tests #832 #872

Closed
wants to merge 2 commits into from
Closed

AMS Benchmark tests #832 #872

wants to merge 2 commits into from

Conversation

joaomdmoura
Copy link
Member

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.

@kurko
Copy link
Member

kurko commented Apr 13, 2015

May I suggest something? Not everyone creates PR's with one commit. Should we perhaps run it against master instead? Or maybe something like rake benchmark master? What do you think about it?

@joaomdmoura
Copy link
Member Author

Totally agreed @kurko I thought about the issue but didn't came up with anything. Good idea ;)

@kurko kurko added this to the 0.10 milestone Apr 19, 2015
@joaomdmoura joaomdmoura force-pushed the benchmark-testing branch 6 times, most recently from c691224 to 0c0ec0e Compare April 21, 2015 17:45
@joaomdmoura
Copy link
Member Author

@kurko and it's done ;)
I think that I also fixed that travis ci thing I told you, the tests are supposed to work with the new dependency, but accordingly to their status page they are having some performance issues today.

@kurko
Copy link
Member

kurko commented Apr 21, 2015

Tests failing. Restarting.

@joaomdmoura joaomdmoura force-pushed the benchmark-testing branch 2 times, most recently from 3cde0a1 to b95063c Compare May 8, 2015 01:01
@joaomdmoura
Copy link
Member Author

Finally! @kurko can you review it for me? 😝

@joaomdmoura
AMS Benchmark tests rails-api#832
be29def 
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.
@joaomdmoura joaomdmoura force-pushed the benchmark-testing branch 3 times, most recently from b81653f to 08a068d Compare May 10, 2015 06:44
@joaomdmoura joaomdmoura mentioned this pull request Jun 22, 2015
Merged
bf4
bf4 reviewed Jun 22, 2015
View reviewed changes
.travis.yml
- "RAILS_VERSION=4.2"
- "RAILS_VERSION=master"
global:
- JRUBY_OPTS=-Xcext.enabled=true
Copy link
Member

Choose a reason for hiding this comment

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

housekeeping: I don't think is a good idea to set JRUBY_OPTS this way since travis uses JRUBY_OPTS="--server -Dcext.enabled=false -Xcompile.invokedynamic=false" now (cext.enabled=false is the future) and ref: rspec/rspec-dev#115

Copy link
Member

Choose a reason for hiding this comment

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

@joaomdmoura ^

Copy link
Member Author

Choose a reason for hiding this comment

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

😢
I'll find a workaround

@joaomdmoura joaomdmoura self-assigned this Jun 25, 2015
@beauby
Copy link
Contributor

beauby commented Sep 23, 2015

What's the status on this one?

bf4 added a commit to bf4/active_model_serializers that referenced this pull request Dec 16, 2015
@bf4
Benchmark serializer attributes caching
f3daad2 
ref:
- rails-api#827
- rails-api#832
- rails-api#872
- rails-api#1045
- rails-api#912
- rails-api#911
- rails-api#904
- rails-api#810
- rails-api#693
- rails-api#849
- rails-api#1029
- rails-api#1300
- rails-api#1372
@bf4
Copy link
Member

bf4 commented Dec 23, 2015

@joaomdmoura have you been able to demonstrate difference in caching vs. non caching in a branch and performance of each across two branches? I've been futzing with this a bit and haven't been able to show a difference.

@bf4 bf4 mentioned this pull request Dec 23, 2015
Merged
@bf4
Copy link
Member

bf4 commented Dec 23, 2015

#1393 is based on this, and is on master, should run across commit history without conflicts

@kurko
Copy link
Member

kurko commented Jan 25, 2016

This is stale. @joaomdmoura any thoughts on this? Any plans to continue?

@bf4 bf4 mentioned this pull request Feb 11, 2016
Closed
@bf4 bf4 closed this Feb 15, 2016
@bf4
Copy link
Member

bf4 commented Feb 15, 2016

I now have a working implementation at #1393 that I'm pretty happy with, for the benchmarking part, and some maturing revision-running code I'll be pushing up soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@joaomdmoura joaomdmoura

Labels
C: Performance S: Needs Submitter Response V: 0.10.x
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.
4 participants
@joaomdmoura @kurko @beauby @bf4