{"action":"created","issue":{"url":"https://api.github.com/repos/mantidproject/mantid/issues/19403","repository_url":"https://api.github.com/repos/mantidproject/mantid","labels_url":"https://api.github.com/repos/mantidproject/mantid/issues/19403/labels{/name}","comments_url":"https://api.github.com/repos/mantidproject/mantid/issues/19403/comments","events_url":"https://api.github.com/repos/mantidproject/mantid/issues/19403/events","html_url":"https://github.com/mantidproject/mantid/pull/19403","id":222762774,"number":19403,"title":"API::ComponentInfo, API::DetectorInfo std::unorderd_map insert performance","user":{"login":"OwenArnold","id":1109536,"avatar_url":"https://avatars0.githubusercontent.com/u/1109536?v=3","gravatar_id":"","url":"https://api.github.com/users/OwenArnold","html_url":"https://github.com/OwenArnold","followers_url":"https://api.github.com/users/OwenArnold/followers","following_url":"https://api.github.com/users/OwenArnold/following{/other_user}","gists_url":"https://api.github.com/users/OwenArnold/gists{/gist_id}","starred_url":"https://api.github.com/users/OwenArnold/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/OwenArnold/subscriptions","organizations_url":"https://api.github.com/users/OwenArnold/orgs","repos_url":"https://api.github.com/users/OwenArnold/repos","events_url":"https://api.github.com/users/OwenArnold/events{/privacy}","received_events_url":"https://api.github.com/users/OwenArnold/received_events","type":"User","site_admin":false},"labels":[{"id":154208631,"url":"https://api.github.com/repos/mantidproject/mantid/labels/State:%20In%20Progress","name":"State: In Progress","color":"fbca04","default":false}],"state":"open","locked":false,"assignee":{"login":"SimonHeybrock","id":12912489,"avatar_url":"https://avatars2.githubusercontent.com/u/12912489?v=3","gravatar_id":"","url":"https://api.github.com/users/SimonHeybrock","html_url":"https://github.com/SimonHeybrock","followers_url":"https://api.github.com/users/SimonHeybrock/followers","following_url":"https://api.github.com/users/SimonHeybrock/following{/other_user}","gists_url":"https://api.github.com/users/SimonHeybrock/gists{/gist_id}","starred_url":"https://api.github.com/users/SimonHeybrock/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SimonHeybrock/subscriptions","organizations_url":"https://api.github.com/users/SimonHeybrock/orgs","repos_url":"https://api.github.com/users/SimonHeybrock/repos","events_url":"https://api.github.com/users/SimonHeybrock/events{/privacy}","received_events_url":"https://api.github.com/users/SimonHeybrock/received_events","type":"User","site_admin":false},"assignees":[{"login":"SimonHeybrock","id":12912489,"avatar_url":"https://avatars2.githubusercontent.com/u/12912489?v=3","gravatar_id":"","url":"https://api.github.com/users/SimonHeybrock","html_url":"https://github.com/SimonHeybrock","followers_url":"https://api.github.com/users/SimonHeybrock/followers","following_url":"https://api.github.com/users/SimonHeybrock/following{/other_user}","gists_url":"https://api.github.com/users/SimonHeybrock/gists{/gist_id}","starred_url":"https://api.github.com/users/SimonHeybrock/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SimonHeybrock/subscriptions","organizations_url":"https://api.github.com/users/SimonHeybrock/orgs","repos_url":"https://api.github.com/users/SimonHeybrock/repos","events_url":"https://api.github.com/users/SimonHeybrock/events{/privacy}","received_events_url":"https://api.github.com/users/SimonHeybrock/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/mantidproject/mantid/milestones/61","html_url":"https://github.com/mantidproject/mantid/milestone/61","labels_url":"https://api.github.com/repos/mantidproject/mantid/milestones/61/labels","id":2064897,"number":61,"title":"Release 3.10","description":"","creator":{"login":"NickDraper","id":1017173,"avatar_url":"https://avatars2.githubusercontent.com/u/1017173?v=3","gravatar_id":"","url":"https://api.github.com/users/NickDraper","html_url":"https://github.com/NickDraper","followers_url":"https://api.github.com/users/NickDraper/followers","following_url":"https://api.github.com/users/NickDraper/following{/other_user}","gists_url":"https://api.github.com/users/NickDraper/gists{/gist_id}","starred_url":"https://api.github.com/users/NickDraper/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NickDraper/subscriptions","organizations_url":"https://api.github.com/users/NickDraper/orgs","repos_url":"https://api.github.com/users/NickDraper/repos","events_url":"https://api.github.com/users/NickDraper/events{/privacy}","received_events_url":"https://api.github.com/users/NickDraper/received_events","type":"User","site_admin":false},"open_issues":270,"closed_issues":534,"state":"open","created_at":"2016-10-12T13:14:59Z","updated_at":"2017-04-20T01:48:23Z","due_on":"2017-06-12T07:00:00Z","closed_at":null},"comments":1,"created_at":"2017-04-19T14:44:28Z","updated_at":"2017-04-20T04:41:23Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mantidproject/mantid/pulls/19403","html_url":"https://github.com/mantidproject/mantid/pull/19403","diff_url":"https://github.com/mantidproject/mantid/pull/19403.diff","patch_url":"https://github.com/mantidproject/mantid/pull/19403.patch"},"body":"Upon completion BinMD copies `ExperimentInfo`s from input to output workspaces. Since BinMD is used interactively as part of the SliceViewer, it is executed many, many times, and the slow down in this copying process has become rather noticeable. \r\n\r\nThe vast majority of the time is spent inserting mapping key-value pairs for `ComponentID`->`ComponentIndex` and `DetectorID`->`DetectorIndex` into the respective `std::unordered_maps` in the constructors of `API::ComponentInfo` and `API::DetectorInfo`. \r\n\r\n**While the problem is quite clear. I am happy to debate the exact fix** as there does not seem to be an approach that is completely satisfactory. **Here are things I have tried**.\r\n\r\n* Reserving the memory for the `std::unordered_map`s. This does make a small ~10% improvement, and I have left this small optimization in place, since we do know the sizes of each of these mappings aprori.\r\n* Tried different container `std::map` on the basis that insertion is less costly. However, you cannot pre-allocate this data structure, and this caused a further slowdown.\r\n* Tried different contatiner `boost::container::flat_tree`. This allows pre-allocation of the mapping, and sorts the contents, so insertion should be good, but this was again significantly slower.\r\n* When an Instrument is set (or reset) on an `ExperimentInfo` we do not expect that base instrument to be different. We should therefore expect that `ComponentID`->`ComponentIndex` and  `DetectorID`->`DetectorIndex`  are immutable! Could we therefore cache the mappings outside of the `ComponentInfo` and `DetectorInfo` for reuse? Code for this starts looking quite ugly and we end up leaking a lot that is currently encapsulated.\r\n\r\nWhat has been implemented here is lazy calculation of the mappings. These don't get calculated until `indexOf` is first called. This does dramatically improve the performance, however, thread synchronization now needs to be considered and this has an overhead.\r\n\r\n**To test:**\r\n\r\n- Fuzzy test - run BinMD in-situ such as in in the SliceViewer. It should be as interactive as v3.9.0\r\n- Time `ExperimentInfo::setInstrument` this should be significantly faster than master and closer to v3.9.0 speeds.\r\n\r\nFixes #19385 \r\n\r\n\r\n---\r\n\r\n#### Reviewer ####\r\n\r\nPlease comment on the following ([full description](http://www.mantidproject.org/Individual_Ticket_Testing)):\r\n\r\n##### Code Review #####\r\n\r\n- [ ] Is the code of an acceptable quality?\r\n- [ ] Does the code conform to the [coding standards](http://www.mantidproject.org/Coding_Standards)? Is it well structured with small focussed classes/methods/functions?\r\n- [ ] Are there unit/system tests in place? Are the unit tests small and test the a class in isolation?\r\n- [ ] If there are changes in the release notes then do they describe the changes appropriately?\r\n\r\n##### Functional Tests #####\r\n\r\n- [ ] Do changes function as described? Add comments below that describe the tests performed?\r\n- [ ] How do the changes handle unexpected situations, e.g. bad input?\r\n- [ ] Has the relevant documentation been added/updated?\r\n- [ ] Is user-facing documentation written in a user-friendly manner?\r\n- [ ] Has developer documentation been updated if required?\r\n\r\n- Does everything look good? Comment with the ship it emoji but don't merge. A member of `@mantidproject/gatekeepers` will take care of it.\r\n"},"comment":{"url":"https://api.github.com/repos/mantidproject/mantid/issues/comments/295578568","html_url":"https://github.com/mantidproject/mantid/pull/19403#issuecomment-295578568","issue_url":"https://api.github.com/repos/mantidproject/mantid/issues/19403","id":295578568,"user":{"login":"SimonHeybrock","id":12912489,"avatar_url":"https://avatars2.githubusercontent.com/u/12912489?v=3","gravatar_id":"","url":"https://api.github.com/users/SimonHeybrock","html_url":"https://github.com/SimonHeybrock","followers_url":"https://api.github.com/users/SimonHeybrock/followers","following_url":"https://api.github.com/users/SimonHeybrock/following{/other_user}","gists_url":"https://api.github.com/users/SimonHeybrock/gists{/gist_id}","starred_url":"https://api.github.com/users/SimonHeybrock/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SimonHeybrock/subscriptions","organizations_url":"https://api.github.com/users/SimonHeybrock/orgs","repos_url":"https://api.github.com/users/SimonHeybrock/repos","events_url":"https://api.github.com/users/SimonHeybrock/events{/privacy}","received_events_url":"https://api.github.com/users/SimonHeybrock/received_events","type":"User","site_admin":false},"created_at":"2017-04-20T04:41:23Z","updated_at":"2017-04-20T04:41:23Z","body":"I would strongly prefer the last option you mention (carrying the maps along with `Instrument`). If the execution of a pice of code can be avoided (or the execution count drastically reduced as in this case) I think that is always preferable to optimizing it. Optimized and threaded code is much harder to debug.\r\n\r\nRegarding uglyness and leaking encapsulation of that last option: We are already storing `Beamline::DetectorInfo`, `Beamline::ComponentInfo`, and a vector of component IDs in the `Instrument`. Now we want to add two more maps to that list. I think this would be the right time to introduce a little `struct` holding these objects together, which might eventually develop into a new `Instrument` class (i.e., the `Instrument` equivalent for Instrument-2.0)."}}
{"actor":{"display_login":"SimonHeybrock"}}