Uh. Old rubies doesn't have RubyVM.stat. I think I'll just use fallback same as jruby for them.

@jazzonmymind according to you comment #1548 (comment), later all_available_methods_cached can be just moved to instance methods without mutex.

https://github.com/pry/pry/blob/master/lib/pry/pry_instance.rb#L136 has to be updated to hold onto the completer instance. something like this could work:

@completer ||= config.completer.new(config.input, self)
@completer.call(...)

… probably take a while to review and test this.

Should I add it to this PR?

@jazzonmymind does pry creates new instance on every binding.pry?

yup.

yup to both questions ;-) locally you could create a new branch from the work you've already done, and confirmed it has no bugs or unintended consequences, then merge into this branch and push but it's your call.

it won't be cleared all the time. it will be cleared between pry sessions, and i think that's fine.

Here is it, tests are green.

thanks, but why are all_available_methods_cached and all_available_methods still class methods? i think we can make them private instance methods, like all_available_methods is.

.methods_cache_version and .all_available_methods doesn't depend on instance's state, so it's better to have them on class level.

okay, but what utility do they provide as class methods? they seem to be better suited as private instance methods to me, since they serve no purpose as a public API.

Ready

thanks. i will test this soon.

@R-obert any updates on it?

sorry, i've been busy with work and this weekend i'm travelling. it looks good to me, i just want to clone it locally and test how it works.

@printercu Could you see whether #1588 improves your use case as well, and if so, by how much?

Copied #1583 (comment)

Here are my benchmarks from benchmark-ips.

• methods_cache_version - implementation from this PR, without first line (counts all methods, for jruby and old rubies).
• all_available_methods - implementation from this pr
• all_available_methods_2 - with swapped sort & uniq
• all_available_methods_im - from your pr
• all_available_methods_im_2 - with swapped sort and uniq back (as in master)

My app setup:

> methods_cache_version
=> 33752 # methods count
> ObjectSpace.each_object(Module).count
=> 7024

methods_cache_version:      27.9 i/s
all_available_methods_im_2: 11.6 i/s - 2.41x  slower
all_available_methods_im:   10.8 i/s - 2.57x  slower
all_available_methods:      0.6 i/s  - 42.90x  slower
all_available_methods_2:    0.6 i/s  - 44.13x  slower

Adding RubyVM.stat if defined?(RubyVM.stat) as :mri:

                 mri:   905997.4 i/s
methods_cache_version:      27.0 i/s - 33540.07x  slower
all_available_methods_im:   12.7 i/s - 71228.09x  slower
all_available_methods_im_2: 11.4 i/s - 79257.78x  slower
all_available_methods_2:    0.6 i/s  - 1577378.45x  slower
all_available_methods:      0.6 i/s  - 1635400.51x  slower

So:

• Caching is faster than not-caching x2.4 times on old rubies, and jruby.
• and x33540 times on modern mri.
• I'll add stuff from your pr.

Really great, thanks! Do you agree we should use cache?

@printercu Could you please clarify the difference between methods_cache_version and all_available_methods?

methods_cache_version runs on every completion, and its result is used to check if cache is stale or not.
all_available_methods only runs to populate cache when its stale.

Ah, so these are the method names. Thanks, now I understand the results.

Regarding the non-MRI code path, I'm a bit uneasy that the computation of the cache key takes about 40% of the original computation. Have you tried asking JRuby guys about an alternative to RubyVM.stat? I could use it in a different project as well.

Do you agree we should use cache?

If 100ms is still too much for you, then sure. And on recent MRI it's blazing fast, so that's good.

But IMHO we should merge my change first: it's simpler, API-compatible, and while there's some risk in the module-traversal approach (as usual, due to misbehaving gems), it seems to hold up so far. Maybe even into the release branch?

I've included your change here. If your one would be merged first, I would rebase too much. I think it's just unnecessary work. And this one also does not provide any api change. omg

I've included your change here.

I didn't ask for that, though.

If your one would be merged first, I would rebase too much. I think it's just unnecessary work.

With some history rewriting, it shouldn't be too hard. Maybe by removing the commits that added my change, or just by taking the latest versions of the files, copying them over and making one commit. Keeping the intermediate is probably not worth it here.

Anyway, if my PR gets merged first, I can do the rebasing for you.

And this one also does not provide any api change. omg

It's a minor one, sure. But maybe there's a completer out there that depends on being instantiated anew each time, IDK.

Also, this patch might use a bit more cooking. You haven't replied to the question about JRuby's alternative to RubyVM.stat.

Why don't you want to talk to jruby guys yourself? Anything else here I should do for you?

i would prefer to merge #1588 first, it includes a significant speed improvement without using a cache (5s to 0.16s), and the implementation is less complex than this one. we could add the cache in a follow up PR, and since this PR adopted the changes in #1588 it should be possible to do.

@R-obert this pr includes #1588

#ignored_modules is that method. I've applied it, and added specs for it.

i know, read my message again :)

omg. i just don't understand why this hard way is required, if you still want to accept this pr. we will get exactly the same result, but i will also resolve conflicts.

#1588 is preferable because its implementation is much more simple but it also improves performance significantly. we might not need a cache anymore, and if we do it's better as a PR based on that work, so we can see the added complexity and if it is worth it or not. @dgutov also made some good points about issues in this PR.

Going to close this. #1588 was merged. If you want to add caching on top, please open another pull request.