summaryrefslogtreecommitdiff
Commit message (Collapse)AuthorAgeFilesLines
...
* | | Auto merge of #4818 - NickLaMuro:better_net_http_reuse_for_compact_index, ↵Homu2016-07-291-3/+15
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | r=indirect Allow for Thread reuse for CompactIndex fetcher Overview -------- One of the main benefits of the `Net::HTTP::Persistent` library is that it maintains the HTTP connections so that multiple requests to the same host don't need to re-instantiate the http tcp socket to make a request, and also allows this to be done across threads. But when used with the `CompactIndex` client, the each iteration of the `remaining_gems?` loop throws away the `Bundle::Worker` and initiates new Threads with new connections. Since the resulting work on these threads only returns to the caller the result, and can be collected cleanly, it is safe to reuse these threads by other repeated `gem_name` iterations and gain a bit of a performance boost in the process. While in this code change, we do run `@bundle_worker.stop`, we could simple skip this now as we will never use more then one `Bundle::Worker`'s worth of threads, keeping around the worker for further calls to `.specs` on the same fetcher. That said, this has not been don at this time. Metrics ------- This is the result of running the fetcher for fetching gem metadata against the Gemfile for the [ManageIQ](https://github.com/ManageIQ/manageiq) project (these are sorted collection of results from the quickest times to the slowest times from a collection of benchmarks made over time, and are not in any other order besides that): | No changes | Shared threads, 25 workers | Shared threads, 15 workers | | ---------- | --------------------------- | -------------------------- | | 14.780272 | 4.429732 | 4.424576 | | 14.987697 | 4.664379 | 4.447966 | | 16.161742 | 7.470335 | 4.596416 | | 16.698832 | 8.538736 | 8.103314 | | 16.973982 | 10.293958 | 10.368032 | | 17.044186 | 12.548805 | 10.873393 | | 17.401267 | 13.527048 | 11.734078 | | 17.417851 | 13.640801 | 12.379456 | | 17.611628 | 13.695791 | 14.071921 | | 17.702503 | 13.930277 | 14.151556 | | 17.926312 | 14.512490 | 14.247501 | | 17.953948 | 14.547216 | 14.372980 | | 18.116374 | 14.562247 | 14.494275 | | 18.193501 | 15.054224 | 14.496102 | | 18.278697 | 16.325938 | 14.496330 | | 18.313061 | 16.773398 | 14.799070 | | 18.561052 | 17.029987 | 15.377202 | | 18.657163 | 17.321094 | 15.658238 | | 18.661411 | 17.443478 | 17.018813 | | 18.739866 | 17.895521 | 17.142276 | | 19.008614 | 18.075015 | 17.414206 | | 19.174890 | 18.159239 | 17.860201 | | 19.235642 | 18.188842 | 18.460573 | The following changes to the `lib/bundler/source/rubygems.rb` were used to capture the metrics. ```diff diff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb index aedad70..2cd637b 100644 --- a/lib/bundler/source/rubygems.rb +++ b/lib/bundler/source/rubygems.rb @@ -350,11 +350,17 @@ module Bundler " Downloading full index instead..." unless allow_api if allow_api + require "benchmark" + puts Benchmark.measure { api_fetchers.each do |f| Bundler.ui.info "Fetching gem metadata from #{f.uri}", Bundler.ui.debug? idx.use f.specs_with_retry(dependency_names, self) Bundler.ui.info "" unless Bundler.ui.debug? # new line now that the dots are over end + } + + puts; puts; puts "exiting..." + exit ``` And the command used to capture the metrics using this branch: ``` $ for I in `seq 1 15`; do \ BUNDLE_DISABLE_POSTIT=1 ruby -I /path/to/bundler/lib /path/to/bundler/exe/bundle update; \ done ``` Some things to note about this data: - These results are the lowest of the recorded and usable results, not an average - I went with the lowest because is show the range of improvement the best, but I didn't get an even number of usable results, so this was the best way (I found) to share a somewhat unbiased distribution - Usable results were the following - Used the `compact_index` resolver - Didn't require a retry - Unfortunately, almost half of my results had to be thrown away because they didn't match that criteria One thing that I also observed while running in debug node and inspecting the threads is the it seems that one or two workers tend take a majority of the requests, while the other workers seem to only process one. This is why the metrics show a second column where I adjusted the number of workers spawned to 15 (no reflected in this code change), something to also consider. Tests ----- Wasn't able to run tests locally when I using the latest version of master (not sure what broke), and unsure how I would test this functionality. Let me know if this is something you would like to go forward with adding and I will look into adding some tests around it.
| * | | Allow for Thread reuse for CompactIndex fetcherNick LaMuro2016-07-271-3/+15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | One of the main benefits of the Net::HTTP persistent library is that it maintains the HTTP connections so that multiple requests to the same host don't need to re-instantiate the http tcp socket to make a request, and also allows this to be done across threads. But when used with the CompactIndex client, the each iteration of gem names throws away the `Bundle::Worker` and initiates new Threads with new connections. Since the resulting work on these threads only returns to the caller the result, and can be collected cleanly, it is safe to reuse these threads by other repeated `gem_name` iterations and gain a bit of a performance boost in the process. While in this code change, we do run `@bundle_worker.stop`, we could simple skip this now as we will never use more then one `Bundle::Worker`'s worth of threads, keeping around the worker for further calles to `.specs` on the same fetcher.
* | | | Auto merge of #4822 - bundler:seg-feature-flag-trampoline, r=indirectHomu2016-07-292-47/+51
|\ \ \ \ | |/ / / |/| | | | | | | | | | | | | | | | | | | | | | | Feature-flag the postit trampoline, disabled by default Set ENV["BUNDLE_ENABLE_TRAMPOLINE"] to enable it Closes #4821
| * | | Feature-flag the postit trampoline, disabled by defaultseg-feature-flag-trampolineSamuel Giddins2016-07-272-47/+51
|/ / / | | | | | | | | | Set ENV["BUNDLE_ENABLE_TRAMPOLINE"] to enable it
* | | Auto merge of #4811 - NickLaMuro:dots_for_compact_index_logger, r=segiddinsHomu2016-07-266-14/+77
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Fix random string printing inconsistencies and formatting Add dot output for CompactIndex fetcher --------------------------------------- The `Bundler::Fetcher::CompactIndex`'s logging was missing the "dot logging" when not in debug mode, which is an assumed behavior based on the code in `lib/bundler/source/rubygems.rb`: ```ruby api_fetchers.each do |f| Bundler.ui.info "Fetching gem metadata from #{f.uri}", Bundler.ui.debug? idx.use f.specs_with_retry(dependency_names, self) Bundler.ui.info "" unless Bundler.ui.debug? # new line now that the dots are over end ``` While this isn't critical for `bundler` function properly by any stretch of the imagination, it provides a small bit of user feedback that requests are still continuing to be processed and `bundler` hasn't stalled. It also maintains logging consistency between the different fetcher models. Add newlines for `Bundler::Retry` --------------------------------- This is very pedantic, but it makes it so that retry warning messages that used to look like this: ``` Fetching gem metadata from https://rubygems.org/....Retrying fetcher due to error (2/4): Error::OMGWatHappened LOL, no idea ..... ``` Now will look like this: ``` Fetching gem metadata from https://rubygems.org/.... Retrying fetcher due to error (2/4): Error::OMGWatHappened LOL, no idea..... ``` I think this reads a bit better and puts context to the "dots" so they now match up with the original attempt and the retries Remove unneeded if statement ---------------------------- Ran across this while failing at writing tests for the above (for longer than I want to admit in public), but basically the `if` statement to determine whether or not to print "name" in the `Bundler.ui.warn` was not needed, as it never could be reached because of a short circuiting return statement prior to the print ```ruby return true unless name ``` Have no idea how to test it anymore than we already are, so instead of embarrassing myself further, I moved on.
| * | | Removes extra if in Bundler::Retry interpolationNick LaMuro2016-07-251-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | This particular if statement will never return false because it always preceded by `return true unless name`, so it will always be true if we reach that line.
| * | | Update Bundler::Retry newlinesNick LaMuro2016-07-252-1/+35
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When using Bundler::Retry in conjunction with "dots" for regular printing, the retry messages show up inline with the dots, then the dots continue on the next line. It reads a bit better to have the retry messages show up on a new line after the dots, then allow the dots to continue on the same line, so that the dots now represent the status for the retry. When in debug, just print the retry message as we always have. The new line will assumed to not be necessary before the message, since the standard is to print newlines after everything in debug mode, and a newline will then also follow the retry message.
| * | | Update CompactIndex to use log_specsNick LaMuro2016-07-252-1/+31
| | | | | | | | | | | | | | | | | | | | | | | | | | | | `Bundler::Fetcher::CompactIndex` was missing the dots that are used by the fallback, `Bundler::Fetcher::Dependency` when running the `specs` method. This simply makes use of the newly extracted `log_specs` method instead of simply calling out to `Bundler.ui.debug`.
| * | | Move Bundler::Fetcher::Dependency#log_specs to BaseNick LaMuro2016-07-252-12/+11
| |/ / | | | | | | | | | | | | | | | Make log_specs a shared method between all fetchers, and require passing the debug message as the argument instead of the debug data, allowing it to be more universal.
* | | Auto merge of #4810 - NickLaMuro:fix_git_version_for_apple_git, r=segiddinsHomu2016-07-265-3/+102
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Removes OSX/msysgit strings from Bundler::Source::Git::GitProxy#version Overview -------- When running the previous version of `Bundler::Source::Git::GitProxy#version` output through `Gem::Version.create` when the git version is something like $ git --version git version 1.2.3 (Apple Git-22) Ruby blows up with the error: ArgumentError: Malformed version number string 1.2.3 (Apple Git-22) The '(Apple Git-22)' and any additions that are added by `msysgit` (ex: `1.8.3.msysgit.0`) can be safely ignored as they are additions to the git version string for those specific versions, but should still line up with the core git version. This regression was added in https://github.com/bundler/bundler/pull/4717, so this is not in a release of the gem yet, only in `1.13.0.rc1` installation. This prevented: * running tests on OSX or on any OS with a non-core git installation * running `bundle install/update` with any git gem dependencies in the Gemfile on OSX or any OS with a non-core git installation This change simply regexps those version additions out, in addition to removing the `git version`, so only the version number (containing numbers or `.`s) will be parsed by this regexp. Also, another method, `full_version` has been added to allow the `Bundler::Env` to report on the specific version of git being used, which is useful when error reports are submitted. Note about tests ---------------- The tests stub out any shell cmd and mocks a return value, which was easier to test the functionality of the regexp and requiring different git executable versions. I also didn't add integration tests to address the regression caused by #4717, but simply tested that the versions parsed would work when sent through `Gem::Version.create` This does mean I only tested a sample of git version types, so there might be something that I missed. Also, the test for `full_version` working in reports was kinda jank, but it was the best I could do...
| * | | Use .full_version in Bundler::EnvNick LaMuro2016-07-252-2/+13
| | | |
| * | | Add Bundler::Source::Git::GitProxy#full_versionNick LaMuro2016-07-252-0/+39
| | | | | | | | | | | | | | | | | | | | | | | | Adds a full_version method that can be used to display the OS specific info. Useful when printing the environment information in the Bundler::Env
| * | | Removes OSX/msysgit strings from git versionNick LaMuro2016-07-252-1/+48
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | When running the original `version` output through `Gem::Version.create` when the git version is something like $ git --version git version 1.2.3 (Apple Git-22) Ruby blows up with the error: ArgumentError: Malformed version number string 1.2.3 (Apple Git-22) The '(Apple Git-22)' and any additions that are added by `msysgit` (ex: 1.8.3.msysgit.0) can be safely ignored as they are additions to the git version for those specific versions. This was affecting running tests on OSX and being able to even bundle with git gem dependencies on OSX (with this version of the gem). This change simply regexes those rules out, in addition to removing the `git version`, so only the version number (containing numbers or `.`s) will be parsed.
| * | | Find git version in spec helper with git_proxyNick LaMuro2016-07-251-1/+3
| |/ / | | | | | | | | | | | | | | | | | | This is minor, but duplicated code that also can be found in `lib/bundler/source/git/git_proxy.rb`, so lets use that so we are finding the git version in the same manner. Bundler is fully loaded in the spec helper anyway, so this is already available to be used.
* | | Auto merge of #4808 - NickLaMuro:add_url_for_debug_http_response, r=segiddinsHomu2016-07-262-2/+2
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Add URI to http response output in debug mode Map request response to original URI when printing debug output for http responses for `bundler/fetcher/downloader`. Overview -------- I have recently been debugging some bundler stalling issues and found this addition to the debug output helpful when ruling out if a particular URL was timing out with the `CompactIndex` fetcher as it does it's requests over 25 threads and returns them asynchronously. Getting a bunch of `HTTP 304 Not Modified` isn't as helpful if you are trying to determine how a thread is locking up. Incidentally, this didn't end up helping solving some issues (further PRs will address this), but it seems like a simple debugging addition that could help rule out specific endpoint issues without requiring more [advanced debugging setups](https://gist.github.com/NickLaMuro/e923fc4e55307437a8f0e97ee6429410).
| * | | Add URI to http response output in debug modeNick LaMuro2016-07-252-2/+2
| |/ / | | | | | | | | | | | | Useful when mapping completed responses to requests when debugging to determine which requests are still in progress.
* | | Auto merge of #4800 - bundler:seg-locked-resolve-keep-platforms, r=indirectHomu2016-07-2613-38/+230
|\ \ \ | | | | | | | | | | | | | | | | | | | | | | | | | | | | [Definition] Ensure gemspec dependencies include all lockfile platforms Closes #4798 This is necessary since `gemspec` declares platforms for the dependency as the reverse platform map of the generic local platform, thus missing out on all those platforms that aren't currently being resolved for
| * | | Use reject! instead of select! for Ruby 1.8.7seg-locked-resolve-keep-platformsSamuel Giddins2016-07-251-1/+1
| | | |
| * | | Update specs for improved gemspec detectionSamuel Giddins2016-07-243-2/+4
| | | |
| * | | [DSL] Add support for multi-platform gems with the `gemspec` methodSamuel Giddins2016-07-223-15/+33
| | | |
| * | | Add install_gemfile! for specsSamuel Giddins2016-07-221-0/+1
| | | |
| * | | [Definition] Converge locked gemspec sources to Source::GemspecSamuel Giddins2016-07-223-10/+27
| | | |
| * | | Fix dev dependencies of gemspec installing on a prior platformSamuel Giddins2016-07-221-1/+1
| | | |
| * | | Make Bundler.locked_gems use the definition when loadedSamuel Giddins2016-07-222-9/+20
| | | |
| * | | Reduce code de-duplication in gemspec specSamuel Giddins2016-07-221-28/+14
| | | |
| * | | Add specs for bundling with locked platform-specific gemspec deps on RUBYSamuel Giddins2016-07-211-0/+146
| | | |
| * | | [Definition] Ensure gemspec dependencies include all lockfile platformsSamuel Giddins2016-07-211-0/+3
| | | | | | | | | | | | | | | | This avoids eliminating platform-specific gems from the lockfile when resolving on RUBY without any changes
| * | | [LazySpecification] Avoid hash collisions in #identifierSamuel Giddins2016-07-211-1/+3
| | | |
| * | | [LazySpecification] Add platform in #to_sSamuel Giddins2016-07-211-1/+5
| | | |
| * | | [LockfileParser] Ensure regexp global variables aren’t clobberedSamuel Giddins2016-07-211-2/+4
| |/ /
* | | Auto merge of #4802 - bundler:seg-fix-bundle-help, r=indirectHomu2016-07-262-4/+18
|\ \ \ | | | | | | | | | | | | | | | | | | | | Fix bundle --help Fixes the issue mentioned in https://github.com/bundler/bundler/issues/4801
| * | | Fix bundle --helpseg-fix-bundle-helpSamuel Giddins2016-07-242-4/+18
| |/ /
* | | Auto merge of #4805 - bundler:seg-should-be-installed-source, r=indirectHomu2016-07-264-15/+34
|\ \ \ | |/ / |/| | | | | | | | | | | Add support for checking source in should_be_installed I was on a ✈️. I got bored. This got written. EOS
| * | Update a pair of specs to use the new source matchingseg-should-be-installed-sourceSamuel Giddins2016-07-242-4/+5
| | |
| * | Add support for checking source in should_be_installedSamuel Giddins2016-07-241-9/+18
| | |
| * | Add a source.rb file by default in the specs to track the source for a gemSamuel Giddins2016-07-241-2/+11
|/ /
* | Auto merge of #4787 - bundler:seg-cleanup, r=indirectHomu2016-07-2135-111/+137
|\ \ | | | | | | | | | Random bits of cleanup ¯\_(ツ)_/¯
| * | [RubyGems] Avoid warning for Installer.new with a pathseg-cleanupSamuel Giddins2016-07-202-1/+7
| | |
| * | [RuboCop] Update to 0.41.2Samuel Giddins2016-07-2029-79/+107
| | |
| * | [Helpers] Use capture helper in capture_outputSamuel Giddins2016-07-201-8/+1
| | |
| * | [Bundler] Use DSLError when eval-ing a gemspec failsSamuel Giddins2016-07-206-17/+16
| | |
| * | [Bundler] Sort autoloadsSamuel Giddins2016-07-201-6/+6
| | |
* | | Auto merge of #4770 - bundler:seg-gem-dep-api-compatibility, r=indirectHomu2016-07-2011-87/+156
|\ \ \ | |/ / |/| | | | | | | | | | | [RubygemsIntegration] Add support for reversing hooks This is a pre-req for being able to use bundler as the backend for RubyGems' Gemfile support, as RubyGems tests run in a single process and the hooks installed (particularly by `replace_entrypoints`) are rather destructive to those tests. More changes to come to get `Gem.use_gemdeps` using bundler (i.e. to get the RG suite green), but this is likely to be the grossest of them.
| * | Mild 1.8.7 $SAFE=1 compatibilityseg-gem-dep-api-compatibilitySamuel Giddins2016-07-194-9/+10
| | |
| * | Fix legacy RubyGems compatibilitySamuel Giddins2016-07-192-6/+6
| | |
| * | [RubygemsIntegration] Move reset and post_reset hooks to instance methodsSamuel Giddins2016-07-192-7/+21
| | |
| * | [RubygemsIntegration] Only 1.8+ has post_reset hooksSamuel Giddins2016-07-191-1/+1
| | |
| * | [RubygemsIntegration] Fix <2 compat with returns in redefined methodsSamuel Giddins2016-07-191-3/+5
| | |
| * | [RubygemsIntegration] Support for old ruby & rubygemsSamuel Giddins2016-07-191-2/+5
| | |
| * | Fully reset in between each spec runSamuel Giddins2016-07-193-8/+10
| | |