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

Add benchmarks for adapters #1511

Closed
wants to merge 1 commit into from
Closed

Add benchmarks for adapters #1511

wants to merge 1 commit into from

Conversation

ianks
Copy link

@ianks ianks commented Feb 11, 2016

I was curious how the adapters stacked up against each other performance wise. While doing this, I realized it would be useful to maintain some simple benchmark tests that can be used during development. This one is a simple benchmark which tests the performance of each adapter. If you are interested in the results from my machine: here they are.

@bf4
Copy link
Member

bf4 commented Feb 11, 2016

@ianks Thanks for this! Very awesome. 👍 Do you have a particular philosophy about writing benchmark code for an app, on where to put it, add to it, and when and how to run it? I have a PR to add a benchmark for performance regressions that I've kind of stalled on... If that interests you, I'd love your input, especially since I'd want it to co-exist with this.

@bf4
Copy link
Member

bf4 commented Feb 11, 2016

ref: #872 and my mess of #1393 and #1412

serializer = PostWithCustomKeysSerializer.new(resource)

Benchmark.ips do |x|
ActiveModel::Serializer::Adapter.adapter_map.each do |adapter_name, adapter_class|
Copy link
Member

Choose a reason for hiding this comment

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

oh fun

@ianks
Copy link
Author

ianks commented Feb 12, 2016

Hey @bf4 I checked out your PR. It is really good.

I don't really have a 'philosophy' on benchmarking per se; other than it is something one should strive to do when coding. I think the approach you take in your PR is very important, as it documents perf regressions from changes in the code automatically. It is almost like an integration test suite for perf.

The goal of this PR, is to simply have a place where people can write benchmarks for small sub-sets of the code they are working on.

For example, I wanted to see if I could increase the perf of the JSONAPI adapter, and benchmarking only the adapters makes sense in this case. Almost like a unit-test of perf.

TL;DR -- Your approach works better for documenting perf and catching regressions. The approach of this PR is more geared towards creating a place for contributors to write benchmarks for specific pieces of code. I hope that makes sense? Ha 😄

@bf4
Copy link
Member

bf4 commented Feb 12, 2016

@ianks you've inspired me.. it's better now.. I focused on just the benchmark before worrying about regression testing across revisions.. and now there's a structure for this PR.. test/dummy/bm_adapter.rb (or should it be test/benchmark/... not sure 'cause I want the dummy app easily accessible). Other thought is to update the https://github.com/rails-api/active-model-serializers-example and use that...

@groyoh
Copy link
Member

groyoh commented Feb 15, 2016

Just a quick thought on this: shouldn't there be a benchmark using the include option to test performance when including associations?

@bf4
Copy link
Member

bf4 commented Feb 15, 2016

@groyoh so many things should be... :)

@ianks ianks mentioned this pull request Feb 18, 2016
@bf4 bf4 mentioned this pull request Mar 10, 2016
25 tasks
@bf4
Copy link
Member

bf4 commented Mar 13, 2016

@ianks So, #1393 has been merged.. wanna use that structure? you'd be the first, so a good time to help see how well it works, easy to use, and write up usage docs :)

@remear
Copy link
Member

remear commented Mar 17, 2016

Benchmarking should use and iterate on the structure introduced by #1393. Closing this, but I'd really like to see the work done in this PR done in a new PR with the new benchmarking structure.

@remear remear closed this Mar 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants