{"action":"created","comment":{"url":"https://api.github.com/repos/realm/realm-core/pulls/comments/103071866","pull_request_review_id":23851573,"id":103071866,"diff_hunk":"@@ -197,38 +197,39 @@ size_t IndexArray::from_list<index_FindAll_nocopy>(StringData value, IntegerColu\n     return size_t(FindRes_column);\n }\n \n-template <IndexMethod method, class T>\n+template <IndexMethod method>\n size_t IndexArray::index_string(StringData value, IntegerColumn& result, InternalFindResult& result_ref,\n                                 ColumnBase* column) const\n {\n     // Return`realm::not_found`, or an index to the (any) match\n-    bool first(method == index_FindFirst);\n+    constexpr bool first(method == index_FindFirst);\n     // Return 0, or the number of items that match the specified `value`\n-    bool get_count(method == index_Count);\n+    constexpr bool get_count(method == index_Count);\n     // Place all row indexes containing `value` into `result`\n     // Returns one of FindRes_not_found[==0] if no matches found\n     // Returns FindRes_single, if one match found: the result row literal is\n     // both placed in `result_ref.payload` and added to `column`\n     // Returns FindRes_column, if more than one match found: the matching row\n     // literals are copied into `column`\n-    bool all(method == index_FindAll);\n+    constexpr bool all(method == index_FindAll);\n     // Same as `index_FindAll` but does not copy matching rows into `column`\n     // returns FindRes_not_found if there are no matches\n     // returns FindRes_single and the row index (literal) in result_ref.payload\n     // or returns FindRes_column and the reference to a column of duplicates in\n     // result_ref.result with the results in the bounds start_ndx, and end_ndx\n-    bool allnocopy(method == index_FindAll_nocopy);\n+    constexpr bool allnocopy(method == index_FindAll_nocopy);\n+\n+    constexpr size_t local_not_found = (allnocopy || all) ? size_t(FindRes_not_found) : first ? not_found : 0;","path":"src/realm/index_string.cpp","position":114,"original_position":114,"commit_id":"bee3ccb4060c1ddf3e039d50b7fd2f2a5fd10e66","original_commit_id":"bee3ccb4060c1ddf3e039d50b7fd2f2a5fd10e66","user":{"login":"ironage","id":2826060,"avatar_url":"https://avatars.githubusercontent.com/u/2826060?v=3","gravatar_id":"","url":"https://api.github.com/users/ironage","html_url":"https://github.com/ironage","followers_url":"https://api.github.com/users/ironage/followers","following_url":"https://api.github.com/users/ironage/following{/other_user}","gists_url":"https://api.github.com/users/ironage/gists{/gist_id}","starred_url":"https://api.github.com/users/ironage/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ironage/subscriptions","organizations_url":"https://api.github.com/users/ironage/orgs","repos_url":"https://api.github.com/users/ironage/repos","events_url":"https://api.github.com/users/ironage/events{/privacy}","received_events_url":"https://api.github.com/users/ironage/received_events","type":"User","site_admin":false},"body":"@danielpovlsen The benchmark results of the find and sort methods have been unusually high on new PR's. At first I thought it could have been a problem with the performance infrastructure. But I think the issue can be traced back to this line in this PR. \r\n\r\nI think the problem is that we are now computing the `local_not_found` in all cases. Most often, a result will be found and therefore we can skip this check as we did before, only calculating the return value at the point where it is needed. For the benchmarks that make many `find_first` calls, the difference is 3 unnecessary `if` conditionals per find result which apparently adds up to a lot of computation time in the benchmarks.\r\n\r\nAdditionally, there was a small optimisation described in the (now removed) comment for `FindRes_not_found` being equal to zero. This lets us skip a check ( the `|| all` part) and return `0` at the end for the case of both `index_Count` and `index_FindAll` because `0` will be cast to `FindRes_not_found` implicitly. Its a small optimisation but since it is on a critical path, it counts.\r\n\r\nIf this analysis makes any sense to you, I recommend removing the above line (going back to the computation on the return lines) and seeing if our benchmarks go back to normal. Unfortunately the sacrifice for performance is often readability :( ","created_at":"2017-02-25T05:33:27Z","updated_at":"2017-02-25T05:33:27Z","html_url":"https://github.com/realm/realm-core/pull/2458#discussion_r103071866","pull_request_url":"https://api.github.com/repos/realm/realm-core/pulls/2458","_links":{"self":{"href":"https://api.github.com/repos/realm/realm-core/pulls/comments/103071866"},"html":{"href":"https://github.com/realm/realm-core/pull/2458#discussion_r103071866"},"pull_request":{"href":"https://api.github.com/repos/realm/realm-core/pulls/2458"}}},"pull_request":{"url":"https://api.github.com/repos/realm/realm-core/pulls/2458","id":106693389,"html_url":"https://github.com/realm/realm-core/pull/2458","diff_url":"https://github.com/realm/realm-core/pull/2458.diff","patch_url":"https://github.com/realm/realm-core/pull/2458.patch","issue_url":"https://api.github.com/repos/realm/realm-core/issues/2458","number":2458,"state":"closed","locked":false,"title":"String index improvements","user":{"login":"danielpovlsen","id":77737,"avatar_url":"https://avatars.githubusercontent.com/u/77737?v=3","gravatar_id":"","url":"https://api.github.com/users/danielpovlsen","html_url":"https://github.com/danielpovlsen","followers_url":"https://api.github.com/users/danielpovlsen/followers","following_url":"https://api.github.com/users/danielpovlsen/following{/other_user}","gists_url":"https://api.github.com/users/danielpovlsen/gists{/gist_id}","starred_url":"https://api.github.com/users/danielpovlsen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danielpovlsen/subscriptions","organizations_url":"https://api.github.com/users/danielpovlsen/orgs","repos_url":"https://api.github.com/users/danielpovlsen/repos","events_url":"https://api.github.com/users/danielpovlsen/events{/privacy}","received_events_url":"https://api.github.com/users/danielpovlsen/received_events","type":"User","site_admin":false},"body":"Extracted from another branch to keep PRs clean.\r\n\r\nProbably easier to review commit by commit.\r\n\r\nThe most significant change here is probably the simplification of selecting the next string offset for the key generation (4c7fc5c). When the end of the search string is reached, which happens immediately for strings of less than 4 bytes, the old code set the next key offset to be the string length + 1.:\r\n```C++\r\nstringoffset += value.size() - stringoffset + 1; // stringoffset = value.size() + 1;\r\n```\r\nUnfortunately that would trigger an unsigned int (`size_t`) underflow at the next iteration at the line:\r\n```C++\r\nif (value.size() - stringoffset >= 4) // (size_t(-1) >= size_t(4)) == true\r\n    stringoffset += 4; // value.size() + 1 + 4\r\n```\r\nAgain, setting the offset further outside the bounds of the string. However, `StringIndex::create_key` handles this case just fine, so instead of having this complex (and slightly buggy) branch, I've changed the offset to always increment by 4.\r\n\r\nA long explanation for something that should be simple. Phew.","created_at":"2017-02-17T09:12:09Z","updated_at":"2017-02-25T05:33:27Z","closed_at":"2017-02-17T11:55:13Z","merged_at":"2017-02-17T11:55:13Z","merge_commit_sha":"9d1817007467c7e98f6862edeff99b449911bf19","assignee":{"login":"danielpovlsen","id":77737,"avatar_url":"https://avatars.githubusercontent.com/u/77737?v=3","gravatar_id":"","url":"https://api.github.com/users/danielpovlsen","html_url":"https://github.com/danielpovlsen","followers_url":"https://api.github.com/users/danielpovlsen/followers","following_url":"https://api.github.com/users/danielpovlsen/following{/other_user}","gists_url":"https://api.github.com/users/danielpovlsen/gists{/gist_id}","starred_url":"https://api.github.com/users/danielpovlsen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danielpovlsen/subscriptions","organizations_url":"https://api.github.com/users/danielpovlsen/orgs","repos_url":"https://api.github.com/users/danielpovlsen/repos","events_url":"https://api.github.com/users/danielpovlsen/events{/privacy}","received_events_url":"https://api.github.com/users/danielpovlsen/received_events","type":"User","site_admin":false},"assignees":[{"login":"danielpovlsen","id":77737,"avatar_url":"https://avatars.githubusercontent.com/u/77737?v=3","gravatar_id":"","url":"https://api.github.com/users/danielpovlsen","html_url":"https://github.com/danielpovlsen","followers_url":"https://api.github.com/users/danielpovlsen/followers","following_url":"https://api.github.com/users/danielpovlsen/following{/other_user}","gists_url":"https://api.github.com/users/danielpovlsen/gists{/gist_id}","starred_url":"https://api.github.com/users/danielpovlsen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danielpovlsen/subscriptions","organizations_url":"https://api.github.com/users/danielpovlsen/orgs","repos_url":"https://api.github.com/users/danielpovlsen/repos","events_url":"https://api.github.com/users/danielpovlsen/events{/privacy}","received_events_url":"https://api.github.com/users/danielpovlsen/received_events","type":"User","site_admin":false}],"milestone":null,"commits_url":"https://api.github.com/repos/realm/realm-core/pulls/2458/commits","review_comments_url":"https://api.github.com/repos/realm/realm-core/pulls/2458/comments","review_comment_url":"https://api.github.com/repos/realm/realm-core/pulls/comments{/number}","comments_url":"https://api.github.com/repos/realm/realm-core/issues/2458/comments","statuses_url":"https://api.github.com/repos/realm/realm-core/statuses/bee3ccb4060c1ddf3e039d50b7fd2f2a5fd10e66","head":{"label":"realm:dp/string_index_improvements","ref":"dp/string_index_improvements","sha":"bee3ccb4060c1ddf3e039d50b7fd2f2a5fd10e66","user":{"login":"realm","id":7575099,"avatar_url":"https://avatars.githubusercontent.com/u/7575099?v=3","gravatar_id":"","url":"https://api.github.com/users/realm","html_url":"https://github.com/realm","followers_url":"https://api.github.com/users/realm/followers","following_url":"https://api.github.com/users/realm/following{/other_user}","gists_url":"https://api.github.com/users/realm/gists{/gist_id}","starred_url":"https://api.github.com/users/realm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/realm/subscriptions","organizations_url":"https://api.github.com/users/realm/orgs","repos_url":"https://api.github.com/users/realm/repos","events_url":"https://api.github.com/users/realm/events{/privacy}","received_events_url":"https://api.github.com/users/realm/received_events","type":"Organization","site_admin":false},"repo":{"id":1917262,"name":"realm-core","full_name":"realm/realm-core","owner":{"login":"realm","id":7575099,"avatar_url":"https://avatars.githubusercontent.com/u/7575099?v=3","gravatar_id":"","url":"https://api.github.com/users/realm","html_url":"https://github.com/realm","followers_url":"https://api.github.com/users/realm/followers","following_url":"https://api.github.com/users/realm/following{/other_user}","gists_url":"https://api.github.com/users/realm/gists{/gist_id}","starred_url":"https://api.github.com/users/realm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/realm/subscriptions","organizations_url":"https://api.github.com/users/realm/orgs","repos_url":"https://api.github.com/users/realm/repos","events_url":"https://api.github.com/users/realm/events{/privacy}","received_events_url":"https://api.github.com/users/realm/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/realm/realm-core","description":"Core database component for the Realm Mobile Database SDKs","fork":false,"url":"https://api.github.com/repos/realm/realm-core","forks_url":"https://api.github.com/repos/realm/realm-core/forks","keys_url":"https://api.github.com/repos/realm/realm-core/keys{/key_id}","collaborators_url":"https://api.github.com/repos/realm/realm-core/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/realm/realm-core/teams","hooks_url":"https://api.github.com/repos/realm/realm-core/hooks","issue_events_url":"https://api.github.com/repos/realm/realm-core/issues/events{/number}","events_url":"https://api.github.com/repos/realm/realm-core/events","assignees_url":"https://api.github.com/repos/realm/realm-core/assignees{/user}","branches_url":"https://api.github.com/repos/realm/realm-core/branches{/branch}","tags_url":"https://api.github.com/repos/realm/realm-core/tags","blobs_url":"https://api.github.com/repos/realm/realm-core/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/realm/realm-core/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/realm/realm-core/git/refs{/sha}","trees_url":"https://api.github.com/repos/realm/realm-core/git/trees{/sha}","statuses_url":"https://api.github.com/repos/realm/realm-core/statuses/{sha}","languages_url":"https://api.github.com/repos/realm/realm-core/languages","stargazers_url":"https://api.github.com/repos/realm/realm-core/stargazers","contributors_url":"https://api.github.com/repos/realm/realm-core/contributors","subscribers_url":"https://api.github.com/repos/realm/realm-core/subscribers","subscription_url":"https://api.github.com/repos/realm/realm-core/subscription","commits_url":"https://api.github.com/repos/realm/realm-core/commits{/sha}","git_commits_url":"https://api.github.com/repos/realm/realm-core/git/commits{/sha}","comments_url":"https://api.github.com/repos/realm/realm-core/comments{/number}","issue_comment_url":"https://api.github.com/repos/realm/realm-core/issues/comments{/number}","contents_url":"https://api.github.com/repos/realm/realm-core/contents/{+path}","compare_url":"https://api.github.com/repos/realm/realm-core/compare/{base}...{head}","merges_url":"https://api.github.com/repos/realm/realm-core/merges","archive_url":"https://api.github.com/repos/realm/realm-core/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/realm/realm-core/downloads","issues_url":"https://api.github.com/repos/realm/realm-core/issues{/number}","pulls_url":"https://api.github.com/repos/realm/realm-core/pulls{/number}","milestones_url":"https://api.github.com/repos/realm/realm-core/milestones{/number}","notifications_url":"https://api.github.com/repos/realm/realm-core/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/realm/realm-core/labels{/name}","releases_url":"https://api.github.com/repos/realm/realm-core/releases{/id}","deployments_url":"https://api.github.com/repos/realm/realm-core/deployments","created_at":"2011-06-18T21:18:46Z","updated_at":"2017-02-23T16:55:11Z","pushed_at":"2017-02-25T04:52:21Z","git_url":"git://github.com/realm/realm-core.git","ssh_url":"git@github.com:realm/realm-core.git","clone_url":"https://github.com/realm/realm-core.git","svn_url":"https://github.com/realm/realm-core","homepage":"https://realm.io","size":53230,"stargazers_count":512,"watchers_count":512,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":29,"mirror_url":null,"open_issues_count":198,"forks":29,"open_issues":198,"watchers":512,"default_branch":"master"}},"base":{"label":"realm:develop","ref":"develop","sha":"18fa18796949c7cc26c80f084da7e0000da055b8","user":{"login":"realm","id":7575099,"avatar_url":"https://avatars.githubusercontent.com/u/7575099?v=3","gravatar_id":"","url":"https://api.github.com/users/realm","html_url":"https://github.com/realm","followers_url":"https://api.github.com/users/realm/followers","following_url":"https://api.github.com/users/realm/following{/other_user}","gists_url":"https://api.github.com/users/realm/gists{/gist_id}","starred_url":"https://api.github.com/users/realm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/realm/subscriptions","organizations_url":"https://api.github.com/users/realm/orgs","repos_url":"https://api.github.com/users/realm/repos","events_url":"https://api.github.com/users/realm/events{/privacy}","received_events_url":"https://api.github.com/users/realm/received_events","type":"Organization","site_admin":false},"repo":{"id":1917262,"name":"realm-core","full_name":"realm/realm-core","owner":{"login":"realm","id":7575099,"avatar_url":"https://avatars.githubusercontent.com/u/7575099?v=3","gravatar_id":"","url":"https://api.github.com/users/realm","html_url":"https://github.com/realm","followers_url":"https://api.github.com/users/realm/followers","following_url":"https://api.github.com/users/realm/following{/other_user}","gists_url":"https://api.github.com/users/realm/gists{/gist_id}","starred_url":"https://api.github.com/users/realm/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/realm/subscriptions","organizations_url":"https://api.github.com/users/realm/orgs","repos_url":"https://api.github.com/users/realm/repos","events_url":"https://api.github.com/users/realm/events{/privacy}","received_events_url":"https://api.github.com/users/realm/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/realm/realm-core","description":"Core database component for the Realm Mobile Database SDKs","fork":false,"url":"https://api.github.com/repos/realm/realm-core","forks_url":"https://api.github.com/repos/realm/realm-core/forks","keys_url":"https://api.github.com/repos/realm/realm-core/keys{/key_id}","collaborators_url":"https://api.github.com/repos/realm/realm-core/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/realm/realm-core/teams","hooks_url":"https://api.github.com/repos/realm/realm-core/hooks","issue_events_url":"https://api.github.com/repos/realm/realm-core/issues/events{/number}","events_url":"https://api.github.com/repos/realm/realm-core/events","assignees_url":"https://api.github.com/repos/realm/realm-core/assignees{/user}","branches_url":"https://api.github.com/repos/realm/realm-core/branches{/branch}","tags_url":"https://api.github.com/repos/realm/realm-core/tags","blobs_url":"https://api.github.com/repos/realm/realm-core/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/realm/realm-core/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/realm/realm-core/git/refs{/sha}","trees_url":"https://api.github.com/repos/realm/realm-core/git/trees{/sha}","statuses_url":"https://api.github.com/repos/realm/realm-core/statuses/{sha}","languages_url":"https://api.github.com/repos/realm/realm-core/languages","stargazers_url":"https://api.github.com/repos/realm/realm-core/stargazers","contributors_url":"https://api.github.com/repos/realm/realm-core/contributors","subscribers_url":"https://api.github.com/repos/realm/realm-core/subscribers","subscription_url":"https://api.github.com/repos/realm/realm-core/subscription","commits_url":"https://api.github.com/repos/realm/realm-core/commits{/sha}","git_commits_url":"https://api.github.com/repos/realm/realm-core/git/commits{/sha}","comments_url":"https://api.github.com/repos/realm/realm-core/comments{/number}","issue_comment_url":"https://api.github.com/repos/realm/realm-core/issues/comments{/number}","contents_url":"https://api.github.com/repos/realm/realm-core/contents/{+path}","compare_url":"https://api.github.com/repos/realm/realm-core/compare/{base}...{head}","merges_url":"https://api.github.com/repos/realm/realm-core/merges","archive_url":"https://api.github.com/repos/realm/realm-core/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/realm/realm-core/downloads","issues_url":"https://api.github.com/repos/realm/realm-core/issues{/number}","pulls_url":"https://api.github.com/repos/realm/realm-core/pulls{/number}","milestones_url":"https://api.github.com/repos/realm/realm-core/milestones{/number}","notifications_url":"https://api.github.com/repos/realm/realm-core/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/realm/realm-core/labels{/name}","releases_url":"https://api.github.com/repos/realm/realm-core/releases{/id}","deployments_url":"https://api.github.com/repos/realm/realm-core/deployments","created_at":"2011-06-18T21:18:46Z","updated_at":"2017-02-23T16:55:11Z","pushed_at":"2017-02-25T04:52:21Z","git_url":"git://github.com/realm/realm-core.git","ssh_url":"git@github.com:realm/realm-core.git","clone_url":"https://github.com/realm/realm-core.git","svn_url":"https://github.com/realm/realm-core","homepage":"https://realm.io","size":53230,"stargazers_count":512,"watchers_count":512,"language":"C++","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":29,"mirror_url":null,"open_issues_count":198,"forks":29,"open_issues":198,"watchers":512,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/realm/realm-core/pulls/2458"},"html":{"href":"https://github.com/realm/realm-core/pull/2458"},"issue":{"href":"https://api.github.com/repos/realm/realm-core/issues/2458"},"comments":{"href":"https://api.github.com/repos/realm/realm-core/issues/2458/comments"},"review_comments":{"href":"https://api.github.com/repos/realm/realm-core/pulls/2458/comments"},"review_comment":{"href":"https://api.github.com/repos/realm/realm-core/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/realm/realm-core/pulls/2458/commits"},"statuses":{"href":"https://api.github.com/repos/realm/realm-core/statuses/bee3ccb4060c1ddf3e039d50b7fd2f2a5fd10e66"}}}}
{"actor":{"display_login":"ironage"}}