-
Notifications
You must be signed in to change notification settings - Fork 601
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
Improve completion speed in large applications #1588
Conversation
Traversing via constant lookup is constant-time, as opposed to O(number of defined modules). And it doesn't use Module#name. So this is fast.
Like Argon2::Password in argon2 < v1.1.1.
irb_exit is defined on IRB's singleton class.
uniq! uses a hash table anyway. But now sort! needs to deal with a shorter list.
end | ||
next if name != "IRB::Context" and | ||
/^(IRB|SLex|RubyLex|RubyToken)/ =~ name | ||
next if to_ignore.include?(m) rescue true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3.times { |i| next if asd rescue true; puts i }
prints 0,1,2. Is it expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why rescue is here, while ignored_modules
returns array all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignored_modules
returns a set, but that's not why this is here, see the message in 353a42b.
The previous versions of Argon2::Password
overrode self.hash
. And Set#include?
calls #hash
on its argument, which in this case raised ArgumentError
. So this is to deal with misbehaving third-party code.
Not sure if we can do better; using a set is measurably faster here than using an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. If such modules should be skipped, should i change it to next if (to_ignore.include?(m) rescue true)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be fine, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, thanks for the catch.
Copied this one on top of #1583, benchmarks at #1583 (comment) |
I would love to see this PR merge. 💕 |
LGTM. |
yeah but the implementation in this PR is more simple and still includes a significant speed up (5s to 0.16s). we could add the cache in a follow up pull request. it would probably be easier to review then as well. if i understood correctly, with a cache we gain 100ms speed improvement? |
@R-obert Thanks for taking a look. Anything I can do to speed this along? What are the odds of getting this into a release soon? |
i'm not sure when the next release will be, but i think this is good to merge. let's just wait and see if anyone else has some input. |
@R-obert So 0.11 is a definite no? I could rebase onto v0.11.0.pre, if that might be considered appropriate. Admittedly, I don't know the reasons for this long pre-release cycle. |
@banister what do you think? can we just release master? |
hey @dgutov - what's the best way to test the performance improvement? i tried this locally in a work app, and i didn't notice any difference with or without your patch. |
@R-obert Please see my comment in #1540 (comment). |
That's the benchmark. With the patch, you should see entirely different (better) numbers. |
thanks! i can confirm |
@dgutov thanks again. |
Woohoo! |
pkgsrc change: add support for pkg_alternatives ### HEAD #### Features * Add Pry::Testable, an improved modular replacement for PryTestHelpers. **breaking change**. See pull request [#1679](pry/pry#1679). * Add a new category module: "Pry::Platform". Loosely related to #1668 below. See pull request [#1670](pry/pry#1670) * Add `mac_osx?` and `linux?` utility functions to Pry::Helpers::BaseHelpers. See pull request [#1668](pry/pry#1668). * Add utility functions for drawing colorised text on a colorised background. See pull request [#1673](pry/pry#1673). #### Bug fixes * Fix a case of infinite recursion in `Pry::Method::WeirdMethodLocator#find_method_in_superclass` that users of the [Hanami](http://hanamirb.org/) web framework experienced and reported since 2015. See pull request [#1639](pry/pry#1689). * Fix a bug where Method objects were not returned for setters inherited from a default (Pry::Config::Default). Eg, this is no longer an error: pry(main)> d = Pry::Config.from_hash({}, Pry::Config::Default.new) pry(main)> d.method(:exception_whitelist=) # Error See pull request [#1688](pry/pry#1688). * Do not capture unused Proc objects in Text helper methods `no_color` and `no_paging`, for performance reasons. Improve the documentation of both methods. See pull request [#1691](pry/pry#1691). * Fix `String#pp` output color. See pull request [#1674](pry/pry#1674). ### 0.11.0 * Add alias 'whereami[?!]+' for 'whereami' command. ([#1597](pry/pry#1597)) * Improve Ruby 2.4 support ([#1611](pry/pry#1611)): * Deprecated constants are hidden from `ls` output by default, use the `-d` switch to see them. * Fix warnings that originate in Pry while using the repl. * Improve completion speed in large applications. ([#1588](pry/pry#1588)) * Pry::ColorPrinter.pp: add `newline` argument and pass it on to PP. ([#1603](pry/pry#1603)) * Use `less` or system pager pager on MS Windows if it is available. ([#1512](pry/pry#1512)) * Add `Pry.configure` as an alternative to the current way of changing configuration options in `.pryrc` files. ([#1502](pry/pry#1502)) * Add `Pry::Config::Behavior#eager_load!` to add a possible workaround for issues like ([#1501](pry/pry#1501)) * Remove Slop as a runtime dependency by vendoring v3.4 as Pry::Slop. People can depend on Slop v4 and Pry at the same time without running into version conflicts. ([#1497](pry/pry#1497)) * Fix auto-indentation of code that uses a single-line rescue ([#1450](pry/pry#1450)) * Remove "Pry::Config#refresh", please use "Pry::Config#clear" instead. * Defining a method called "ls" no longer breaks the "ls" command ([#1407](pry/pry#1407)) * Don't raise when directory permissions don't allow file expansion ([#1432](pry/pry#1432)) * Syntax highlight <tt> tags in documentation output. * Add support for BasicObject subclasses who implement their own #inspect (#1341) * Fix 'include RSpec::Matchers' at the top-level (#1277) * Add 'gem-readme' command, prints the README file bundled with a rubygem * Add 'gem-search' command, searches for a gem with the rubygems.org HTTP API * Fixed bug in the `cat` command where it was impossible to use line numbers with files ([#1349](pry/pry#1349)) * Fixed uncaught Errno::EOPNOTSUPP exception when $stdout is a socket ([#1352](pry/pry#1352)) * Display a warning when you cd'ed inside a C object and executed 'show-source' without arguments ([#691](pry/pry#691)) * Make the stagger_output method more reliable by reusing possibly available Pry instance ([#1364](pry/pry#1364)) * Make the 'gem-install' message less confusing by removing backticks ([#1350](pry/pry#1350)) * Fixed error when Pry was trying to load incompatible versions of plugins ([#1312](pry/pry#1312)) * Fixed bug when `hist --clear` led to ArgumentError ([#1340](pry/pry#1340)) * Fixed the "uninitialized constant Pry::ObjectPath::StringScanner" exception during autocomplete ([#1330](pry/pry#1330)) * Secured usage of colours with special characters (RL_PROMPT_START_IGNORE and RL_PROMPT_END_IGNORE) in Pry::Helpers::Text ([#493](pry/pry#493 (comment))) * Fixed regression with `pry -e` when it messes the terminal ([#1387](pry/pry#1387)) * Fixed regression with space prefixes of expressions ([#1369](pry/pry#1369)) * Introduced the new way to define hooks for commands (with `Pry.hooks.add_hook("{before,after}_commandName")`). The old way is deprecated, but still supported (with `Pry.commands.{before,after}_command`) ([#651](pry/pry#651)) * Removed old API's using `Pry::Hooks.from_hash` altogether * Removed hints on Foreman support (see [this](ddollar/foreman#536)) * Fixed support for the tee command ([#1334](pry/pry#1334)) * Implemented support for CDPATH for ShellCommand ([#1433](pry/pry#1433), [#1434](pry/pry#1434)) * `Pry::CLI.parse_options` does not start Pry anymore ([#1393](pry/pry#1393)) * The gem uses CPU-less platforms for Windows now ([#1410](pry/pry#1410)) * Add `Pry::Config::Memoization` to make it easier to implement your own `Pry::Config::Default` class.([#1503](pry/pry#1503)) * Lazy load the config defaults for `Pry.config.history` and `Pry.config.gist`.
This is another attempt to improve completion speed (e.g. #1540). It's a non-caching solution which improved full scan completion speed in our production app from
5s
to0.16s
.Even if we end up adding cache, I'd rather it didn't take long to populate.