-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Introduces autoloading strategy for service gems #3098
Conversation
dbcd6e0
to
3585360
Compare
3585360
to
2b8a515
Compare
Thanks for opening a pull request on this! We were looking to do this to sso, ssoidc, and sts gems that are stuck inside of core, but we ran into some compatibility problems across versions. For example, we need to consider cases where the service gem is older but the core gem is newer. Could you please revert the code generated changes so the PR is more manageable? |
This reverts commit 2b8a515.
My pleasure! I have very much enjoyed working on this.
Sure, that makes sense.
Sure! I did this as a vanilla revert commit for now so you can inspect the codegen changes if needed. If you prefer I pop both commits off altogether I can do that. [This commit}(https://github.com//pull/3098/commits/44fd76db259a1fcf0360cfbf1380c29f96ec794b) still has a fair few files that had to be done by hand to support production eager loading. I did not make that new codegeneration because it shouldn't change. |
Great. This will take some time to review and test and I can scope some time out early next week. I saw some Rails specific stuff, please ensure that's removed too. |
Thanks
This code only runs if the gem is used within a Rails application. ( @gmcgibbon do you know if there's a novel way to achieve eager-loading on the application-end without having the Railtie logic upstream? Or could we roll our own? |
The rails specific logic could potentially be moved to https://github.com/aws/aws-sdk-rails. In general I like the idea, though we had to revert our attempts at making bundled services in core autoload (see #3088) - complex load_paths in some customer cases ultimately made it impossible to support safely for all existing users. My overall concern with this method is that it moves the cost of loading the sdk from require time to first use (that is, when creating a service client for the first time). Considering a typical service case, this means that time could end up impacting a user request rather than just service startup. Do you have any benchmarking on client creation? |
Thanks for the reply @alextwoods, much appreciated. 🙇
Okay, I have no problem with doing that.
Yes. Right now it's fairly expensive to boot time to require script# typed: true
# frozen_string_literal: true
require "benchmark"
original_require = Kernel.method(:require)
$require_times = {}
Kernel.define_method(:require) do |name|
result = nil
time = Benchmark.measure do
result = original_require.call(name)
end
$require_times[name] = time.real * 1000 if result
result
end
require_relative "../config/environment"
puts "slowest requires:"
$require_times.sort_by { |_, time| -time }.first(20).each do |name, time|
puts format("%-60s %10.4f", name, time)
end You are right that autoloading by default defers parts of this load time to the first time the user would hit, say,
For users of these gems who don't have eager load capability, it is probably good to have this benchmark. I don't have this code in front of me right now, but let me circle back to you with that. 👍 |
So far this change also seems to be passing in our internal build system (with a few of the fixes in the branch I linked), so thats looking good. I believe it may avoid some of the complex load path issues that I ran into with #3083 (but haven't completed a full 100% test of it). I've been doing some initial benchmarking on client initialization for S3(without the eager load). It does definitely increase the first client initialize time (since it has deferred all of the require until then), but there is still a net win when adding the require + initialize time (since we are avoiding requiring other un-used parts of the SDK). While I am still a bit concerned with this, I think its a reasonable tradeoff. With Autoload: Require Allocated: 11450 kb Current State (no autoload): Require Allocated: 30835 kb |
@alextwoods thanks for this, you beat me to those benchmarks! It does seem that time-to-first-client might still be an improvement in production even with an autoload-only setup.
I'm working on doing this right now, I'll circle back when access is working. Thank you! |
@alextwoods adding you as a collaborator on a fork on our org proved surprisingly trickier than it should be. 😅 I got decent advice it would be much simpler to use my own fork where I have all admin privileges and add you as a collaborator. I've pushed this to my personal fork and invited you, you should be able to do whatever you want with the branch now I believe. Any issues let me know. New PR/branch: #3105 |
Thanks! Accepted and I'll see if I can get my changes merged into that PR and then clean up these others. |
Closing in favor of #3105 |
What are we trying to accomplish?
This PR introduces an autoloading strategy for the top-level service modules in the AWS SDK for Ruby. Specifically, we're updating the
service_class
template, which generates the main entry point for each AWS service module. Our goal is to significantly improve performance by reducing memory footprint and speeding up gem require times.We also ensure eager loading is still possible for production environments.
Key Changes:
service_class.rb
andservice_class.mustache
are updated to useautoload
instead ofrequire_relative
. Other templates that are explicitly loaded inservice_class.mustache
also are updated.customizations
directly into resources if the file exists.The PR is structured in six commits to facilitate review. The resulting code generation is the final commit.
Benchmarks
aws-sdk-core
is used in these examples.Require Time
require 'aws-sdk-core'
Benchmark Script
Memory Allocation
Memory Profiler Script
Real Life Boot Time
This is what we're seeing with our application at boot time, pointed to this branch.
~250ms average improvement on boot time. Could be much more for applications that rely on a lot of gems.
Before:
~3.69s, GC ~465ms
After:
~3.42s, GC ~435ms
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.