Skip to content

Build: avoid leaking memory from generate_plugins_version #12763

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
kares merged 2 commits into from
Mar 24, 2021
Merged

Build: avoid leaking memory from generate_plugins_version #12763

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
addd498
Build: reduce plugin version script memory usage
kares Mar 23, 2021
e49b589
Build: further reduce pressure from RGs index building
kares Mar 23, 2021
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
24 changes: 24 additions & 0 deletions rakelib/plugins_docs_dependencies.rake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ class PluginVersionWorking
end

def try_plugin(plugin, successful_dependencies)
Bundler::DepProxy.__clear!
builder = Bundler::Dsl.new
gemfile = LogStash::Gemfile.new(File.new(LogStash::Environment::GEMFILE_PATH, "r+")).load
gemfile.update(plugin)
Expand Down Expand Up @@ -203,6 +204,29 @@ task :generate_plugins_version do
end
end
end
DepProxy.class_eval do
# Bundler caches it's dep-proxy objects (which contain Gem::Dependency objects) from all resolutions.
# The Hash itself continues to grow between dependency resolutions and hold up a lot of memory, to avoid
# the issue we expose a way of clear-ing the cached objects before each plugin resolution.
def self.__clear!
@proxies.clear
end
end

Fetcher::CompactIndex.class_eval do
alias_method :__bundle_worker, :bundle_worker
# The compact index is built using multiple threads and this is hard-coded atm to 25 threads:
# `Bundler::Worker.new(Bundler.current_ruby.rbx? ? 1 : 25, worker_name, func)`
# each thread might built up a Bundler::Source::Rubygems object which retains more than 100MB.
# By limiting the worker thread count we make sure not to produce too many of these objects.
Copy link
Member

@yaauie yaauie Mar 23, 2021

Choose a reason for hiding this comment

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

This is a really good find.

I'm a bit wary of the Bundler::Fetcher::CompactIndex#bundle_worker implementation changing out of under us -- if it were to perform an operation that had a side-effect of starting the worker threads, our interception here would be too late to make a difference.

An alternate approach would be to patch Bundler::Worker to cap the size of the pool, something like:

Worker.class_eval do
  alias_method :__initialize, :initialize
  def initialize(size, name, func)
    if size > 2
      $stderr.puts("Bundler::Worker: initializing limited worker pool (size: 2; requested: #{size})"
      size = 2
    end
    __initialize(size, name, func)
  end
end

Copy link
Contributor Author

@kares kares Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I understand the concern and have thought about patching the Worker as well.
The worker was introduced 4.5+ years ago with Bundler 2.2.
(has bee used previously but existed inline - there wasn't a bundle_worker method here).

Would say the main use-case for Bundler::Worker is the parallel installation (from the installer), this can be disabled using the API if we choose to (command line --jobs option).

That being said I would rather not cause future surprises by affecting bundle install, unless we know it to be problematic. My general approach to patches tends to be to be contained and have minimal effect on other places.

Instantiating Bundle::Worker does not spawn threads, there needs to be an enq operation to do so (original code changing to do so would show up as changing arguments for the method).

Interestingly the 25 constant seems to have a life of its own, will try to raise this upstream with a reproducer.

def bundle_worker(func = nil)
__bundle_worker(func).tap do |worker|
size = worker.instance_variable_get(:@size)
fail("@size = #{size.inspect} is no longer an integer") unless size.is_a?(Integer)
worker.instance_variable_set(:@size, 2)
end
end
end
end

PluginVersionWorking.new.generate
Expand Down