{"action":"created","comment":{"url":"https://api.github.com/repos/pantsbuild/pants/pulls/comments/99442642","pull_request_review_id":20125109,"id":99442642,"diff_hunk":"@@ -299,20 +300,49 @@ class RunnableComplete(datatype('RunnableComplete', ['value', 'is_throw'])):\n   \"\"\"Corresponds to the native object of the same name.\"\"\"\n \n \n-class ExternContext(object):\n-  \"\"\"A wrapper around python objects used in static extern functions in this module.\n+class ObjectIdMap(object):\n+  \"\"\"In the native context, assign and memoize python objects an unique unsigned-integer Id.\n+\n+  Underlying, the id is uniquely derived from object's digest instead of using its hash code\n+  because the content may change and object's hash function could be overridden. In order not\n+  to return the stale object we trade performance for correctness.\n \n-  In the native context, python objects are identified by an unsigned-integer Id which is\n-  assigned and memoized here. Note that this is independent-from and much-lighter-than\n-  the Digest computed when an object is stored via storage.py (which is generally only necessary\n-  for multi-processing or cache lookups).\n+  In the future, we could improve performance by only computing digests for mutable objects.\n+  For this reason referring an implementation-independent id instead of the digest in the native\n+  context is more flexible.\n   \"\"\"\n \n   def __init__(self):\n+    # objects indexed by their keys, i.e, content digests\n+    self._objects = Storage.create(in_memory=True)\n+    # Memoized object Ids.\n+    self._id_to_key = dict()\n+    self._key_to_id = dict()\n+    self._next_id = 0\n+\n+  def put(self, obj):\n+    key = self._objects.put(obj)\n+    new_id = self._next_id\n+    _id = self._key_to_id.setdefault(key, new_id)\n+    if _id is not new_id:\n+      # Object already existed.\n+      return _id\n+\n+    # Object is new/unique.\n+    self._id_to_key[_id] = key\n+    self._next_id += 1\n+    return _id\n+\n+  def get(self, id_):\n+    return self._objects.get(self._id_to_key[id_])","path":"src/python/pants/engine/subsystem/native.py","position":52,"original_position":52,"commit_id":"e8ec041fa10b71a8d522aecd6bfba945582b5dcf","original_commit_id":"e8ec041fa10b71a8d522aecd6bfba945582b5dcf","user":{"login":"stuhood","id":46740,"avatar_url":"https://avatars.githubusercontent.com/u/46740?v=3","gravatar_id":"","url":"https://api.github.com/users/stuhood","html_url":"https://github.com/stuhood","followers_url":"https://api.github.com/users/stuhood/followers","following_url":"https://api.github.com/users/stuhood/following{/other_user}","gists_url":"https://api.github.com/users/stuhood/gists{/gist_id}","starred_url":"https://api.github.com/users/stuhood/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stuhood/subscriptions","organizations_url":"https://api.github.com/users/stuhood/orgs","repos_url":"https://api.github.com/users/stuhood/repos","events_url":"https://api.github.com/users/stuhood/events{/privacy}","received_events_url":"https://api.github.com/users/stuhood/received_events","type":"User","site_admin":false},"body":"+1","created_at":"2017-02-03T23:22:40Z","updated_at":"2017-02-03T23:22:40Z","html_url":"https://github.com/pantsbuild/pants/pull/4233#discussion_r99442642","pull_request_url":"https://api.github.com/repos/pantsbuild/pants/pulls/4233","_links":{"self":{"href":"https://api.github.com/repos/pantsbuild/pants/pulls/comments/99442642"},"html":{"href":"https://github.com/pantsbuild/pants/pull/4233#discussion_r99442642"},"pull_request":{"href":"https://api.github.com/repos/pantsbuild/pants/pulls/4233"}}},"pull_request":{"url":"https://api.github.com/repos/pantsbuild/pants/pulls/4233","id":104466493,"html_url":"https://github.com/pantsbuild/pants/pull/4233","diff_url":"https://github.com/pantsbuild/pants/pull/4233.diff","patch_url":"https://github.com/pantsbuild/pants/pull/4233.patch","issue_url":"https://api.github.com/repos/pantsbuild/pants/issues/4233","number":4233,"state":"open","locked":false,"title":"Derive object id used in the native context from object's content","user":{"login":"peiyuwang","id":2474428,"avatar_url":"https://avatars.githubusercontent.com/u/2474428?v=3","gravatar_id":"","url":"https://api.github.com/users/peiyuwang","html_url":"https://github.com/peiyuwang","followers_url":"https://api.github.com/users/peiyuwang/followers","following_url":"https://api.github.com/users/peiyuwang/following{/other_user}","gists_url":"https://api.github.com/users/peiyuwang/gists{/gist_id}","starred_url":"https://api.github.com/users/peiyuwang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/peiyuwang/subscriptions","organizations_url":"https://api.github.com/users/peiyuwang/orgs","repos_url":"https://api.github.com/users/peiyuwang/repos","events_url":"https://api.github.com/users/peiyuwang/events{/privacy}","received_events_url":"https://api.github.com/users/peiyuwang/received_events","type":"User","site_admin":false},"body":"### Problem\r\n\r\nIn the native context, object uses an integer identifier which is derived by checking hash existence. In the case of user overriding hash function, this can be buggy because the object change may not be reflected. \r\n\r\n#4199 illustrates the problem in more detail. Note `Struct` as of day is not used as a subject, neither does any of the subject objects override their hash function, but this is still a potential risk.\r\n\r\n### Solution\r\n\r\nSwitch to use the pickled content digest, still keep the integer as the external facing identifier to live in the native context, for the purpose of hiding the implementation detail. As in this review, it's one to one: digest to integer id.\r\n\r\n### Result\r\n\r\nPerformance impact estimate: negative `~40%`","created_at":"2017-02-03T02:44:00Z","updated_at":"2017-02-03T23:22:40Z","closed_at":null,"merged_at":null,"merge_commit_sha":"f765cebb5a76e92de721acb36d69e8967973106c","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/pantsbuild/pants/pulls/4233/commits","review_comments_url":"https://api.github.com/repos/pantsbuild/pants/pulls/4233/comments","review_comment_url":"https://api.github.com/repos/pantsbuild/pants/pulls/comments{/number}","comments_url":"https://api.github.com/repos/pantsbuild/pants/issues/4233/comments","statuses_url":"https://api.github.com/repos/pantsbuild/pants/statuses/e8ec041fa10b71a8d522aecd6bfba945582b5dcf","head":{"label":"peiyuwang:peiyu/use-pickling-for-key","ref":"peiyu/use-pickling-for-key","sha":"e8ec041fa10b71a8d522aecd6bfba945582b5dcf","user":{"login":"peiyuwang","id":2474428,"avatar_url":"https://avatars.githubusercontent.com/u/2474428?v=3","gravatar_id":"","url":"https://api.github.com/users/peiyuwang","html_url":"https://github.com/peiyuwang","followers_url":"https://api.github.com/users/peiyuwang/followers","following_url":"https://api.github.com/users/peiyuwang/following{/other_user}","gists_url":"https://api.github.com/users/peiyuwang/gists{/gist_id}","starred_url":"https://api.github.com/users/peiyuwang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/peiyuwang/subscriptions","organizations_url":"https://api.github.com/users/peiyuwang/orgs","repos_url":"https://api.github.com/users/peiyuwang/repos","events_url":"https://api.github.com/users/peiyuwang/events{/privacy}","received_events_url":"https://api.github.com/users/peiyuwang/received_events","type":"User","site_admin":false},"repo":{"id":27841884,"name":"pants","full_name":"peiyuwang/pants","owner":{"login":"peiyuwang","id":2474428,"avatar_url":"https://avatars.githubusercontent.com/u/2474428?v=3","gravatar_id":"","url":"https://api.github.com/users/peiyuwang","html_url":"https://github.com/peiyuwang","followers_url":"https://api.github.com/users/peiyuwang/followers","following_url":"https://api.github.com/users/peiyuwang/following{/other_user}","gists_url":"https://api.github.com/users/peiyuwang/gists{/gist_id}","starred_url":"https://api.github.com/users/peiyuwang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/peiyuwang/subscriptions","organizations_url":"https://api.github.com/users/peiyuwang/orgs","repos_url":"https://api.github.com/users/peiyuwang/repos","events_url":"https://api.github.com/users/peiyuwang/events{/privacy}","received_events_url":"https://api.github.com/users/peiyuwang/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/peiyuwang/pants","description":"Pants Build System","fork":true,"url":"https://api.github.com/repos/peiyuwang/pants","forks_url":"https://api.github.com/repos/peiyuwang/pants/forks","keys_url":"https://api.github.com/repos/peiyuwang/pants/keys{/key_id}","collaborators_url":"https://api.github.com/repos/peiyuwang/pants/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/peiyuwang/pants/teams","hooks_url":"https://api.github.com/repos/peiyuwang/pants/hooks","issue_events_url":"https://api.github.com/repos/peiyuwang/pants/issues/events{/number}","events_url":"https://api.github.com/repos/peiyuwang/pants/events","assignees_url":"https://api.github.com/repos/peiyuwang/pants/assignees{/user}","branches_url":"https://api.github.com/repos/peiyuwang/pants/branches{/branch}","tags_url":"https://api.github.com/repos/peiyuwang/pants/tags","blobs_url":"https://api.github.com/repos/peiyuwang/pants/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/peiyuwang/pants/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/peiyuwang/pants/git/refs{/sha}","trees_url":"https://api.github.com/repos/peiyuwang/pants/git/trees{/sha}","statuses_url":"https://api.github.com/repos/peiyuwang/pants/statuses/{sha}","languages_url":"https://api.github.com/repos/peiyuwang/pants/languages","stargazers_url":"https://api.github.com/repos/peiyuwang/pants/stargazers","contributors_url":"https://api.github.com/repos/peiyuwang/pants/contributors","subscribers_url":"https://api.github.com/repos/peiyuwang/pants/subscribers","subscription_url":"https://api.github.com/repos/peiyuwang/pants/subscription","commits_url":"https://api.github.com/repos/peiyuwang/pants/commits{/sha}","git_commits_url":"https://api.github.com/repos/peiyuwang/pants/git/commits{/sha}","comments_url":"https://api.github.com/repos/peiyuwang/pants/comments{/number}","issue_comment_url":"https://api.github.com/repos/peiyuwang/pants/issues/comments{/number}","contents_url":"https://api.github.com/repos/peiyuwang/pants/contents/{+path}","compare_url":"https://api.github.com/repos/peiyuwang/pants/compare/{base}...{head}","merges_url":"https://api.github.com/repos/peiyuwang/pants/merges","archive_url":"https://api.github.com/repos/peiyuwang/pants/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/peiyuwang/pants/downloads","issues_url":"https://api.github.com/repos/peiyuwang/pants/issues{/number}","pulls_url":"https://api.github.com/repos/peiyuwang/pants/pulls{/number}","milestones_url":"https://api.github.com/repos/peiyuwang/pants/milestones{/number}","notifications_url":"https://api.github.com/repos/peiyuwang/pants/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/peiyuwang/pants/labels{/name}","releases_url":"https://api.github.com/repos/peiyuwang/pants/releases{/id}","deployments_url":"https://api.github.com/repos/peiyuwang/pants/deployments","created_at":"2014-12-10T22:02:15Z","updated_at":"2016-04-25T06:47:59Z","pushed_at":"2017-02-03T00:39:47Z","git_url":"git://github.com/peiyuwang/pants.git","ssh_url":"git@github.com:peiyuwang/pants.git","clone_url":"https://github.com/peiyuwang/pants.git","svn_url":"https://github.com/peiyuwang/pants","homepage":"http://pantsbuild.github.io/","size":78168,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1,"mirror_url":null,"open_issues_count":0,"forks":1,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"pantsbuild:master","ref":"master","sha":"3641dd30c1dd9c5468175b6ce25b65424e4c2704","user":{"login":"pantsbuild","id":3065172,"avatar_url":"https://avatars.githubusercontent.com/u/3065172?v=3","gravatar_id":"","url":"https://api.github.com/users/pantsbuild","html_url":"https://github.com/pantsbuild","followers_url":"https://api.github.com/users/pantsbuild/followers","following_url":"https://api.github.com/users/pantsbuild/following{/other_user}","gists_url":"https://api.github.com/users/pantsbuild/gists{/gist_id}","starred_url":"https://api.github.com/users/pantsbuild/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pantsbuild/subscriptions","organizations_url":"https://api.github.com/users/pantsbuild/orgs","repos_url":"https://api.github.com/users/pantsbuild/repos","events_url":"https://api.github.com/users/pantsbuild/events{/privacy}","received_events_url":"https://api.github.com/users/pantsbuild/received_events","type":"Organization","site_admin":false},"repo":{"id":7209075,"name":"pants","full_name":"pantsbuild/pants","owner":{"login":"pantsbuild","id":3065172,"avatar_url":"https://avatars.githubusercontent.com/u/3065172?v=3","gravatar_id":"","url":"https://api.github.com/users/pantsbuild","html_url":"https://github.com/pantsbuild","followers_url":"https://api.github.com/users/pantsbuild/followers","following_url":"https://api.github.com/users/pantsbuild/following{/other_user}","gists_url":"https://api.github.com/users/pantsbuild/gists{/gist_id}","starred_url":"https://api.github.com/users/pantsbuild/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pantsbuild/subscriptions","organizations_url":"https://api.github.com/users/pantsbuild/orgs","repos_url":"https://api.github.com/users/pantsbuild/repos","events_url":"https://api.github.com/users/pantsbuild/events{/privacy}","received_events_url":"https://api.github.com/users/pantsbuild/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/pantsbuild/pants","description":"Pants Build System","fork":false,"url":"https://api.github.com/repos/pantsbuild/pants","forks_url":"https://api.github.com/repos/pantsbuild/pants/forks","keys_url":"https://api.github.com/repos/pantsbuild/pants/keys{/key_id}","collaborators_url":"https://api.github.com/repos/pantsbuild/pants/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/pantsbuild/pants/teams","hooks_url":"https://api.github.com/repos/pantsbuild/pants/hooks","issue_events_url":"https://api.github.com/repos/pantsbuild/pants/issues/events{/number}","events_url":"https://api.github.com/repos/pantsbuild/pants/events","assignees_url":"https://api.github.com/repos/pantsbuild/pants/assignees{/user}","branches_url":"https://api.github.com/repos/pantsbuild/pants/branches{/branch}","tags_url":"https://api.github.com/repos/pantsbuild/pants/tags","blobs_url":"https://api.github.com/repos/pantsbuild/pants/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/pantsbuild/pants/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/pantsbuild/pants/git/refs{/sha}","trees_url":"https://api.github.com/repos/pantsbuild/pants/git/trees{/sha}","statuses_url":"https://api.github.com/repos/pantsbuild/pants/statuses/{sha}","languages_url":"https://api.github.com/repos/pantsbuild/pants/languages","stargazers_url":"https://api.github.com/repos/pantsbuild/pants/stargazers","contributors_url":"https://api.github.com/repos/pantsbuild/pants/contributors","subscribers_url":"https://api.github.com/repos/pantsbuild/pants/subscribers","subscription_url":"https://api.github.com/repos/pantsbuild/pants/subscription","commits_url":"https://api.github.com/repos/pantsbuild/pants/commits{/sha}","git_commits_url":"https://api.github.com/repos/pantsbuild/pants/git/commits{/sha}","comments_url":"https://api.github.com/repos/pantsbuild/pants/comments{/number}","issue_comment_url":"https://api.github.com/repos/pantsbuild/pants/issues/comments{/number}","contents_url":"https://api.github.com/repos/pantsbuild/pants/contents/{+path}","compare_url":"https://api.github.com/repos/pantsbuild/pants/compare/{base}...{head}","merges_url":"https://api.github.com/repos/pantsbuild/pants/merges","archive_url":"https://api.github.com/repos/pantsbuild/pants/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/pantsbuild/pants/downloads","issues_url":"https://api.github.com/repos/pantsbuild/pants/issues{/number}","pulls_url":"https://api.github.com/repos/pantsbuild/pants/pulls{/number}","milestones_url":"https://api.github.com/repos/pantsbuild/pants/milestones{/number}","notifications_url":"https://api.github.com/repos/pantsbuild/pants/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/pantsbuild/pants/labels{/name}","releases_url":"https://api.github.com/repos/pantsbuild/pants/releases{/id}","deployments_url":"https://api.github.com/repos/pantsbuild/pants/deployments","created_at":"2012-12-17T17:39:04Z","updated_at":"2017-01-31T05:38:02Z","pushed_at":"2017-02-03T23:00:14Z","git_url":"git://github.com/pantsbuild/pants.git","ssh_url":"git@github.com:pantsbuild/pants.git","clone_url":"https://github.com/pantsbuild/pants.git","svn_url":"https://github.com/pantsbuild/pants","homepage":"http://pantsbuild.github.io/","size":78199,"stargazers_count":723,"watchers_count":723,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":212,"mirror_url":null,"open_issues_count":453,"forks":212,"open_issues":453,"watchers":723,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/pantsbuild/pants/pulls/4233"},"html":{"href":"https://github.com/pantsbuild/pants/pull/4233"},"issue":{"href":"https://api.github.com/repos/pantsbuild/pants/issues/4233"},"comments":{"href":"https://api.github.com/repos/pantsbuild/pants/issues/4233/comments"},"review_comments":{"href":"https://api.github.com/repos/pantsbuild/pants/pulls/4233/comments"},"review_comment":{"href":"https://api.github.com/repos/pantsbuild/pants/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/pantsbuild/pants/pulls/4233/commits"},"statuses":{"href":"https://api.github.com/repos/pantsbuild/pants/statuses/e8ec041fa10b71a8d522aecd6bfba945582b5dcf"}}}}
{"actor":{"display_login":"stuhood"}}