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

Refactor JsonApi adapter to avoid redundant computations. #1447

Merged
merged 1 commit into from
Feb 2, 2016

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Jan 19, 2016

This is a rewrite of #1300, stripped down to the bare minimum.

@anonova
Copy link

anonova commented Jan 21, 2016

rails-api/active_model_serializers:master: [active_model_serializers] Rendered ActiveModel::Serializer::CollectionSerializer with ActiveModel::Serializer::Adapter::JsonApi (2186.42ms)
rails-api/active_model_serializers:master: [active_model_serializers] Rendered ActiveModel::Serializer::CollectionSerializer with ActiveModel::Serializer::Adapter::Attributes (515.21ms)
beauby/active_model_serializers:jsonapi-refactor-2016: [active_model_serializers] Rendered ActiveModel::Serializer::CollectionSerializer with ActiveModel::Serializer::Adapter::JsonApi (1094.52ms)

Nice gains! Seeing about a 2x speedup from master with 1457 objects (250 root + 55 references + 1152 included resources). I may do some profiling at some point since this still seems quite slow, but it's definitely better than before.

It's also about 2x slower, rather than 4x, than the attributes adapter now.

@beauby
Copy link
Contributor Author

beauby commented Jan 21, 2016

@anonova Thanks for those numbers! We really need to get that benchmarking PR done soon. In general, I would expect JsonApi to be slower than Attributes, because in case of has_many or has_one associations, JsonApi will have to access the related models to fill the relationships part of the base object with the resource identifiers, even if the associated models are not included, whereas the Attributes adapter will simply skip them. So in the end, the JsonApi adapter is slightly richer, and it would seem more fair to compare JsonApi with nesting level n against JsonApi with nesting level n+1.
But in any case, we'll make sure the JsonApi adapter gets as fast as possible!

@beauby
Copy link
Contributor Author

beauby commented Jan 24, 2016

Any objection to this @rails-api/ams?

@bf4
Copy link
Member

bf4 commented Feb 2, 2016

Looks good. @anonova gist or paste bm code w results?

@kurko
Copy link
Member

kurko commented Feb 2, 2016

LGTM. +2

bf4 added a commit that referenced this pull request Feb 2, 2016
[PERF] Refactor JsonApi adapter to avoid redundant computations.
@bf4 bf4 merged commit 72c2c9f into rails-api:master Feb 2, 2016
@beauby beauby deleted the jsonapi-refactor-2016 branch April 22, 2016 09:07
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