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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce memory used by ActiveSupport::Callbacks #49728

Merged
merged 2 commits into from Oct 23, 2023

Conversation

jhawthorn
Copy link
Member

@jhawthorn jhawthorn commented Oct 21, 2023

Previously, callbacks which were shared with subclasses would not share between them the procs generated in Filters::Before and Filters::After. This was also lazily generated so would cause memory growth after an application is booted (and not sharable between workers via Cope on Write).

This was because we would rebuild the objects used to invoke the callbacks via CallbackChain#compile, so any difference in the callback chain would result in all of the callback procs being rebuilt.

This commit changes before and after callbacks (but not around!) to be shared between all subclasses of where it was defined. This is done by changing Filters::Before and Filters::After to plain classes which respond to call instead of generating procs (which wasn't strictly necessary but was easier to implement, and also results in simpler objects which use less memory). These objects avoid referencing and tied to a specific callback sequence and so can be memoized and reused.

This has the most impact on applications with many Controllers, and many callbacks in the ApplicationController (or similar).

I also took this opportunity to merge together all the different forms of procs generated (halting, halting_and_conditional, conditional, simple) into one form with if statements. There isn't any significant performance benefit from the specialization previously being done.

I didn't make this eagerly build the callable filter objects, but that's probably worth doing in a follow-up.

I think @byroot will appreciate this 馃槉

Reproduction

This is a self contained app which created 1000 controllers all inheriting from an ApplicationController with 50 before callbacks (numbers which are pretty comparable to GitHub's largest application), and makes one request to each controller going through the whole stack. Measuring live objects and RSS we see:

https://gist.github.com/jhawthorn/1540799e0d5c0a4be7f929fa6edd8d5b

main

$ be ruby benchmark_controller_callback.rb
live objects before: 252506
RSS before: 70044

live objects after: 594812
RSS after: 126052

object delta: 342306
RSS delta: 56008

this branch

$ be ruby benchmark_controller_callback.rb
live objects before: 252517
RSS before: 70036

live objects after: 339032
RSS after: 97756

object delta: 86515
RSS delta: 27720

This removes half of the memory growth from boot to post-request-serving (most of the remainder being I believe callcaches).

Previously, callbacks which were shared with subclasses would not share
between them the procs generated in Filters::Before and Filters::After.
This was also lazily generated so would cause memory growth after an
application is booted (and not sharable between workers via CoW).

This was because we would rebuild the objects used to invoke the
callbacks via CallbackChain#compile, so any difference in the callback
chain would result in all of the callback procs being rebuilt.

This commit changes before and after callbacks (but not around!) to be
shared between all subclasses of where it was defined. This is done by
changing Filters::Before and Filters::After to plain classes which
respond to call instead of generating procs (which wasn't strictly
necessary but was easier to implement, and also results in simpler
objects which use less memory). These objects avoid referencing and tied
to a specific callback sequence and so can be memoized and reused.

This has the most impact on applications with many Controllers, and many
callbacks in the ApplicationController (or similar).

I also took this opportunity to merge together all the different forms
of procs generated (halting, halting_and_conditional, conditional,
simple) into one form with if statements. There isn't any significant
performance benefit from the specialization previously being done.
ActiveSupport::Callbacks often ended up with empty Arrays: on callbacks
without a conditional, and on callback sequences which had no before
and/or after callbacks.
@jhawthorn jhawthorn merged commit 08473e3 into rails:main Oct 23, 2023
4 checks passed
@jhawthorn jhawthorn deleted the callbacks_sharing branch October 23, 2023 04: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.

None yet

1 participant