{"action":"created","issue":{"url":"https://api.github.com/repos/mantidproject/mantid/issues/16382","repository_url":"https://api.github.com/repos/mantidproject/mantid","labels_url":"https://api.github.com/repos/mantidproject/mantid/issues/16382/labels{/name}","comments_url":"https://api.github.com/repos/mantidproject/mantid/issues/16382/comments","events_url":"https://api.github.com/repos/mantidproject/mantid/issues/16382/events","html_url":"https://github.com/mantidproject/mantid/pull/16382","id":156508104,"number":16382,"title":"Remove locking mechanism from ISpectrum.","user":{"login":"SimonHeybrock","id":12912489,"avatar_url":"https://avatars.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},"labels":[{"url":"https://api.github.com/repos/mantidproject/mantid/labels/Component:%20Framework","name":"Component: Framework","color":"009800"},{"url":"https://api.github.com/repos/mantidproject/mantid/labels/Misc:%20Core","name":"Misc: Core","color":"5319e7"}],"state":"open","locked":false,"assignee":{"login":"martyngigg","id":733773,"avatar_url":"https://avatars.githubusercontent.com/u/733773?v=3","gravatar_id":"","url":"https://api.github.com/users/martyngigg","html_url":"https://github.com/martyngigg","followers_url":"https://api.github.com/users/martyngigg/followers","following_url":"https://api.github.com/users/martyngigg/following{/other_user}","gists_url":"https://api.github.com/users/martyngigg/gists{/gist_id}","starred_url":"https://api.github.com/users/martyngigg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/martyngigg/subscriptions","organizations_url":"https://api.github.com/users/martyngigg/orgs","repos_url":"https://api.github.com/users/martyngigg/repos","events_url":"https://api.github.com/users/martyngigg/events{/privacy}","received_events_url":"https://api.github.com/users/martyngigg/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/mantidproject/mantid/milestones/58","html_url":"https://github.com/mantidproject/mantid/milestones/Release%203.8","labels_url":"https://api.github.com/repos/mantidproject/mantid/milestones/58/labels","id":1505614,"number":58,"title":"Release 3.8","description":"","creator":{"login":"granrothge","id":5910574,"avatar_url":"https://avatars.githubusercontent.com/u/5910574?v=3","gravatar_id":"","url":"https://api.github.com/users/granrothge","html_url":"https://github.com/granrothge","followers_url":"https://api.github.com/users/granrothge/followers","following_url":"https://api.github.com/users/granrothge/following{/other_user}","gists_url":"https://api.github.com/users/granrothge/gists{/gist_id}","starred_url":"https://api.github.com/users/granrothge/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/granrothge/subscriptions","organizations_url":"https://api.github.com/users/granrothge/orgs","repos_url":"https://api.github.com/users/granrothge/repos","events_url":"https://api.github.com/users/granrothge/events{/privacy}","received_events_url":"https://api.github.com/users/granrothge/received_events","type":"User","site_admin":false},"open_issues":614,"closed_issues":144,"state":"open","created_at":"2016-01-13T20:00:53Z","updated_at":"2016-06-17T08:22:39Z","due_on":"2016-10-09T23:00:00Z","closed_at":null},"comments":8,"created_at":"2016-05-24T13:32:23Z","updated_at":"2016-06-17T08:48:36Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mantidproject/mantid/pulls/16382","html_url":"https://github.com/mantidproject/mantid/pull/16382","diff_url":"https://github.com/mantidproject/mantid/pull/16382.diff","patch_url":"https://github.com/mantidproject/mantid/pull/16382.patch"},"body":"See #16378 for a description.\r\n\r\n**To test:**\r\n\r\nCode review, try to understand the MRU and locking mechanism thoroughly. In particular, try to verify that my understanding and reasoning is correct:\r\n\r\n- Each thread has its own MRU.\r\n- Therefore, locking is only needed if there is a risk that a particular thread kicks its own buffered data from the MRU, i.e., in code like this:\r\n\r\n  ```cpp\r\n  auto &y0 = eventWS->readY(0);\r\n  for(size_t i=0; i<eventWS->numberOfHistograms(); ++i) {\r\n    auto &y = eventWS->readY(i);\r\n    // y0 becomes an invalid reference after 50 iterations,\r\n    // if we need it we would need to lock\r\n  }\r\n  ```\r\n- Code like this does not seem to be present anywhere in the cases where `ISpectrum::lockData()` is used currently, so we can drop it completely.\r\n\r\nPlease also see below for my two comments regarding Poco threading.\r\n\r\nFixes #16378.\r\n\r\n*Internal change, does not need to be in the release notes.*\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- [x] Is the code of an acceptable quality?\r\n- [x] 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- [x] 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/226715338","html_url":"https://github.com/mantidproject/mantid/pull/16382#issuecomment-226715338","issue_url":"https://api.github.com/repos/mantidproject/mantid/issues/16382","id":226715338,"user":{"login":"martyngigg","id":733773,"avatar_url":"https://avatars.githubusercontent.com/u/733773?v=3","gravatar_id":"","url":"https://api.github.com/users/martyngigg","html_url":"https://github.com/martyngigg","followers_url":"https://api.github.com/users/martyngigg/followers","following_url":"https://api.github.com/users/martyngigg/following{/other_user}","gists_url":"https://api.github.com/users/martyngigg/gists{/gist_id}","starred_url":"https://api.github.com/users/martyngigg/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/martyngigg/subscriptions","organizations_url":"https://api.github.com/users/martyngigg/orgs","repos_url":"https://api.github.com/users/martyngigg/repos","events_url":"https://api.github.com/users/martyngigg/events{/privacy}","received_events_url":"https://api.github.com/users/martyngigg/received_events","type":"User","site_admin":false},"created_at":"2016-06-17T08:48:36Z","updated_at":"2016-06-17T08:48:36Z","body":"I agree with Simon on the changes here and the code looks fine. \r\n\r\nThe parallel code in `ConvertToDiffractionMDWorkspace` version 1 looks neater now with OpenMP loop rather than the more verbose thread tasks approach.\r\n\r\nHappy to :shipit: "}}
{"actor":{"display_login":"martyngigg"}}