Skip to content

Memoize resource relationships #1686

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

Merged
merged 2 commits into from
Apr 18, 2016
Merged

Memoize resource relationships #1686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7e2bc0
Memoize resource relationships
bf4 Apr 18, 2016
e554ba2
Nicer debug; compare caching by serializer, grouped by caching on/off
bf4 Apr 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/active_model_serializers/adapter/attributes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member Author

Choose a reason for hiding this comment

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

Source of this should be investigated, that serializer.associations(@include_tree).each seems to yield each association multiple times

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Apr 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

good find

Copy link
Member

Choose a reason for hiding this comment

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

Holy crap! I'll try to investigate it

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

@beauby any objections to merging? there's no changelog, but I don't think it necessarily needs one

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 can make an issue and assign you as owner :)

end

relationships
Expand Down
26 changes: 11 additions & 15 deletions test/benchmark/bm_caching.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,14 @@ def get_non_caching(on_off = 'on'.freeze)
get("/non_caching/#{on_off}")
end

def debug(msg = '')
if block_given? && ENV['DEBUG'] =~ /\Atrue|on|0\z/i
STDERR.puts yield
else
STDERR.puts msg
end
end

private

def assert_responses(caching, non_caching)
Expand Down Expand Up @@ -85,33 +93,21 @@ def assert_equal(expected, actual, message)
STDERR.puts message unless ENV['SUMMARIZE']
end
end

def debug(msg = '')
if block_given? && ENV['DEBUG'] =~ /\Atrue|on|0\z/i
STDERR.puts yield
else
STDERR.puts msg
end
end
end
assertion = ApiAssertion.new
assertion.valid?
# STDERR.puts assertion.get_status
assertion.debug { assertion.get_status }

time = 10
{
'caching on: caching serializers: gc off' => { disable_gc: true, send: [:get_caching, 'on'] },
# 'caching on: caching serializers: gc on' => { disable_gc: false, send: [:get_caching, 'on'] },
'caching off: caching serializers: gc off' => { disable_gc: true, send: [:get_caching, 'off'] },
# 'caching off: caching serializers: gc on' => { disable_gc: false, send: [:get_caching, 'off'] },
'caching on: non-caching serializers: gc off' => { disable_gc: true, send: [:get_non_caching, 'on'] },
# 'caching on: non-caching serializers: gc on' => { disable_gc: false, send: [:get_non_caching, 'on'] },
'caching off: caching serializers: gc off' => { disable_gc: true, send: [:get_caching, 'off'] },
'caching off: non-caching serializers: gc off' => { disable_gc: true, send: [:get_non_caching, 'off'] }
# 'caching off: non-caching serializers: gc on' => { disable_gc: false, send: [:get_non_caching, 'off'] }
}.each do |label, options|
assertion.clear
Benchmark.ams(label, time: time, disable_gc: options[:disable_gc]) do
assertion.send(*options[:send])
end
# STDERR.puts assertion.get_status(options[:send][-1])
assertion.debug { assertion.get_status(options[:send][-1]) }
end