Skip to content

Bundler::DepProxy#name performance improvement.#5536

Closed
technicalpickles wants to merge 1 commit intoruby:masterfrom
technicalpickles:memoize-dep-proxy-name
Closed

Bundler::DepProxy#name performance improvement.#5536
technicalpickles wants to merge 1 commit intoruby:masterfrom
technicalpickles:memoize-dep-proxy-name

Conversation

@technicalpickles
Copy link
Copy Markdown
Contributor

What was the end-user or developer problem that led to this PR?

Looking for ways to speed up require 'bundler/setup'

What is your fix for the problem, implemented in this PR?

Looking over flamegraphs for require 'bundler/setup', I'm seeing
Bundler::DepProxy#name show up quite often. The method itself is really
simple, delegating to the dependency's proxy. It's taking up to 1ms sometimes.

Screen Shot 2022-05-13 at 4 57 23 PM

I suspect it is getting called enough that memoizing the value improves
will improve performance by saving a method call, in exchange for saving
the value in memory.

When testing with this patch plus #5533
I saw time go from 0.92s to 0.75s.

Make sure the following tasks are checked

Looking over flamegraphs for `require 'bundler/setup'`, I'm seeing
`Bundler::DepProxy#name` show up quite often. The method itself is really
simple, delegating to the dependency's proxy.

I suspect it is getting called enough that memoizing the value improves
will improve performance by saving a method call, in exchange for saving
the value in memory.

When testing with this patch plus ruby#5533
I saw time go from 0.92s to 0.75s.
@indirect
Copy link
Copy Markdown

Can you provide a repro script that others can use to verify/see the performance improvements? Ideally using a benchmarking tool like hyperfine to reduce outliers. 🙏🏻

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

This is really awesome! Funny enough, yesterday I was working on making Bundler::SpecSet#for not use any DepProxy's at all, so that would also fix this issue I believe. I'm really glad we're making bundler/setup this much faster!

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

I kept exploring the idea of not using DepProxy's at all in SpecSet#for, but instead use an array of [name, platform] tuples. The idea works and simplifies the code, and removes the need to call expand_dependencies in Definition at all every time before using SpecSet#for. But does not seem to improve performance unless I use a hash instead of an array to keep track of handled elements like #5537 does.

So my understanding is that #5537 makes this PR unnecessary but I'd like @technicalpickles to confirm!

@technicalpickles
Copy link
Copy Markdown
Contributor Author

Can you provide a repro script that others can use to verify/see the performance improvements? Ideally using a benchmarking tool like hyperfine to reduce outliers. 🙏🏻

I will work on it, yes! I think the main thing is getting a sufficiently large Gemfile (SpecSet#for was showing it resolving 1290 gems at one point for ours), and then running ruby -e 'require "bundler/setup"' through hyperfine

But does not seem to improve performance unless I use a hash instead of an array to keep track of handled elements like #5537 does.
So my understanding is that #5537 makes this PR unnecessary but I'd like @technicalpickles to confirm!

I'll test to confirm, the flamegraph should tell us if it's still frequently in the stack.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

After all the other improvements made, this PR seems to no longer speed up things, so I guess we can close it! Thanks @technicalpickles!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants