{"action":"created","comment":{"url":"https://api.github.com/repos/rails/rails/pulls/comments/223469288","pull_request_review_id":162584923,"id":223469288,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDIyMzQ2OTI4OA==","diff_hunk":"@@ -0,0 +1,165 @@\n+# frozen_string_literal: true\n+\n+module ActiveSupport\n+  module Cache\n+    # This class is responsible for cohercing input into a valid\n+    # key format and cache version for cache stores.\n+    #\n+    # The goal of this class is to provide a composable way\n+    # to build keys. You can put an instance of KeyMaster\n+    # into another instance of KeyMaster and it should produce\n+    # the same `cache_key` and `cache_version`.\n+    #\n+    # Example:\n+    #\n+    #   k1 = KeyMaster.new(\"i_aint_afraid_of_no_ghosts\")\n+    #   k2 = KeyMaster.new(k1)\n+    #   k1.cache_key == k2.cache_key\n+    #   # => true\n+    #\n+    # In addition to passing a key into the initialize method\n+    # key fragments can be added onto an existing key through the\n+    # `<<` method.\n+    #\n+    # Example:\n+    #\n+    #   key = KeyMaster.new\n+    #   key << \"who-ya-gonna-call\"\n+    #   key << \"ghost-busters\"\n+    #   key.cache_key\n+    #   # => \"who-ya-gonna-call/ghost-busters\"\n+    #\n+    # Multiple key entries generate the same key regardless of how\n+    # they are added. I.e. if they're added via multiple arrays\n+    # or directly via the `<<` operator.\n+    #\n+    # Example:\n+    #\n+    #   k1 = KeyMaster.new(\"slimer\")\n+    #   k1 << \"stay-puft\"\n+    #   k2 = KeyMaster.new([\"slimer\", \"stay-puft\"])\n+    #   k1.cache_key == k2.cache_key\n+    #   # => true\n+    #   k3 = KeyMaster.new([\"slimer\", [\"stay-puft\"]])\n+    #   k2.cache_key == k3.cache_key\n+    #   # => true\n+    #\n+    # It is faster to directly add objects to the\n+    # KeyMaster object via `<<` than to generate\n+    # an intermediate array of objects\n+    #\n+    # The class supports strings, arrays, hashes,\n+    # objects with a `cache_key` method and anything\n+    # that can be implicitly cast to a string:\n+    #\n+    # Examples:\n+    #\n+    #   KeyMaster.new(\"there\").cache_key\n+    #   # => \"there\"\n+    #\n+    #   KeyMaster.new([\"is\", \"no\"]).cache_key\n+    #   # => \"is/no\"\n+    #\n+    #   KeyMaster.new({ dana: 1, only: 2 }).cache_key\n+    #   # => \"dana=1/only=2\"\n+    #\n+    #   klass = Class.new { def cache_key; \"zuul\"; end }\n+    #   KeyMaster.new(klass.new).cache_key\n+    #   # => \"zuul\"\n+    #\n+    # When generating a a cache key, `nil` will be filtered\n+    # out, so `[\"ghost\", nil \"busters\"]` will generate the\n+    # same output as [\"ghost\", \"busters\"]. If presence of a\n+    # `nil` is important, then convert it to a string.\n+    #\n+    # In addition to generating a `cache_key`\n+    # a `cache_version` is also generated if the object\n+    # passed in responds to `cache_version` or if it contains\n+    # an element that responds to `cache_version`.\n+    #\n+    # The purpose of keeping this value seperate from\n+    # `cache_key` is to support the use of \"cache versioning\"\n+    # where the same key is recycled but the version of the object\n+    # being stored is kept directly in the cache.\n+    #\n+    # Example:\n+    #\n+    #   klass = Class.new { def cache_version; \"patty\"; end }\n+    #   KeyMaster.new(klass.new).cache_version\n+    #   # => \"patty\"\n+    #\n+    #   klass = Class.new { def cache_version; \"egon\"; end }\n+    #   KeyMaster.new([klass.new]).cache_version\n+    #   # => \"egon\"\n+    #\n+    #   klass = Class.new { def cache_version; \"venkman\"; end }\n+    #   KeyMaster.new({foo: klass.new}).cache_version\n+    #   # => \"venkman\"\n+    class KeyMaster # :nodoc:\n+      attr_reader :cache_key, :cache_version\n+\n+      def self.to_key(key)\n+        return key if key.is_a?(KeyMaster)\n+\n+        self.new(key)\n+      end","path":"activesupport/lib/active_support/cache/key_master.rb","position":105,"original_position":105,"commit_id":"86908fb6c0e26fcafc7dd09af463550800898569","original_commit_id":"86908fb6c0e26fcafc7dd09af463550800898569","user":{"login":"jeremy","id":199,"node_id":"MDQ6VXNlcjE5OQ==","avatar_url":"https://avatars1.githubusercontent.com/u/199?v=4","gravatar_id":"","url":"https://api.github.com/users/jeremy","html_url":"https://github.com/jeremy","followers_url":"https://api.github.com/users/jeremy/followers","following_url":"https://api.github.com/users/jeremy/following{/other_user}","gists_url":"https://api.github.com/users/jeremy/gists{/gist_id}","starred_url":"https://api.github.com/users/jeremy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jeremy/subscriptions","organizations_url":"https://api.github.com/users/jeremy/orgs","repos_url":"https://api.github.com/users/jeremy/repos","events_url":"https://api.github.com/users/jeremy/events{/privacy}","received_events_url":"https://api.github.com/users/jeremy/received_events","type":"User","site_admin":false},"body":"Would compare before/after usage with `.for(key)` or simply `.new(key)`. The `to_*` methods indicate that the receiver is being coerced, not the argument.\r\n\r\nNo need for a guard here, since it's expected that a key may be an instance of this class. A conditional expression is appropriate.","created_at":"2018-10-08T19:10:11Z","updated_at":"2018-10-08T19:53:05Z","html_url":"https://github.com/rails/rails/pull/33944#discussion_r223469288","pull_request_url":"https://api.github.com/repos/rails/rails/pulls/33944","author_association":"MEMBER","_links":{"self":{"href":"https://api.github.com/repos/rails/rails/pulls/comments/223469288"},"html":{"href":"https://github.com/rails/rails/pull/33944#discussion_r223469288"},"pull_request":{"href":"https://api.github.com/repos/rails/rails/pulls/33944"}}},"pull_request":{"url":"https://api.github.com/repos/rails/rails/pulls/33944","id":217376338,"node_id":"MDExOlB1bGxSZXF1ZXN0MjE3Mzc2MzM4","html_url":"https://github.com/rails/rails/pull/33944","diff_url":"https://github.com/rails/rails/pull/33944.diff","patch_url":"https://github.com/rails/rails/pull/33944.patch","issue_url":"https://api.github.com/repos/rails/rails/issues/33944","number":33944,"state":"open","locked":false,"title":"WIP - Cache-key-object \"Keymaster\" - 7% faster Pages, 4.6% fewer bytes allocated","user":{"login":"schneems","id":59744,"node_id":"MDQ6VXNlcjU5NzQ0","avatar_url":"https://avatars2.githubusercontent.com/u/59744?v=4","gravatar_id":"","url":"https://api.github.com/users/schneems","html_url":"https://github.com/schneems","followers_url":"https://api.github.com/users/schneems/followers","following_url":"https://api.github.com/users/schneems/following{/other_user}","gists_url":"https://api.github.com/users/schneems/gists{/gist_id}","starred_url":"https://api.github.com/users/schneems/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/schneems/subscriptions","organizations_url":"https://api.github.com/users/schneems/orgs","repos_url":"https://api.github.com/users/schneems/repos","events_url":"https://api.github.com/users/schneems/events{/privacy}","received_events_url":"https://api.github.com/users/schneems/received_events","type":"User","site_admin":false},"body":"This is an experimental proof of concept to improve speed when generating cache keys. Currently, a cache key can be anything. Usually, this means that it's an array that has other things in it. Usually, this means other arrays. \r\n\r\nIn this process we don't control the full key from beginning to end, because of this there are lots of very small inefficiencies. For example, we repeatedly must call `compact` on arrays to deal with nil elements when instead we can simply reject a nil element when it's added to the cache key object. \r\n\r\nThe process currently generates intermediate objects even when the ultimate end is a string. Instead, we can make one primary string key object and append to it.\r\n\r\nAnother inefficiency is that similar transformations must be performed for both generating a key and version. If one object controls both calculations we can re-use intermediate representations when possible.\r\n\r\nIn collection_caching.rb each iteration of the cache generates an array what will eventually be an identical string. This is expensive. Instead, we can generate the string once and use it as a base for all future keys.\r\n\r\nThis class currently represents the existing functionality of `normalize_version` and `normalize_key`.\r\n\r\n## Performance\r\n\r\nNote since this is very preliminary work there might be a significant error in the existing implementation that is causing unusually good performance. These numbers shouldn't be considered final until all tests are passing and the patch is ready to move.\r\n\r\nI'm presenting these numbers as a demonstration of what such a performance improvement **if it works** could accomplish, at best.\r\n\r\n### Memory\r\n\r\nBefore:\r\n\r\n```\r\nTotal allocated: 741750 bytes (6645 objects)\r\nTotal retained:  18917 bytes (166 objects)\r\n```\r\n\r\nAfter:\r\n\r\n```\r\nTotal allocated: 707548 bytes (6006 objects)\r\nTotal retained:  18917 bytes (166 objects)\r\n```\r\n\r\nDiff:\r\n\r\n```\r\n(741750 - 707548) / 741750 # => ~4.6 %\r\n```\r\n\r\n### Performance \r\n\r\nHere's the raw results: https://gist.github.com/schneems/7e808e7a866c1bc3a2d50b5e0ae4db88 for 63 runs, each with 1000 hits to the index of CodeTriage.\r\n\r\nOn average this patch is **7.24%** faster than the commit before it. This number is statistically significant. \r\n\r\n\r\n## Future work\r\n\r\nGet tests passing, add extra tests to ensure key format is identical to the current format. Then we need to decide what to do with our existing methods, duplicating this logic isn't great, the current methods such as `expanded_key` and `expanded_version` are private but if a class inherits from this class it can technically be used. There's also other behavior such as `retrieve_cache_key` and namespacing. Also not sure if we would expose this class or keep it private as an internal representation.\r\n","created_at":"2018-09-21T19:14:51Z","updated_at":"2018-10-08T19:53:05Z","closed_at":null,"merged_at":null,"merge_commit_sha":"b25dd21b287e04792c9c6796d359d8698946d4f3","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[{"id":3666649,"node_id":"MDU6TGFiZWwzNjY2NjQ5","url":"https://api.github.com/repos/rails/rails/labels/actionview","name":"actionview","color":"d7e102","default":false},{"id":107194,"node_id":"MDU6TGFiZWwxMDcxOTQ=","url":"https://api.github.com/repos/rails/rails/labels/activesupport","name":"activesupport","color":"FC9300","default":false}],"milestone":null,"commits_url":"https://api.github.com/repos/rails/rails/pulls/33944/commits","review_comments_url":"https://api.github.com/repos/rails/rails/pulls/33944/comments","review_comment_url":"https://api.github.com/repos/rails/rails/pulls/comments{/number}","comments_url":"https://api.github.com/repos/rails/rails/issues/33944/comments","statuses_url":"https://api.github.com/repos/rails/rails/statuses/86908fb6c0e26fcafc7dd09af463550800898569","head":{"label":"schneems:schneems/keymaster","ref":"schneems/keymaster","sha":"86908fb6c0e26fcafc7dd09af463550800898569","user":{"login":"schneems","id":59744,"node_id":"MDQ6VXNlcjU5NzQ0","avatar_url":"https://avatars2.githubusercontent.com/u/59744?v=4","gravatar_id":"","url":"https://api.github.com/users/schneems","html_url":"https://github.com/schneems","followers_url":"https://api.github.com/users/schneems/followers","following_url":"https://api.github.com/users/schneems/following{/other_user}","gists_url":"https://api.github.com/users/schneems/gists{/gist_id}","starred_url":"https://api.github.com/users/schneems/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/schneems/subscriptions","organizations_url":"https://api.github.com/users/schneems/orgs","repos_url":"https://api.github.com/users/schneems/repos","events_url":"https://api.github.com/users/schneems/events{/privacy}","received_events_url":"https://api.github.com/users/schneems/received_events","type":"User","site_admin":false},"repo":{"id":2873136,"node_id":"MDEwOlJlcG9zaXRvcnkyODczMTM2","name":"rails","full_name":"schneems/rails","private":false,"owner":{"login":"schneems","id":59744,"node_id":"MDQ6VXNlcjU5NzQ0","avatar_url":"https://avatars2.githubusercontent.com/u/59744?v=4","gravatar_id":"","url":"https://api.github.com/users/schneems","html_url":"https://github.com/schneems","followers_url":"https://api.github.com/users/schneems/followers","following_url":"https://api.github.com/users/schneems/following{/other_user}","gists_url":"https://api.github.com/users/schneems/gists{/gist_id}","starred_url":"https://api.github.com/users/schneems/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/schneems/subscriptions","organizations_url":"https://api.github.com/users/schneems/orgs","repos_url":"https://api.github.com/users/schneems/repos","events_url":"https://api.github.com/users/schneems/events{/privacy}","received_events_url":"https://api.github.com/users/schneems/received_events","type":"User","site_admin":false},"html_url":"https://github.com/schneems/rails","description":"Ruby on Rails","fork":true,"url":"https://api.github.com/repos/schneems/rails","forks_url":"https://api.github.com/repos/schneems/rails/forks","keys_url":"https://api.github.com/repos/schneems/rails/keys{/key_id}","collaborators_url":"https://api.github.com/repos/schneems/rails/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/schneems/rails/teams","hooks_url":"https://api.github.com/repos/schneems/rails/hooks","issue_events_url":"https://api.github.com/repos/schneems/rails/issues/events{/number}","events_url":"https://api.github.com/repos/schneems/rails/events","assignees_url":"https://api.github.com/repos/schneems/rails/assignees{/user}","branches_url":"https://api.github.com/repos/schneems/rails/branches{/branch}","tags_url":"https://api.github.com/repos/schneems/rails/tags","blobs_url":"https://api.github.com/repos/schneems/rails/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/schneems/rails/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/schneems/rails/git/refs{/sha}","trees_url":"https://api.github.com/repos/schneems/rails/git/trees{/sha}","statuses_url":"https://api.github.com/repos/schneems/rails/statuses/{sha}","languages_url":"https://api.github.com/repos/schneems/rails/languages","stargazers_url":"https://api.github.com/repos/schneems/rails/stargazers","contributors_url":"https://api.github.com/repos/schneems/rails/contributors","subscribers_url":"https://api.github.com/repos/schneems/rails/subscribers","subscription_url":"https://api.github.com/repos/schneems/rails/subscription","commits_url":"https://api.github.com/repos/schneems/rails/commits{/sha}","git_commits_url":"https://api.github.com/repos/schneems/rails/git/commits{/sha}","comments_url":"https://api.github.com/repos/schneems/rails/comments{/number}","issue_comment_url":"https://api.github.com/repos/schneems/rails/issues/comments{/number}","contents_url":"https://api.github.com/repos/schneems/rails/contents/{+path}","compare_url":"https://api.github.com/repos/schneems/rails/compare/{base}...{head}","merges_url":"https://api.github.com/repos/schneems/rails/merges","archive_url":"https://api.github.com/repos/schneems/rails/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/schneems/rails/downloads","issues_url":"https://api.github.com/repos/schneems/rails/issues{/number}","pulls_url":"https://api.github.com/repos/schneems/rails/pulls{/number}","milestones_url":"https://api.github.com/repos/schneems/rails/milestones{/number}","notifications_url":"https://api.github.com/repos/schneems/rails/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/schneems/rails/labels{/name}","releases_url":"https://api.github.com/repos/schneems/rails/releases{/id}","deployments_url":"https://api.github.com/repos/schneems/rails/deployments","created_at":"2011-11-29T06:48:49Z","updated_at":"2018-09-21T14:33:56Z","pushed_at":"2018-10-08T19:51:10Z","git_url":"git://github.com/schneems/rails.git","ssh_url":"git@github.com:schneems/rails.git","clone_url":"https://github.com/schneems/rails.git","svn_url":"https://github.com/schneems/rails","homepage":"http://rubyonrails.org","size":158136,"stargazers_count":2,"watchers_count":2,"language":"Ruby","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"archived":false,"open_issues_count":0,"license":{"key":"mit","name":"MIT License","spdx_id":"MIT","url":"https://api.github.com/licenses/mit","node_id":"MDc6TGljZW5zZTEz"},"forks":0,"open_issues":0,"watchers":2,"default_branch":"master"}},"base":{"label":"rails:master","ref":"master","sha":"3882a4d0d475f1347a7dc3774e9285c80b1c8fe2","user":{"login":"rails","id":4223,"node_id":"MDEyOk9yZ2FuaXphdGlvbjQyMjM=","avatar_url":"https://avatars1.githubusercontent.com/u/4223?v=4","gravatar_id":"","url":"https://api.github.com/users/rails","html_url":"https://github.com/rails","followers_url":"https://api.github.com/users/rails/followers","following_url":"https://api.github.com/users/rails/following{/other_user}","gists_url":"https://api.github.com/users/rails/gists{/gist_id}","starred_url":"https://api.github.com/users/rails/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rails/subscriptions","organizations_url":"https://api.github.com/users/rails/orgs","repos_url":"https://api.github.com/users/rails/repos","events_url":"https://api.github.com/users/rails/events{/privacy}","received_events_url":"https://api.github.com/users/rails/received_events","type":"Organization","site_admin":false},"repo":{"id":8514,"node_id":"MDEwOlJlcG9zaXRvcnk4NTE0","name":"rails","full_name":"rails/rails","private":false,"owner":{"login":"rails","id":4223,"node_id":"MDEyOk9yZ2FuaXphdGlvbjQyMjM=","avatar_url":"https://avatars1.githubusercontent.com/u/4223?v=4","gravatar_id":"","url":"https://api.github.com/users/rails","html_url":"https://github.com/rails","followers_url":"https://api.github.com/users/rails/followers","following_url":"https://api.github.com/users/rails/following{/other_user}","gists_url":"https://api.github.com/users/rails/gists{/gist_id}","starred_url":"https://api.github.com/users/rails/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rails/subscriptions","organizations_url":"https://api.github.com/users/rails/orgs","repos_url":"https://api.github.com/users/rails/repos","events_url":"https://api.github.com/users/rails/events{/privacy}","received_events_url":"https://api.github.com/users/rails/received_events","type":"Organization","site_admin":false},"html_url":"https://github.com/rails/rails","description":"Ruby on Rails","fork":false,"url":"https://api.github.com/repos/rails/rails","forks_url":"https://api.github.com/repos/rails/rails/forks","keys_url":"https://api.github.com/repos/rails/rails/keys{/key_id}","collaborators_url":"https://api.github.com/repos/rails/rails/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/rails/rails/teams","hooks_url":"https://api.github.com/repos/rails/rails/hooks","issue_events_url":"https://api.github.com/repos/rails/rails/issues/events{/number}","events_url":"https://api.github.com/repos/rails/rails/events","assignees_url":"https://api.github.com/repos/rails/rails/assignees{/user}","branches_url":"https://api.github.com/repos/rails/rails/branches{/branch}","tags_url":"https://api.github.com/repos/rails/rails/tags","blobs_url":"https://api.github.com/repos/rails/rails/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/rails/rails/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/rails/rails/git/refs{/sha}","trees_url":"https://api.github.com/repos/rails/rails/git/trees{/sha}","statuses_url":"https://api.github.com/repos/rails/rails/statuses/{sha}","languages_url":"https://api.github.com/repos/rails/rails/languages","stargazers_url":"https://api.github.com/repos/rails/rails/stargazers","contributors_url":"https://api.github.com/repos/rails/rails/contributors","subscribers_url":"https://api.github.com/repos/rails/rails/subscribers","subscription_url":"https://api.github.com/repos/rails/rails/subscription","commits_url":"https://api.github.com/repos/rails/rails/commits{/sha}","git_commits_url":"https://api.github.com/repos/rails/rails/git/commits{/sha}","comments_url":"https://api.github.com/repos/rails/rails/comments{/number}","issue_comment_url":"https://api.github.com/repos/rails/rails/issues/comments{/number}","contents_url":"https://api.github.com/repos/rails/rails/contents/{+path}","compare_url":"https://api.github.com/repos/rails/rails/compare/{base}...{head}","merges_url":"https://api.github.com/repos/rails/rails/merges","archive_url":"https://api.github.com/repos/rails/rails/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/rails/rails/downloads","issues_url":"https://api.github.com/repos/rails/rails/issues{/number}","pulls_url":"https://api.github.com/repos/rails/rails/pulls{/number}","milestones_url":"https://api.github.com/repos/rails/rails/milestones{/number}","notifications_url":"https://api.github.com/repos/rails/rails/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/rails/rails/labels{/name}","releases_url":"https://api.github.com/repos/rails/rails/releases{/id}","deployments_url":"https://api.github.com/repos/rails/rails/deployments","created_at":"2008-04-11T02:19:47Z","updated_at":"2018-10-08T19:21:28Z","pushed_at":"2018-10-08T19:51:18Z","git_url":"git://github.com/rails/rails.git","ssh_url":"git@github.com:rails/rails.git","clone_url":"https://github.com/rails/rails.git","svn_url":"https://github.com/rails/rails","homepage":"https://rubyonrails.org","size":167371,"stargazers_count":41061,"watchers_count":41061,"language":"Ruby","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":16598,"mirror_url":null,"archived":false,"open_issues_count":1064,"license":{"key":"mit","name":"MIT License","spdx_id":"MIT","url":"https://api.github.com/licenses/mit","node_id":"MDc6TGljZW5zZTEz"},"forks":16598,"open_issues":1064,"watchers":41061,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/rails/rails/pulls/33944"},"html":{"href":"https://github.com/rails/rails/pull/33944"},"issue":{"href":"https://api.github.com/repos/rails/rails/issues/33944"},"comments":{"href":"https://api.github.com/repos/rails/rails/issues/33944/comments"},"review_comments":{"href":"https://api.github.com/repos/rails/rails/pulls/33944/comments"},"review_comment":{"href":"https://api.github.com/repos/rails/rails/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/rails/rails/pulls/33944/commits"},"statuses":{"href":"https://api.github.com/repos/rails/rails/statuses/86908fb6c0e26fcafc7dd09af463550800898569"}},"author_association":"MEMBER"}}
{"actor":{"display_login":"jeremy"}}