GitHub PullRequestReviewCommentEvent at 2014-12-22 18:45:57
Repository: chapel
Actor: ronawho
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/chapel-lang/chapel/pulls/comments/22182508","id":22182508,"diff_hunk":"@@ -342,6 +340,17 @@ module LocaleModel {\n \n   //////////////////////////////////////////\n   //\n+  // utilities\n+  //\n+  inline","path":"modules/internal/localeModels/numa/LocaleModel.chpl","position":42,"original_position":42,"commit_id":"50588bdcef0c7c1ebd15aecd3245ae65f4f95dfd","original_commit_id":"50588bdcef0c7c1ebd15aecd3245ae65f4f95dfd","user":{"login":"ronawho","id":1588337,"avatar_url":"https://avatars.githubusercontent.com/u/1588337?v=3","gravatar_id":"","url":"https://api.github.com/users/ronawho","html_url":"https://github.com/ronawho","followers_url":"https://api.github.com/users/ronawho/followers","following_url":"https://api.github.com/users/ronawho/following{/other_user}","gists_url":"https://api.github.com/users/ronawho/gists{/gist_id}","starred_url":"https://api.github.com/users/ronawho/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ronawho/subscriptions","organizations_url":"https://api.github.com/users/ronawho/orgs","repos_url":"https://api.github.com/users/ronawho/repos","events_url":"https://api.github.com/users/ronawho/events{/privacy}","received_events_url":"https://api.github.com/users/ronawho/received_events","type":"User","site_admin":false},"body":"I agree that explicitly calling chpl_getSublocale() if you know there are no sublocales seems wrong. I was thinking it's annoying to have to guard that call with a check to see what the locale model is. \r\n\r\nIt's not bad in the current world, but I was imagining in the future if we have dozens of locale models, or if you're writing a numa aware iterator that you also want to work on non-numa locale models, it would be annoying to have to think about that and while it might not be as performant it would be nice to just have that call work. \r\n\r\nMy example is pretty contrived, and I don't have any concrete example of why I think it would be nice, so I think the compilerError is the way to go.","created_at":"2014-12-22T18:45:57Z","updated_at":"2014-12-22T18:45:57Z","html_url":"https://github.com/chapel-lang/chapel/pull/1056#discussion_r22182508","pull_request_url":"https://api.github.com/repos/chapel-lang/chapel/pulls/1056","_links":{"self":{"href":"https://api.github.com/repos/chapel-lang/chapel/pulls/comments/22182508"},"html":{"href":"https://github.com/chapel-lang/chapel/pull/1056#discussion_r22182508"},"pull_request":{"href":"https://api.github.com/repos/chapel-lang/chapel/pulls/1056"}}},"pull_request":{"url":"https://api.github.com/repos/chapel-lang/chapel/pulls/1056","id":26417075,"html_url":"https://github.com/chapel-lang/chapel/pull/1056","diff_url":"https://github.com/chapel-lang/chapel/pull/1056.diff","patch_url":"https://github.com/chapel-lang/chapel/pull/1056.patch","issue_url":"https://api.github.com/repos/chapel-lang/chapel/issues/1056","number":1056,"state":"open","locked":false,"title":"Simplify some locale model interactions with the runtime.","user":{"login":"gbtitus","id":7740880,"avatar_url":"https://avatars.githubusercontent.com/u/7740880?v=3","gravatar_id":"","url":"https://api.github.com/users/gbtitus","html_url":"https://github.com/gbtitus","followers_url":"https://api.github.com/users/gbtitus/followers","following_url":"https://api.github.com/users/gbtitus/following{/other_user}","gists_url":"https://api.github.com/users/gbtitus/gists{/gist_id}","starred_url":"https://api.github.com/users/gbtitus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gbtitus/subscriptions","organizations_url":"https://api.github.com/users/gbtitus/orgs","repos_url":"https://api.github.com/users/gbtitus/repos","events_url":"https://api.github.com/users/gbtitus/events{/privacy}","received_events_url":"https://api.github.com/users/gbtitus/received_events","type":"User","site_admin":false},"body":"These are minor simplifications based on what really gets used, and not\r\nused, for locale models that do (numa) and don't (flat) have sublocales.\r\nIn the runtime I've removed CHPL_LOCALE_MODEL_NUM_SUBLOCALES, which was\r\nused to #ifdef out runtime code having to do with sublocales when we\r\nwere using a locale model that didn't have them.  But it turns out much\r\nof that code isn't called in the first place except with a locale model\r\nthat DOES have sublocales, and for the rest the performance savings\r\n(which are tiny, probably unmeasurable) don't justify the impact to code\r\nreadability.\r\n\r\nElsewhere, I did some tidying up in the locale model code and some numa\r\nspecific tests, as preparation for future optimizations which will\r\nshort-circuit some places where we currently call the runtime for\r\nsublocale-related information.\r\n\r\nWhen we started down this development path we weren't sure what we would need and\r\nnot need in the various cases. This clean-up is taking advantage of greater knowledge\r\nnow, to remove what has turned out not to be needed.\r\n\r\nThe specific file changes are as follows.\r\n\r\nruntime/src/tasks/fifo/tasks-fifo.c\r\nruntime/src/tasks/massivethreads/tasks-massivethreads.c\r\n  Remove the dependence on CHPL_LOCALE_MODEL_HAS_SUBLOCALES and just\r\n  return 0 (zero) from chpl_task_getNumSublocales().\r\n\r\nthird-party/qthread/qthread-1.10/src/interfaces/chapel/tasks-qthreads.c\r\n  Remove the dependence on CHPL_LOCALE_MODEL_HAS_SUBLOCALES and just\r\n  return qthread_num_shepherds() from chpl_task_getNumSublocales().\r\n\r\nthird-party/qthread/qthread-1.10/src/interfaces/chapel/tasks-qthreads.h\r\n  Remove the dependences on CHPL_LOCALE_MODEL_HAS_SUBLOCALES.  Always\r\n  store a sublocale index in task private data, even when we don't need\r\n  it, such as when we're using the flat locale model.  The unmeasurably\r\n  small performance gain isn't worth the code messiness.  And in these\r\n  three functions, we didn't need CHPL_LOCALE_MODEL_HAS_SUBLOCALES\r\n  anyway, because they're only called when we're using the numa locale\r\n  model:\r\n  - Always return qthread_shep() for chpl_task_getSubloc().\r\n  - Always set the sublocale in chpl_task_setSubloc().\r\n  - Always return the sublocale in chpl_task_getRequestedSubloc().\r\n\r\nruntime/include/localeModels/flat/chpl-locale-model.h\r\n  Remove the definition of CHPL_LOCALE_MODEL_HAS_SUBLOCALES (now no\r\n  longer used) and a superfluous comment.\r\n\r\nmodules/internal/localeModels/flat/LocaleModel.chpl\r\nmodules/internal/localeModels/numa/LocaleModel.chpl\r\n  Remove the mention of CHPL_LOCALE_MODEL_NUM_SUBLOCALES, which no\r\n  longer exists.\r\n  Hoist the extern definitions for c_sublocid_none and c_sublocid_any,\r\n  instead of defining them multiple times, near where they're used.\r\n  For numa, add chpl_getSubloc() to wrap chpl_task_getSubloc().  So far\r\n  this is just a simple wrapper, but in the future I expect it to avoid\r\n  calling the tasking layer when we know what the result will be.  For\r\n  flat, there are no calls to chpl_task_getSubloc(), so no need for this\r\n  wrapper.\r\n\r\nmodules/internal/ChapelRange.chpl\r\nmodules/internal/DefaultRectangular.chpl\r\n  Call chpl_getSubloc() instead of chpl_task_getSubloc().  Note that\r\n  these calls only occur under a param-based if-stmt that checks whether\r\n  the current locale has sublocales.\r\n\r\ntest/localeModels/numa/basics/checkSubloc.chpl\r\ntest/localeModels/numa/dataParallel/forall.chpl\r\n  Call chpl_getSUbloc() instead of chpl_task_getSubloc().  Note that\r\n  these tests are only run for the numa locale model.","created_at":"2014-12-20T14:58:23Z","updated_at":"2014-12-22T18:45:57Z","closed_at":null,"merged_at":null,"merge_commit_sha":"d279451de31d858e97c6395d6e1bbeb94c8eaabd","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/chapel-lang/chapel/pulls/1056/commits","review_comments_url":"https://api.github.com/repos/chapel-lang/chapel/pulls/1056/comments","review_comment_url":"https://api.github.com/repos/chapel-lang/chapel/pulls/comments/{number}","comments_url":"https://api.github.com/repos/chapel-lang/chapel/issues/1056/comments","statuses_url":"https://api.github.com/repos/chapel-lang/chapel/statuses/50588bdcef0c7c1ebd15aecd3245ae65f4f95dfd","head":{"label":"gbtitus:subloc-interface-in-modules","ref":"subloc-interface-in-modules","sha":"50588bdcef0c7c1ebd15aecd3245ae65f4f95dfd","user":{"login":"gbtitus","id":7740880,"avatar_url":"https://avatars.githubusercontent.com/u/7740880?v=3","gravatar_id":"","url":"https://api.github.com/users/gbtitus","html_url":"https://github.com/gbtitus","followers_url":"https://api.github.com/users/gbtitus/followers","following_url":"https://api.github.com/users/gbtitus/following{/other_user}","gists_url":"https://api.github.com/users/gbtitus/gists{/gist_id}","starred_url":"https://api.github.com/users/gbtitus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gbtitus/subscriptions","organizations_url":"https://api.github.com/users/gbtitus/orgs","repos_url":"https://api.github.com/users/gbtitus/repos","events_url":"https://api.github.com/users/gbtitus/events{/privacy}","received_events_url":"https://api.github.com/users/gbtitus/received_events","type":"User","site_admin":false},"repo":{"id":21912579,"name":"chapel","full_name":"gbtitus/chapel","owner":{"login":"gbtitus","id":7740880,"avatar_url":"https://avatars.githubusercontent.com/u/7740880?v=3","gravatar_id":"","url":"https://api.github.com/users/gbtitus","html_url":"https://github.com/gbtitus","followers_url":"https://api.github.com/users/gbtitus/followers","following_url":"https://api.github.com/users/gbtitus/following{/other_user}","gists_url":"https://api.github.com/users/gbtitus/gists{/gist_id}","starred_url":"https://api.github.com/users/gbtitus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gbtitus/subscriptions","organizations_url":"https://api.github.com/users/gbtitus/orgs","repos_url":"https://api.github.com/users/gbtitus/repos","events_url":"https://api.github.com/users/gbtitus/events{/privacy}","received_events_url":"https://api.github.com/users/gbtitus/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/gbtitus/chapel","description":"a Productive Parallel Programming Language","fork":true,"url":"https://api.github.com/repos/gbtitus/chapel","forks_url":"https://api.github.com/repos/gbtitus/chapel/forks","keys_url":"https://api.github.com/repos/gbtitus/chapel/keys{/key_id}","collaborators_url":"https://api.github.com/repos/gbtitus/chapel/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/gbtitus/chapel/teams","hooks_url":"https://api.github.com/repos/gbtitus/chapel/hooks","issue_events_url":"https://api.github.com/repos/gbtitus/chapel/issues/events{/number}","events_url":"https://api.github.com/repos/gbtitus/chapel/events","assignees_url":"https://api.github.com/repos/gbtitus/chapel/assignees{/user}","branches_url":"https://api.github.com/repos/gbtitus/chapel/branches{/branch}","tags_url":"https://api.github.com/repos/gbtitus/chapel/tags","blobs_url":"https://api.github.com/repos/gbtitus/chapel/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/gbtitus/chapel/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/gbtitus/chapel/git/refs{/sha}","trees_url":"https://api.github.com/repos/gbtitus/chapel/git/trees{/sha}","statuses_url":"https://api.github.com/repos/gbtitus/chapel/statuses/{sha}","languages_url":"https://api.github.com/repos/gbtitus/chapel/languages","stargazers_url":"https://api.github.com/repos/gbtitus/chapel/stargazers","contributors_url":"https://api.github.com/repos/gbtitus/chapel/contributors","subscribers_url":"https://api.github.com/repos/gbtitus/chapel/subscribers","subscription_url":"https://api.github.com/repos/gbtitus/chapel/subscription","commits_url":"https://api.github.com/repos/gbtitus/chapel/commits{/sha}","git_commits_url":"https://api.github.com/repos/gbtitus/chapel/git/commits{/sha}","comments_url":"https://api.github.com/repos/gbtitus/chapel/comments{/number}","issue_comment_url":"https://api.github.com/repos/gbtitus/chapel/issues/comments/{number}","contents_url":"https://api.github.com/repos/gbtitus/chapel/contents/{+path}","compare_url":"https://api.github.com/repos/gbtitus/chapel/compare/{base}...{head}","merges_url":"https://api.github.com/repos/gbtitus/chapel/merges","archive_url":"https://api.github.com/repos/gbtitus/chapel/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/gbtitus/chapel/downloads","issues_url":"https://api.github.com/repos/gbtitus/chapel/issues{/number}","pulls_url":"https://api.github.com/repos/gbtitus/chapel/pulls{/number}","milestones_url":"https://api.github.com/repos/gbtitus/chapel/milestones{/number}","notifications_url":"https://api.github.com/repos/gbtitus/chapel/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/gbtitus/chapel/labels{/name}","releases_url":"https://api.github.com/repos/gbtitus/chapel/releases{/id}","created_at":"2014-07-16T18:39:09Z","updated_at":"2014-12-19T21:17:25Z","pushed_at":"2014-12-20T00:58:55Z","git_url":"git://github.com/gbtitus/chapel.git","ssh_url":"git@github.com:gbtitus/chapel.git","clone_url":"https://github.com/gbtitus/chapel.git","svn_url":"https://github.com/gbtitus/chapel","homepage":"http://chapel.cray.com/","size":256275,"stargazers_count":2,"watchers_count":2,"language":"Chapel","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":2,"default_branch":"master"}},"base":{"label":"chapel-lang:master","ref":"master","sha":"e8630cb256ad82f327d720dd104dd5fcd0e588de","user":{"login":"chapel-lang","id":7597261,"avatar_url":"https://avatars.githubusercontent.com/u/7597261?v=3","gravatar_id":"","url":"https://api.github.com/users/chapel-lang","html_url":"https://github.com/chapel-lang","followers_url":"https://api.github.com/users/chapel-lang/followers","following_url":"https://api.github.com/users/chapel-lang/following{/other_user}","gists_url":"https://api.github.com/users/chapel-lang/gists{/gist_id}","starred_url":"https://api.github.com/users/chapel-lang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chapel-lang/subscriptions","organizations_url":"https://api.github.com/users/chapel-lang/orgs","repos_url":"https://api.github.com/users/chapel-lang/repos","events_url":"https://api.github.com/users/chapel-lang/events{/privacy}","received_events_url":"https://api.github.com/users/chapel-lang/received_events","type":"Organization","site_admin":false},"repo":{"id":21663285,"name":"chapel","full_name":"chapel-lang/chapel","owner":{"login":"chapel-lang","id":7597261,"avatar_url":"https://avatars.githubusercontent.com/u/7597261?v=3","gravatar_id":"","url":"https://api.github.com/users/chapel-lang","html_url":"https://github.com/chapel-lang","followers_url":"https://api.github.com/users/chapel-lang/followers","following_url":"https://api.github.com/users/chapel-lang/following{/other_user}","gists_url":"https://api.github.com/users/chapel-lang/gists{/gist_id}","starred_url":"https://api.github.com/users/chapel-lang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/chapel-lang/subscriptions","organizations_url":"https://api.github.com/users/chapel-lang/orgs","repos_url":"https://api.github.com/users/chapel-lang/repos","events_url":"https://api.github.com/users/chapel-lang/events{/privacy}","received_events_url":"https://api.github.com/users/chapel-lang/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/chapel-lang/chapel","description":"a Productive Parallel Programming Language","fork":false,"url":"https://api.github.com/repos/chapel-lang/chapel","forks_url":"https://api.github.com/repos/chapel-lang/chapel/forks","keys_url":"https://api.github.com/repos/chapel-lang/chapel/keys{/key_id}","collaborators_url":"https://api.github.com/repos/chapel-lang/chapel/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/chapel-lang/chapel/teams","hooks_url":"https://api.github.com/repos/chapel-lang/chapel/hooks","issue_events_url":"https://api.github.com/repos/chapel-lang/chapel/issues/events{/number}","events_url":"https://api.github.com/repos/chapel-lang/chapel/events","assignees_url":"https://api.github.com/repos/chapel-lang/chapel/assignees{/user}","branches_url":"https://api.github.com/repos/chapel-lang/chapel/branches{/branch}","tags_url":"https://api.github.com/repos/chapel-lang/chapel/tags","blobs_url":"https://api.github.com/repos/chapel-lang/chapel/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/chapel-lang/chapel/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/chapel-lang/chapel/git/refs{/sha}","trees_url":"https://api.github.com/repos/chapel-lang/chapel/git/trees{/sha}","statuses_url":"https://api.github.com/repos/chapel-lang/chapel/statuses/{sha}","languages_url":"https://api.github.com/repos/chapel-lang/chapel/languages","stargazers_url":"https://api.github.com/repos/chapel-lang/chapel/stargazers","contributors_url":"https://api.github.com/repos/chapel-lang/chapel/contributors","subscribers_url":"https://api.github.com/repos/chapel-lang/chapel/subscribers","subscription_url":"https://api.github.com/repos/chapel-lang/chapel/subscription","commits_url":"https://api.github.com/repos/chapel-lang/chapel/commits{/sha}","git_commits_url":"https://api.github.com/repos/chapel-lang/chapel/git/commits{/sha}","comments_url":"https://api.github.com/repos/chapel-lang/chapel/comments{/number}","issue_comment_url":"https://api.github.com/repos/chapel-lang/chapel/issues/comments/{number}","contents_url":"https://api.github.com/repos/chapel-lang/chapel/contents/{+path}","compare_url":"https://api.github.com/repos/chapel-lang/chapel/compare/{base}...{head}","merges_url":"https://api.github.com/repos/chapel-lang/chapel/merges","archive_url":"https://api.github.com/repos/chapel-lang/chapel/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/chapel-lang/chapel/downloads","issues_url":"https://api.github.com/repos/chapel-lang/chapel/issues{/number}","pulls_url":"https://api.github.com/repos/chapel-lang/chapel/pulls{/number}","milestones_url":"https://api.github.com/repos/chapel-lang/chapel/milestones{/number}","notifications_url":"https://api.github.com/repos/chapel-lang/chapel/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/chapel-lang/chapel/labels{/name}","releases_url":"https://api.github.com/repos/chapel-lang/chapel/releases{/id}","created_at":"2014-07-09T18:15:54Z","updated_at":"2014-12-22T18:11:00Z","pushed_at":"2014-12-22T18:11:00Z","git_url":"git://github.com/chapel-lang/chapel.git","ssh_url":"git@github.com:chapel-lang/chapel.git","clone_url":"https://github.com/chapel-lang/chapel.git","svn_url":"https://github.com/chapel-lang/chapel","homepage":"http://chapel.cray.com/","size":325841,"stargazers_count":55,"watchers_count":55,"language":"Chapel","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":32,"mirror_url":null,"open_issues_count":11,"forks":32,"open_issues":11,"watchers":55,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/chapel-lang/chapel/pulls/1056"},"html":{"href":"https://github.com/chapel-lang/chapel/pull/1056"},"issue":{"href":"https://api.github.com/repos/chapel-lang/chapel/issues/1056"},"comments":{"href":"https://api.github.com/repos/chapel-lang/chapel/issues/1056/comments"},"review_comments":{"href":"https://api.github.com/repos/chapel-lang/chapel/pulls/1056/comments"},"review_comment":{"href":"https://api.github.com/repos/chapel-lang/chapel/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/chapel-lang/chapel/pulls/1056/commits"},"statuses":{"href":"https://api.github.com/repos/chapel-lang/chapel/statuses/50588bdcef0c7c1ebd15aecd3245ae65f4f95dfd"}}}}