-
-
Notifications
You must be signed in to change notification settings - Fork 7k
[Ruby] Use Ruby autoload to lower memory usage and load times #12680
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
Conversation
Fixes OpenAPITools#12648 Requiring all models up front can be very expensive in both time and memory if there are many models. In an example client with 6000 models, this would consume nearly 400MB of memory and take about 7 seconds to load. This is mostly unnecessary as most users of the client library will only actually use a small percentage of the library. The changes in this commit use Ruby's autoload capability to defer the loading until the constant is actually used. In that same example client with 6000 models, when initially requiring the library, the memory usage dropped to ~20MB and loaded in 0.3 seconds. As the constants are loaded on-demand, the memory would increase towards that 400MB ceiling, but if only a few constants are actually used, then memory will never actually hit that ceiling. An additional side effect of using Ruby's autoload is that the order of declaring the constants is not important, as Ruby will naturally load them in the correct order when they are needed. Thus, this commit obviates PR OpenAPITools#9103 and fixes OpenAPITools#4690.
{{#parent}} | ||
require '{{gemName}}/{{modelPackage}}/{{classFilename}}' | ||
{{/parent}} | ||
{{moduleName}}.autoload :{{classname}}, '{{gemName}}/{{modelPackage}}/{{classFilename}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a reason why we do it with the parent tags (to fix a bug reported previously about parent models not yet required) so please keep it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and likely we need an option for this (I can add it after merging this PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wing328 ... thanks for the review.
There's a reason why we do it with the parent tags (to fix a bug reported previously about parent models not yet required) so please keep it that way.
The parent tags were intentionally removed because they are no longer necessary.
The parent tags were introduced in #9103 in order to fix #4690, prior to which they didn't exist. When using require
, hierarchical ordering of the requires is necessary because the parent constant must exist, however when using autoload
, Ruby will naturally load things in the correct hierarchical order regardless of the order of the autoload statements. The fix in #9103 was only a partial fix, because it only handled one level of inheritance (see #9103 (comment)), however, with autoload, we can support any level of inheritance. Thus, this PR is a more robust alternative to #9103, with the side benefit of improved memory and load time.
Here's an example where autoload doesn't care about the order, and also shows more than one level of inheritance:
Given:
# foo.rb:
class Foo; end
# bar.rb:
class Bar < Foo; end
# baz.rb:
class Baz < Bar; end
Before:
# main.rb
require "bar"
require "baz"
require "foo"
puts Baz.new
# => uninitialized constant Foo (NameError)
After:
# main.rb
autoload :Bar, "bar"
autoload :Baz, "baz"
autoload :Foo, "foo"
puts Baz.new
# => #<Baz:0x00007fcebe0f1470>
and likely we need an option for this (I can add it after merging this PR)
I'm not sure I understand the need for an option. While my primary motivation was memory and load time, this can also be seen as a more robust version of #9103, which also didn't need an option as it was a bug fix.
Closed via #13153 by adding an option autoload was supposed to be removed from ruby 3.0 but later matz decided to keep it for the time being: https://bugs.ruby-lang.org/issues/5653 |
Thank you @wing328 !! |
Fixes #12648
Requiring all models up front can be very expensive in both time and
memory if there are many models. In an example client with 6000 models,
this would consume nearly 400MB of memory and take about 7 seconds to
load. This is mostly unnecessary as most users of the client library
will only actually use a small percentage of the library.
The changes in this commit use Ruby's autoload capability to defer the
loading until the constant is actually used. In that same example client
with 6000 models, when initially requiring the library, the memory
usage dropped to ~20MB and loaded in 0.3 seconds. As the constants are
loaded on-demand, the memory would increase towards that 400MB ceiling,
but if only a few constants are actually used, then memory will never
actually hit that ceiling.
An additional side effect of using Ruby's autoload is that the order of
declaring the constants is not important, as Ruby will naturally load
them in the correct order when they are needed. Thus, this commit obviates
PR #9103 and fixes #4690.
@cliffano @zlx @autopp Please review. I filed this against master, but if you think it should go elsewhere, please let me know.
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(6.0.1) (patch release),6.1.x
(breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)