Use autoLoad to speed up loading code #454
Comments
|
@bdewater thank you for the suggestion! It looks like this functionality was added in OpenAPI generator 6.1 and we are currently on 5.1.1. I know the team has been looking into migrating our libraries to use version 6, but it's turned out to be a non-trivial project and not on the roadmap for H1. I've logged a task in our internal tracker so we can track this as both yet another reason to upgrade and to have it on the task list for when we do upgrade. |
|
To +1 this; the |
|
Just to follow up on this, we've upgraded to auto-generator 6.1, for this release, which is a pre-requisite for enabling autoload. I'm going to try enabling it and run this by the team and if all goes well, autoload can be enabled in our next ruby client library release |
|
Following up again, I'm currently at the stage of vetting this proposal with our internal ruby expert, and he asked if you could let us know what kind of load time you're seeing on the Plaid gem? Any further memory profiling data that would have would also be really useful. Getting this info will help us understand the impact of not using autoload and the risk/reward payoff of enabling it. |
|
@phoenixy1 thank you for following up. That's what I have right now given my time constraints. What I can say is that my app uses over 170 gems, (including Rails) and Plaid represents 30% of memory of my app on boot, so it feels like maybe the gem is not doing some sort of best practice, as I also use a fair amount of involved clients of 3rd party services. Given I only use the Gem in a few places, I guess my alternative is to just NOT use it and roll my own, but it feels like maybe a smell that autoloading would resolve. I'm not an expert in Gem packaging, but I do know many gems follow a pattern where including the gem does not immediately pull in EVERY file into memory, and follow more of the lazy autoload pattern. |
|
@phoenixy1 I'm currently on leave, but IIRC after adding the Plaid gem I saw ~1s increase in app cold boot in our Rails app in development environment. |
|
BTW, this didn't make today's release, but I did get approval and plan to get it out for next month. |
|
Hey @phoenixy1 , did this make it into a release? |
|
@seanbjornsson sorry for the delay and thank you for the reminder! I got super busy right around then with some other projects, but I'm making another stab at adding this and currently have a PR awaiting review that I think should fix it. |
|
I forgot to circle back. Quick benchmark script, store as require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
gem "memory_profiler"
gem "rails", "7.1.0", require: false
gem "plaid", "23.0.0", require: false
end
report = MemoryProfiler.report do
require "rails/all"
require "plaid"
end
report.pretty_printRun with Memory and objects allocated by Plaid are an order of magnitude more than any other gem, and ~50% of total: Similar picture for retained: Removing the memory_profiler reporting and calling
Note that Rails 7.1 and it's dependencies comprise 64 gems, so half a second for Plaid alone is a lot. |
|
Just following up, the PR that builds the library with useAutoload=true has just been merged, so this should be incorporated into the next client library release |
|
@bdewater thanks for sharing that script. I ran that first with Plaid 22.0.0, then with Plaid 24.2.0 and noticed a significant decrease in allocated and retained memory for the gem. |
|
This has now been shipped in 24.0.0, so I'm going to close this out. Thanks all who helped report and profile this issue! |
The Plaid gem is quite heavy with over 1000 model files in
lib/plaid/models. These are all forcibly loaded here:plaid-ruby/lib/plaid.rb
Lines 20 to 1091 in 9143e24
If this were to use Ruby's autoload feature it would only load files on demand, speeding up the time it takes to start a Rake task or Rails app (and lowering memory usage). Fortunately the OpenAPI generator supports this, see:
useAutoloadoption to use autoload instead of require OpenAPITools/openapi-generator#13153Could this be implemented?
The text was updated successfully, but these errors were encountered: