{"action":"created","issue":{"url":"https://api.github.com/repos/bundler/bundler/issues/4818","repository_url":"https://api.github.com/repos/bundler/bundler","labels_url":"https://api.github.com/repos/bundler/bundler/issues/4818/labels{/name}","comments_url":"https://api.github.com/repos/bundler/bundler/issues/4818/comments","events_url":"https://api.github.com/repos/bundler/bundler/issues/4818/events","html_url":"https://github.com/bundler/bundler/pull/4818","id":167695282,"number":4818,"title":"Allow for Thread reuse for CompactIndex fetcher","user":{"login":"NickLaMuro","id":314014,"avatar_url":"https://avatars.githubusercontent.com/u/314014?v=3","gravatar_id":"","url":"https://api.github.com/users/NickLaMuro","html_url":"https://github.com/NickLaMuro","followers_url":"https://api.github.com/users/NickLaMuro/followers","following_url":"https://api.github.com/users/NickLaMuro/following{/other_user}","gists_url":"https://api.github.com/users/NickLaMuro/gists{/gist_id}","starred_url":"https://api.github.com/users/NickLaMuro/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NickLaMuro/subscriptions","organizations_url":"https://api.github.com/users/NickLaMuro/orgs","repos_url":"https://api.github.com/users/NickLaMuro/repos","events_url":"https://api.github.com/users/NickLaMuro/events{/privacy}","received_events_url":"https://api.github.com/users/NickLaMuro/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":9,"created_at":"2016-07-26T19:49:38Z","updated_at":"2016-07-29T00:10:36Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/bundler/bundler/pulls/4818","html_url":"https://github.com/bundler/bundler/pull/4818","diff_url":"https://github.com/bundler/bundler/pull/4818.diff","patch_url":"https://github.com/bundler/bundler/pull/4818.patch"},"body":"Overview\r\n--------\r\nOne 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.\r\n\r\nSince 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.\r\n\r\nWhile 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.\r\n\r\n\r\nMetrics\r\n-------\r\nThis 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):\r\n\r\n| No changes | Shared threads, 25 workers  | Shared threads, 15 workers |\r\n| ---------- | --------------------------- | -------------------------- |\r\n|  14.780272 |                    4.429732 |                   4.424576 |\r\n|  14.987697 |                    4.664379 |                   4.447966 |\r\n|  16.161742 |                    7.470335 |                   4.596416 |\r\n|  16.698832 |                    8.538736 |                   8.103314 |\r\n|  16.973982 |                   10.293958 |                  10.368032 |\r\n|  17.044186 |                   12.548805 |                  10.873393 |\r\n|  17.401267 |                   13.527048 |                  11.734078 |\r\n|  17.417851 |                   13.640801 |                  12.379456 |\r\n|  17.611628 |                   13.695791 |                  14.071921 |\r\n|  17.702503 |                   13.930277 |                  14.151556 |\r\n|  17.926312 |                   14.512490 |                  14.247501 |\r\n|  17.953948 |                   14.547216 |                  14.372980 |\r\n|  18.116374 |                   14.562247 |                  14.494275 |\r\n|  18.193501 |                   15.054224 |                  14.496102 |\r\n|  18.278697 |                   16.325938 |                  14.496330 |\r\n|  18.313061 |                   16.773398 |                  14.799070 |\r\n|  18.561052 |                   17.029987 |                  15.377202 |\r\n|  18.657163 |                   17.321094 |                  15.658238 |\r\n|  18.661411 |                   17.443478 |                  17.018813 |\r\n|  18.739866 |                   17.895521 |                  17.142276 |\r\n|  19.008614 |                   18.075015 |                  17.414206 |\r\n|  19.174890 |                   18.159239 |                  17.860201 |\r\n|  19.235642 |                   18.188842 |                  18.460573 |\r\n\r\n\r\nThe following changes to the `lib/bundler/source/rubygems.rb` were used to capture the metrics.\r\n\r\n```diff\r\ndiff --git a/lib/bundler/source/rubygems.rb b/lib/bundler/source/rubygems.rb\r\nindex aedad70..2cd637b 100644\r\n--- a/lib/bundler/source/rubygems.rb\r\n+++ b/lib/bundler/source/rubygems.rb\r\n@@ -350,11 +350,17 @@ module Bundler\r\n             \" Downloading full index instead...\" unless allow_api\r\n\r\n           if allow_api\r\n+            require \"benchmark\"\r\n+            puts Benchmark.measure {\r\n             api_fetchers.each do |f|\r\n               Bundler.ui.info \"Fetching gem metadata from #{f.uri}\", Bundler.ui.debug?\r\n               idx.use f.specs_with_retry(dependency_names, self)\r\n               Bundler.ui.info \"\" unless Bundler.ui.debug? # new line now that the dots are over\r\n             end\r\n+            }\r\n+\r\n+            puts; puts; puts \"exiting...\"\r\n+            exit\r\n```\r\n\r\nAnd the command used to capture the metrics using this branch:\r\n\r\n```\r\n$ for I in `seq 1 15`; do \\\r\n    BUNDLE_DISABLE_POSTIT=1 ruby -I /path/to/bundler/lib /path/to/bundler/exe/bundle update; \\\r\n  done\r\n```\r\n\r\n\r\n\r\nSome things to note about this data:\r\n\r\n- These results are the lowest of the recorded and usable results, not an average\r\n- 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\r\n- Usable results were the following\r\n  - Used the `compact_index` resolver\r\n  - Didn't require a retry\r\n- Unfortunately, almost half of my results had to be thrown away because they didn't match that criteria\r\n\r\nOne 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.\r\n\r\n\r\nTests\r\n-----\r\nWasn'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."},"comment":{"url":"https://api.github.com/repos/bundler/bundler/issues/comments/236061375","html_url":"https://github.com/bundler/bundler/pull/4818#issuecomment-236061375","issue_url":"https://api.github.com/repos/bundler/bundler/issues/4818","id":236061375,"user":{"login":"homu","id":10212162,"avatar_url":"https://avatars.githubusercontent.com/u/10212162?v=3","gravatar_id":"","url":"https://api.github.com/users/homu","html_url":"https://github.com/homu","followers_url":"https://api.github.com/users/homu/followers","following_url":"https://api.github.com/users/homu/following{/other_user}","gists_url":"https://api.github.com/users/homu/gists{/gist_id}","starred_url":"https://api.github.com/users/homu/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/homu/subscriptions","organizations_url":"https://api.github.com/users/homu/orgs","repos_url":"https://api.github.com/users/homu/repos","events_url":"https://api.github.com/users/homu/events{/privacy}","received_events_url":"https://api.github.com/users/homu/received_events","type":"User","site_admin":false},"created_at":"2016-07-29T00:10:35Z","updated_at":"2016-07-29T00:10:35Z","body":":sunny: Test successful - [status](https://travis-ci.org/bundler/bundler/builds/148166578)"}}
{"actor":{"display_login":"homu"}}