Memoize resource relationships #1686
Conversation
bin/bench_regression "version": "0.10.0.rc5", "rails_version": "4.2.6", "benchmark_run[environment]": "2.2.2p95", perf/only_calc_associations_once "commit_hash": "1e7c428", caching on: caching serializers: gc off 741.7702402782281/ips; 1355 objects caching on: non-caching serializers: gc off 712.3752615532874/ips; 1257 objects caching off: caching serializers: gc off 706.0789199312495/ips; 1355 objects caching off: non-caching serializers: gc off 751.5310710635379/ips; 1257 objects master "commit_hash": "1033b711c7d7c231bb5b832e7dfe7f99389f22c4" caching on: caching serializers: gc off 567.7959835633892/ips; 1803 objects caching on: non-caching serializers: gc off 776.4929551133658/ips; 1257 objects caching off: caching serializers: gc off 538.046851190591/ips; 1803 objects caching off: non-caching serializers: gc off 738.5596630209004/ips; 1257 objects
bf4
force-pushed
the
perf/only_calc_associations_once
branch
from
bec951d to
22d585c
Compare
| @@ -34,7 +34,7 @@ def serializable_hash_for_single_resource(options) | |||
| def resource_relationships(options) | |||
| relationships = {} | |||
| serializer.associations(@include_tree).each do |association| | |||
| relationships[association.key] = relationship_value_for(association, options) | |||
| relationships[association.key] ||= relationship_value_for(association, options) | |||
There was a problem hiding this comment.
Source of this should be investigated, that serializer.associations(@include_tree).each seems to yield each association multiple times
There was a problem hiding this comment.
Holy crap! I'll try to investigate it
There was a problem hiding this comment.
yeah, discovered it in bf4/active_model_serializers@serializer_can_fully_serialize_itself...wip/serializer_can_fully_serialize_itself and was kind of floored by the performance impact
There was a problem hiding this comment.
That's weird AF. If serializer.associations(@include_tree).each yields an association multiple times, it means self.class._reflections.each (here) yields them several times.
There was a problem hiding this comment.
do we want to hold off on merging this PR until this issue is resolve, or should we go ahead and merge? personally, I think optimizations to associations should be a seperate PR :-)
There was a problem hiding this comment.
@beauby any objections to merging? there's no changelog, but I don't think it necessarily needs one
There was a problem hiding this comment.
No objection to merging per se, but I'd really like to understand what's going on here. Let's merge it and make sure we investigate this properly.
There was a problem hiding this comment.
I can make an issue and assign you as owner :)
|
rubocop error on travis ;-) |
bundle exec bin/bench_regression a5eaf6cd7a7fed42d9e64777753a1762e187eadc 1033b71 --pattern bm_caching ["perf/only_calc_associations_once", "a5eaf6cd7a7fed42d9e64777753a1762e187eadc", "1033b711c7d7c231bb5b832e7dfe7f99389f22c4", "a5eaf6c"] "version": "0.10.0.rc5", "rails_version": "4.2.6", "benchmark_run[environment]": "2.2.2p95", Note: checking out 'a5eaf6cd7a7fed42d9e64777753a1762e187eadc'. HEAD is now at a5eaf6c... Nicer debug; compare caching by serializer, grouped by caching on/off caching on: caching serializers: gc off 783.6956866669746/ips; 1355 objects caching on: non-caching serializers: gc off 798.8629770532652/ips; 1257 objects caching off: caching serializers: gc off 682.3661326140281/ips; 1355 objects caching off: non-caching serializers: gc off 721.2175067555897/ips; 1257 objects HEAD is now at 1033b71... Merge pull request rails-api#1638 from bf4/caching_redux caching on: caching serializers: gc off 570.6905948477781/ips; 1803 objects caching on: non-caching serializers: gc off 822.8418206976623/ips; 1257 objects caching off: caching serializers: gc off 523.4174806572001/ips; 1803 objects caching off: non-caching serializers: gc off 747.6026493097758/ips; 1257 objects
bf4
force-pushed
the
perf/only_calc_associations_once
branch
from
22d585c to
e554ba2
Compare
Purpose: improve performance
related: #1586
Changes: memoize serialized associations
Results: caching serializers are now at least as performant as non-caching
With caching on, serializer performance is now
Caveats: should porbably be fixed at the association-yielding level
Nicer debug; compare caching by serializer, grouped by caching on/off