{"action":"created","issue":{"url":"https://api.github.com/repos/monero-project/monero/issues/1291","repository_url":"https://api.github.com/repos/monero-project/monero","labels_url":"https://api.github.com/repos/monero-project/monero/issues/1291/labels{/name}","comments_url":"https://api.github.com/repos/monero-project/monero/issues/1291/comments","events_url":"https://api.github.com/repos/monero-project/monero/issues/1291/events","html_url":"https://github.com/monero-project/monero/pull/1291","id":186951298,"number":1291,"title":"adding thread_group for managing async tasks","user":{"login":"vtnerd","id":6564514,"avatar_url":"https://avatars.githubusercontent.com/u/6564514?v=3","gravatar_id":"","url":"https://api.github.com/users/vtnerd","html_url":"https://github.com/vtnerd","followers_url":"https://api.github.com/users/vtnerd/followers","following_url":"https://api.github.com/users/vtnerd/following{/other_user}","gists_url":"https://api.github.com/users/vtnerd/gists{/gist_id}","starred_url":"https://api.github.com/users/vtnerd/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vtnerd/subscriptions","organizations_url":"https://api.github.com/users/vtnerd/orgs","repos_url":"https://api.github.com/users/vtnerd/repos","events_url":"https://api.github.com/users/vtnerd/events{/privacy}","received_events_url":"https://api.github.com/users/vtnerd/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2016-11-02T23:57:45Z","updated_at":"2016-11-03T10:50:08Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/monero-project/monero/pulls/1291","html_url":"https://github.com/monero-project/monero/pull/1291","diff_url":"https://github.com/monero-project/monero/pull/1291.diff","patch_url":"https://github.com/monero-project/monero/pull/1291.patch"},"body":"Looking for general feedback (tests need to be written, etc., before merging)...\r\n\r\nThis was from a [discussion in a prior pull request](https://github.com/monero-project/monero/pull/1272). This does not use `asio::io_service` because it is a bit clunky to wait for all threads to complete `io::service::run()` (to guarantee that all handlers have completed), then restart the `io::service::run()` to re-use the threads - busy spin loops or condition variables would be needed which makes the implementation look similar to this. I added the thread re-use \"feature\" because one of the functions in `rctSigs.cpp` created two different `thread_group` objects, mainly so that the dispatched functions were guaranteed to complete before moving to the next section. Arguably, `std::future` should be used instead - retrieving values would be sequenced specifically with the associated tasks, which would require some additional allocations for the shared_state inside of `std::future` (and thus be some amount slower). Or maybe the `thread_group` should be a \"one shot\" like it is currently (i.e. cannot re-use threads after synchronization). So the obvious choices are: (1) no change, (2) this, (3) this style with `asio::io_service` and thread joining after each group of tasks, or (4) this style with `asio::io_service` and `std::future` for blocking after each group of tasks.\r\n\r\nThis can be used by blockchain.cpp and wallet2.cpp which requires similar changes. Also:\r\n- No valgrind errors or leaks\r\n- The class automatically ensures that threads join on destruction\r\n- Its easy to block until all functions complete without introducing a bug\r\n- The current thread can block until all work has completed without `join`ing the thread, allowing for thread-reuse (see `rctSigs.cpp`)\r\n- The execution times of the tests are really close before and after the patch on my VM.\r\n\r\nDisadvantages:\r\n- Using `asio::io_service` would allow for running I/O events on the same threads (I didn't see any obvious place where a refactor would allow this).\r\n- `asio::io_service` would have less hand-rolled stuff"},"comment":{"url":"https://api.github.com/repos/monero-project/monero/issues/comments/258111755","html_url":"https://github.com/monero-project/monero/pull/1291#issuecomment-258111755","issue_url":"https://api.github.com/repos/monero-project/monero/issues/1291","id":258111755,"user":{"login":"hyc","id":306354,"avatar_url":"https://avatars.githubusercontent.com/u/306354?v=3","gravatar_id":"","url":"https://api.github.com/users/hyc","html_url":"https://github.com/hyc","followers_url":"https://api.github.com/users/hyc/followers","following_url":"https://api.github.com/users/hyc/following{/other_user}","gists_url":"https://api.github.com/users/hyc/gists{/gist_id}","starred_url":"https://api.github.com/users/hyc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hyc/subscriptions","organizations_url":"https://api.github.com/users/hyc/orgs","repos_url":"https://api.github.com/users/hyc/repos","events_url":"https://api.github.com/users/hyc/events{/privacy}","received_events_url":"https://api.github.com/users/hyc/received_events","type":"User","site_admin":false},"created_at":"2016-11-03T10:50:08Z","updated_at":"2016-11-03T10:50:08Z","body":"I've done 1 quick pass thru this commit and the previous PR. My general impression is this looks like a good way to go. In general I would prefer that we have 1 persistent thread pool instead of spawning and killing threads everywhere we use them. Will look in more detail later."}}
{"actor":{"display_login":"hyc"}}