{"action":"created","issue":{"url":"https://api.github.com/repos/GafferHQ/gaffer/issues/2251","repository_url":"https://api.github.com/repos/GafferHQ/gaffer","labels_url":"https://api.github.com/repos/GafferHQ/gaffer/issues/2251/labels{/name}","comments_url":"https://api.github.com/repos/GafferHQ/gaffer/issues/2251/comments","events_url":"https://api.github.com/repos/GafferHQ/gaffer/issues/2251/events","html_url":"https://github.com/GafferHQ/gaffer/pull/2251","id":253504127,"number":2251,"title":"OSLShadingEngine :  use tbb parallel_for for evaluation.","user":{"login":"donboie","id":25311256,"avatar_url":"https://avatars1.githubusercontent.com/u/25311256?v=4","gravatar_id":"","url":"https://api.github.com/users/donboie","html_url":"https://github.com/donboie","followers_url":"https://api.github.com/users/donboie/followers","following_url":"https://api.github.com/users/donboie/following{/other_user}","gists_url":"https://api.github.com/users/donboie/gists{/gist_id}","starred_url":"https://api.github.com/users/donboie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/donboie/subscriptions","organizations_url":"https://api.github.com/users/donboie/orgs","repos_url":"https://api.github.com/users/donboie/repos","events_url":"https://api.github.com/users/donboie/events{/privacy}","received_events_url":"https://api.github.com/users/donboie/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":11,"created_at":"2017-08-29T01:40:20Z","updated_at":"2017-09-05T16:26:10Z","closed_at":null,"author_association":"COLLABORATOR","pull_request":{"url":"https://api.github.com/repos/GafferHQ/gaffer/pulls/2251","html_url":"https://github.com/GafferHQ/gaffer/pull/2251","diff_url":"https://github.com/GafferHQ/gaffer/pull/2251.diff","patch_url":"https://github.com/GafferHQ/gaffer/pull/2251.patch"},"body":"Created per thread data:\r\n* renderstate to store the write index\r\n* globals so renderstate & values can be updated per thread.\r\n\r\nAlso attempted to allocate output data arrays with as little contention as possible by using tbb concurrent_hash_map.\r\n\r\nA simple displaced 4096 x 4096 plane OSLObject compute time went from 3.4s to 1.35s which seems a little modest on my 16 HT machine.  Perhaps a pass with vtune in the future would explain the lack of performance.\r\n"},"comment":{"url":"https://api.github.com/repos/GafferHQ/gaffer/issues/comments/327229701","html_url":"https://github.com/GafferHQ/gaffer/pull/2251#issuecomment-327229701","issue_url":"https://api.github.com/repos/GafferHQ/gaffer/issues/2251","id":327229701,"user":{"login":"donboie","id":25311256,"avatar_url":"https://avatars1.githubusercontent.com/u/25311256?v=4","gravatar_id":"","url":"https://api.github.com/users/donboie","html_url":"https://github.com/donboie","followers_url":"https://api.github.com/users/donboie/followers","following_url":"https://api.github.com/users/donboie/following{/other_user}","gists_url":"https://api.github.com/users/donboie/gists{/gist_id}","starred_url":"https://api.github.com/users/donboie/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/donboie/subscriptions","organizations_url":"https://api.github.com/users/donboie/orgs","repos_url":"https://api.github.com/users/donboie/repos","events_url":"https://api.github.com/users/donboie/events{/privacy}","received_events_url":"https://api.github.com/users/donboie/received_events","type":"User","site_admin":false},"created_at":"2017-09-05T16:26:10Z","updated_at":"2017-09-05T16:26:10Z","author_association":"COLLABORATOR","body":"> Yeah, it does feel like things are getting a bit fiddly in there, but without really diving in I didn't have any great ideas for improvements. One thing I found odd was the \"return an iterator but also a bool\" thing - seemed an odd mishmash of idioms. I wish I could remember if there's a good reason it's using a sorted vector and not a boost::flat_map because it seems like that would make the whole thing a bit less verbose. I hadn't even spotted the hardcoded grain size to be honest. I'm still feeling my way on when to use auto/lambda really, but I think that big lambda is waay clearer than manually making a functor to carry all that stuff it needs...\r\n\r\nAgreed looks a little odd but I supose one is in insertion point and the other indicates if it's a match.  I only learnt about flat map while reading the some other parts of the Gaffer code and was wondering why it wasn't used here.\r\n\r\n> I think I'm inclined to merge unless you want to take another pass. It's all internal anyway so we can easily refactor later if we want.\r\n\r\nI think there are other areas where there are larger wins and this can be improved on in the future as you say.\r\n\r\n> Is that because of the ResamplePrimitiveVariables node? I thought we were using ShadingEngine::needsAttribute() to limit the resampling to only the stuff that was actually used, but it looks like that's maybe not the case?\r\n\r\nThat's how it's supposed to work but vtune tells me otherwise. I'll put together a few tests to see what's going on regarding the resample node.\r\n\r\n\r\n"}}
{"actor":{"display_login":"donboie"}}