{"action":"created","comment":{"url":"https://api.github.com/repos/gradle/gradle/pulls/comments/117625301","pull_request_review_id":39349759,"id":117625301,"diff_hunk":"@@ -15,8 +15,127 @@\n  */\n package org.gradle.cache.internal;\n \n+import com.google.common.util.concurrent.Striped;\n import org.gradle.internal.Factory;\n \n-public interface ProducerGuard<T> {\n-    <V> V guardByKey(T key, Factory<V> factory);\n+import java.util.concurrent.ConcurrentHashMap;\n+import java.util.concurrent.ConcurrentMap;\n+import java.util.concurrent.locks.Lock;\n+import java.util.concurrent.locks.ReentrantLock;\n+\n+/**\n+ * Synchronizes access to some resource, by making sure that 2 threads do not try to produce it at the same time.\n+ * The resource to be accessed is represented by a key, and the factory is whatever needs to be done to produce it.\n+ * This is <b>not</b> a cache. The factory should take care of caching wherever it makes sense.\n+ *\n+ * <p>\n+ * The concurrency level and memory usage depend on the implementation, see {@link #mapBased()}, {@link #striped()} and {@link #serial()} for details.\n+ * </p>\n+ *\n+ * @param <T> the type of key to lock on\n+ */\n+public abstract class ProducerGuard<T> {\n+\n+    /**\n+     * Creates a {@link ProducerGuard} which guarantees that different keys will block on different locks,\n+     * ensuring maximum concurrency. This comes at the cost of allocating locks for each key, leading to\n+     * relatively high memory pressure. If the above guarantee is not necessary, consider using a {@link #striped()}\n+     * guard instead.\n+     */\n+    public static <T> ProducerGuard<T> mapBased() {\n+        return new MapBasedProducerGuard<T>();\n+    }\n+\n+    /**\n+     * Creates a {@link ProducerGuard} which evenly spreads calls over a fixed number of locks.\n+     * This means that in some cases two different keys can block on the same lock. The benefit of\n+     * this strategy is that it uses only a fixed amount of memory. If your code depends on\n+     * different keys always getting different locks, use a {@link #mapBased()} guard instead.\n+     */\n+    public static <T> ProducerGuard<T> striped() {\n+        return new StripedProducerGuard<T>();\n+    }\n+\n+    /**\n+     * Creates a {@link ProducerGuard} which limits concurrency to a single factory at a time,\n+     * ignoring the key. This is mainly useful for testing.\n+     */\n+    public static <T> ProducerGuard<T> serial() {\n+        return new SerialProducerGuard<T>();\n+    }\n+\n+    private ProducerGuard() {\n+\n+    }\n+\n+    /**\n+     * Runs the given factory, guarded by the given key.\n+     *\n+     * @param key the key to lock on\n+     * @param factory the factory to run under the lock\n+     * @param <V> the type returned by the factory\n+     * @return the value returned by the factory\n+     */\n+    public final <V> V guardByKey(T key, Factory<V> factory) {\n+        Lock lock = getLock(key);\n+        try {\n+            lock.lock();\n+            return factory.create();\n+        } finally {\n+            lock.unlock();\n+            returnLock(key);\n+        }\n+    }\n+\n+    protected abstract Lock getLock(T key);\n+\n+    protected abstract void returnLock(T key);\n+\n+    private static class MapBasedProducerGuard<T> extends ProducerGuard<T> {\n+        private final ConcurrentMap<T, Lock> locks = new ConcurrentHashMap<T, Lock>();\n+\n+        @Override\n+        protected Lock getLock(T key) {\n+            Lock lock = locks.get(key);\n+            if (lock == null) {\n+                ReentrantLock newLock = new ReentrantLock();\n+                lock = locks.putIfAbsent(key, newLock);\n+                if (lock == null) {\n+                    lock = newLock;\n+                }\n+            }\n+            return lock;\n+        }\n+\n+        @Override\n+        protected void returnLock(T key) {\n+            locks.remove(key);","path":"subprojects/core/src/main/java/org/gradle/cache/internal/ProducerGuard.java","position":null,"original_position":100,"commit_id":"b21470cb9434d2fb36b3d743ae1a54deba5c897c","original_commit_id":"54f71c5b457ea042489f2f78504c9a11b8d7ddb3","user":{"login":"oehme","id":1191797,"avatar_url":"https://avatars1.githubusercontent.com/u/1191797?v=3","gravatar_id":"","url":"https://api.github.com/users/oehme","html_url":"https://github.com/oehme","followers_url":"https://api.github.com/users/oehme/followers","following_url":"https://api.github.com/users/oehme/following{/other_user}","gists_url":"https://api.github.com/users/oehme/gists{/gist_id}","starred_url":"https://api.github.com/users/oehme/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/oehme/subscriptions","organizations_url":"https://api.github.com/users/oehme/orgs","repos_url":"https://api.github.com/users/oehme/repos","events_url":"https://api.github.com/users/oehme/events{/privacy}","received_events_url":"https://api.github.com/users/oehme/received_events","type":"User","site_admin":false},"body":"Good catch! I really should have written the unit test before changing the existing implementation. There is now a test demonstrating that the contract is honored.\r\n\r\nI could make the map-based approach work by using a cache with weak keys and never calling `remove()` explictily. But that would lead to more memory pressure, so I instead reverted back to your algorithm. \r\n\r\nThe main performance gain of this PR was introducing the striped implementation for file system snapshotting (which is called much more often and suffered from the synchronization overhead). If the synchronization ever becomes a problem in dependency resoultion too, we can reevaluate the weak cache.","created_at":"2017-05-21T07:32:11Z","updated_at":"2017-05-21T07:32:11Z","html_url":"https://github.com/gradle/gradle/pull/2100#discussion_r117625301","pull_request_url":"https://api.github.com/repos/gradle/gradle/pulls/2100","_links":{"self":{"href":"https://api.github.com/repos/gradle/gradle/pulls/comments/117625301"},"html":{"href":"https://github.com/gradle/gradle/pull/2100#discussion_r117625301"},"pull_request":{"href":"https://api.github.com/repos/gradle/gradle/pulls/2100"}}},"pull_request":{"url":"https://api.github.com/repos/gradle/gradle/pulls/2100","id":121625860,"html_url":"https://github.com/gradle/gradle/pull/2100","diff_url":"https://github.com/gradle/gradle/pull/2100.diff","patch_url":"https://github.com/gradle/gradle/pull/2100.patch","issue_url":"https://api.github.com/repos/gradle/gradle/issues/2100","number":2100,"state":"open","locked":false,"title":"Add a striped ProducerGuard","user":{"login":"oehme","id":1191797,"avatar_url":"https://avatars1.githubusercontent.com/u/1191797?v=3","gravatar_id":"","url":"https://api.github.com/users/oehme","html_url":"https://github.com/oehme","followers_url":"https://api.github.com/users/oehme/followers","following_url":"https://api.github.com/users/oehme/following{/other_user}","gists_url":"https://api.github.com/users/oehme/gists{/gist_id}","starred_url":"https://api.github.com/users/oehme/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/oehme/subscriptions","organizations_url":"https://api.github.com/users/oehme/orgs","repos_url":"https://api.github.com/users/oehme/repos","events_url":"https://api.github.com/users/oehme/events{/privacy}","received_events_url":"https://api.github.com/users/oehme/received_events","type":"User","site_admin":false},"body":"The existing ProduerGuard implementation funnells all calls through a single lock\r\nto decide whether to block or whether to run the factory. It also allocates\r\nmemory for each call (by using a HashSet). It is doing so because dependency\r\nresolution (currently) needs the guarantee that calls for different keys will\r\nnever block each other. The other consumers of ProducerGuard didn't need this\r\nguarantee and were paing the price unnecessarily.\r\n\r\nThere are now two implementations:\r\n\r\nDependency resolution uses the existing implementation. This could possibly be\r\nimproved to notify only the threads blocking on a given key instead of notifying\r\nall waiters. However, this did not show up as a bottleneck during dependency resolution\r\nyet. If it ever does, using a Key->Lock cache with weak keys could work.\r\n\r\nAll other clients use a striped guard instead, which spreads the calls over a fixed\r\nnumber of locks, giving almost optimal throughput at much lower memory pressure and thread\r\ncontention. This speeds up file system snapshotting, because there the high number of calls\r\nwas bottlenecked on the synchronization of the existing implementation.","created_at":"2017-05-20T14:43:22Z","updated_at":"2017-05-21T07:32:11Z","closed_at":null,"merged_at":null,"merge_commit_sha":"dd59c139fb89480a56360191e8fe9c083be28acb","assignee":{"login":"oehme","id":1191797,"avatar_url":"https://avatars1.githubusercontent.com/u/1191797?v=3","gravatar_id":"","url":"https://api.github.com/users/oehme","html_url":"https://github.com/oehme","followers_url":"https://api.github.com/users/oehme/followers","following_url":"https://api.github.com/users/oehme/following{/other_user}","gists_url":"https://api.github.com/users/oehme/gists{/gist_id}","starred_url":"https://api.github.com/users/oehme/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/oehme/subscriptions","organizations_url":"https://api.github.com/users/oehme/orgs","repos_url":"https://api.github.com/users/oehme/repos","events_url":"https://api.github.com/users/oehme/events{/privacy}","received_events_url":"https://api.github.com/users/oehme/received_events","type":"User","site_admin":false},"assignees":[{"login":"oehme","id":1191797,"avatar_url":"https://avatars1.githubusercontent.com/u/1191797?v=3","gravatar_id":"","url":"https://api.github.com/users/oehme","html_url":"https://github.com/oehme","followers_url":"https://api.github.com/users/oehme/followers","following_url":"https://api.github.com/users/oehme/following{/other_user}","gists_url":"https://api.github.com/users/oehme/gists{/gist_id}","starred_url":"https://api.github.com/users/oehme/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/oehme/subscriptions","organizations_url":"https://api.github.com/users/oehme/orgs","repos_url":"https://api.github.com/users/oehme/repos","events_url":"https://api.github.com/users/oehme/events{/privacy}","received_events_url":"https://api.github.com/users/oehme/received_events","type":"User","site_admin":false}],"requested_reviewers":[],"milestone":{"url":"https://api.github.com/repos/gradle/gradle/milestones/29","html_url":"https://github.com/gradle/gradle/milestone/29","labels_url":"https://api.github.com/repos/gradle/gradle/milestones/29/labels","id":2409326,"number":29,"title":"4.1 RC1","description":"","creator":{"login":"eriwen","id":51534,"avatar_url":"https://avatars1.githubusercontent.com/u/51534?v=3","gravatar_id":"","url":"https://api.github.com/users/eriwen","html_url":"https://github.com/eriwen","followers_url":"https://api.github.com/users/eriwen/followers","following_url":"https://api.github.com/users/eriwen/following{/other_user}","gists_url":"https://api.github.com/users/eriwen/gists{/gist_id}","starred_url":"https://api.github.com/users/eriwen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eriwen/subscriptions","organizations_url":"https://api.github.com/users/eriwen/orgs","repos_url":"https://api.github.com/users/eriwen/repos","events_url":"https://api.github.com/users/eriwen/events{/privacy}","received_events_url":"https://api.github.com/users/eriwen/received_events","type":"User","site_admin":false},"open_issues":30,"closed_issues":1,"state":"open","created_at":"2017-03-24T14:43:16Z","updated_at":"2017-05-20T14:43:23Z","due_on":"2017-06-19T07:00:00Z","closed_at":null},"commits_url":"https://api.github.com/repos/gradle/gradle/pulls/2100/commits","review_comments_url":"https://api.github.com/repos/gradle/gradle/pulls/2100/comments","review_comment_url":"https://api.github.com/repos/gradle/gradle/pulls/comments{/number}","comments_url":"https://api.github.com/repos/gradle/gradle/issues/2100/comments","statuses_url":"https://api.github.com/repos/gradle/gradle/statuses/b21470cb9434d2fb36b3d743ae1a54deba5c897c","head":{"label":"gradle:so-faster-producer-guard","ref":"so-faster-producer-guard","sha":"b21470cb9434d2fb36b3d743ae1a54deba5c897c","user":{"login":"gradle","id":124156,"avatar_url":"https://avatars1.githubusercontent.com/u/124156?v=3","gravatar_id":"","url":"https://api.github.com/users/gradle","html_url":"https://github.com/gradle","followers_url":"https://api.github.com/users/gradle/followers","following_url":"https://api.github.com/users/gradle/following{/other_user}","gists_url":"https://api.github.com/users/gradle/gists{/gist_id}","starred_url":"https://api.github.com/users/gradle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gradle/subscriptions","organizations_url":"https://api.github.com/users/gradle/orgs","repos_url":"https://api.github.com/users/gradle/repos","events_url":"https://api.github.com/users/gradle/events{/privacy}","received_events_url":"https://api.github.com/users/gradle/received_events","type":"Organization","site_admin":false},"repo":{"id":302322,"name":"gradle","full_name":"gradle/gradle","owner":{"login":"gradle","id":124156,"avatar_url":"https://avatars1.githubusercontent.com/u/124156?v=3","gravatar_id":"","url":"https://api.github.com/users/gradle","html_url":"https://github.com/gradle","followers_url":"https://api.github.com/users/gradle/followers","following_url":"https://api.github.com/users/gradle/following{/other_user}","gists_url":"https://api.github.com/users/gradle/gists{/gist_id}","starred_url":"https://api.github.com/users/gradle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gradle/subscriptions","organizations_url":"https://api.github.com/users/gradle/orgs","repos_url":"https://api.github.com/users/gradle/repos","events_url":"https://api.github.com/users/gradle/events{/privacy}","received_events_url":"https://api.github.com/users/gradle/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/gradle/gradle","description":"Adaptable, fast automation for all","fork":false,"url":"https://api.github.com/repos/gradle/gradle","forks_url":"https://api.github.com/repos/gradle/gradle/forks","keys_url":"https://api.github.com/repos/gradle/gradle/keys{/key_id}","collaborators_url":"https://api.github.com/repos/gradle/gradle/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/gradle/gradle/teams","hooks_url":"https://api.github.com/repos/gradle/gradle/hooks","issue_events_url":"https://api.github.com/repos/gradle/gradle/issues/events{/number}","events_url":"https://api.github.com/repos/gradle/gradle/events","assignees_url":"https://api.github.com/repos/gradle/gradle/assignees{/user}","branches_url":"https://api.github.com/repos/gradle/gradle/branches{/branch}","tags_url":"https://api.github.com/repos/gradle/gradle/tags","blobs_url":"https://api.github.com/repos/gradle/gradle/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/gradle/gradle/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/gradle/gradle/git/refs{/sha}","trees_url":"https://api.github.com/repos/gradle/gradle/git/trees{/sha}","statuses_url":"https://api.github.com/repos/gradle/gradle/statuses/{sha}","languages_url":"https://api.github.com/repos/gradle/gradle/languages","stargazers_url":"https://api.github.com/repos/gradle/gradle/stargazers","contributors_url":"https://api.github.com/repos/gradle/gradle/contributors","subscribers_url":"https://api.github.com/repos/gradle/gradle/subscribers","subscription_url":"https://api.github.com/repos/gradle/gradle/subscription","commits_url":"https://api.github.com/repos/gradle/gradle/commits{/sha}","git_commits_url":"https://api.github.com/repos/gradle/gradle/git/commits{/sha}","comments_url":"https://api.github.com/repos/gradle/gradle/comments{/number}","issue_comment_url":"https://api.github.com/repos/gradle/gradle/issues/comments{/number}","contents_url":"https://api.github.com/repos/gradle/gradle/contents/{+path}","compare_url":"https://api.github.com/repos/gradle/gradle/compare/{base}...{head}","merges_url":"https://api.github.com/repos/gradle/gradle/merges","archive_url":"https://api.github.com/repos/gradle/gradle/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/gradle/gradle/downloads","issues_url":"https://api.github.com/repos/gradle/gradle/issues{/number}","pulls_url":"https://api.github.com/repos/gradle/gradle/pulls{/number}","milestones_url":"https://api.github.com/repos/gradle/gradle/milestones{/number}","notifications_url":"https://api.github.com/repos/gradle/gradle/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/gradle/gradle/labels{/name}","releases_url":"https://api.github.com/repos/gradle/gradle/releases{/id}","deployments_url":"https://api.github.com/repos/gradle/gradle/deployments","created_at":"2009-09-09T18:27:19Z","updated_at":"2017-05-21T02:08:23Z","pushed_at":"2017-05-21T07:28:42Z","git_url":"git://github.com/gradle/gradle.git","ssh_url":"git@github.com:gradle/gradle.git","clone_url":"https://github.com/gradle/gradle.git","svn_url":"https://github.com/gradle/gradle","homepage":"https://gradle.org","size":168129,"stargazers_count":4894,"watchers_count":4894,"language":"Java","has_issues":true,"has_projects":false,"has_downloads":false,"has_wiki":true,"has_pages":false,"forks_count":1757,"mirror_url":null,"open_issues_count":599,"forks":1757,"open_issues":599,"watchers":4894,"default_branch":"master"}},"base":{"label":"gradle:master","ref":"master","sha":"5203fa3fc5e03a3426029331a6c0cc6dd8e210bf","user":{"login":"gradle","id":124156,"avatar_url":"https://avatars1.githubusercontent.com/u/124156?v=3","gravatar_id":"","url":"https://api.github.com/users/gradle","html_url":"https://github.com/gradle","followers_url":"https://api.github.com/users/gradle/followers","following_url":"https://api.github.com/users/gradle/following{/other_user}","gists_url":"https://api.github.com/users/gradle/gists{/gist_id}","starred_url":"https://api.github.com/users/gradle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gradle/subscriptions","organizations_url":"https://api.github.com/users/gradle/orgs","repos_url":"https://api.github.com/users/gradle/repos","events_url":"https://api.github.com/users/gradle/events{/privacy}","received_events_url":"https://api.github.com/users/gradle/received_events","type":"Organization","site_admin":false},"repo":{"id":302322,"name":"gradle","full_name":"gradle/gradle","owner":{"login":"gradle","id":124156,"avatar_url":"https://avatars1.githubusercontent.com/u/124156?v=3","gravatar_id":"","url":"https://api.github.com/users/gradle","html_url":"https://github.com/gradle","followers_url":"https://api.github.com/users/gradle/followers","following_url":"https://api.github.com/users/gradle/following{/other_user}","gists_url":"https://api.github.com/users/gradle/gists{/gist_id}","starred_url":"https://api.github.com/users/gradle/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/gradle/subscriptions","organizations_url":"https://api.github.com/users/gradle/orgs","repos_url":"https://api.github.com/users/gradle/repos","events_url":"https://api.github.com/users/gradle/events{/privacy}","received_events_url":"https://api.github.com/users/gradle/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/gradle/gradle","description":"Adaptable, fast automation for all","fork":false,"url":"https://api.github.com/repos/gradle/gradle","forks_url":"https://api.github.com/repos/gradle/gradle/forks","keys_url":"https://api.github.com/repos/gradle/gradle/keys{/key_id}","collaborators_url":"https://api.github.com/repos/gradle/gradle/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/gradle/gradle/teams","hooks_url":"https://api.github.com/repos/gradle/gradle/hooks","issue_events_url":"https://api.github.com/repos/gradle/gradle/issues/events{/number}","events_url":"https://api.github.com/repos/gradle/gradle/events","assignees_url":"https://api.github.com/repos/gradle/gradle/assignees{/user}","branches_url":"https://api.github.com/repos/gradle/gradle/branches{/branch}","tags_url":"https://api.github.com/repos/gradle/gradle/tags","blobs_url":"https://api.github.com/repos/gradle/gradle/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/gradle/gradle/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/gradle/gradle/git/refs{/sha}","trees_url":"https://api.github.com/repos/gradle/gradle/git/trees{/sha}","statuses_url":"https://api.github.com/repos/gradle/gradle/statuses/{sha}","languages_url":"https://api.github.com/repos/gradle/gradle/languages","stargazers_url":"https://api.github.com/repos/gradle/gradle/stargazers","contributors_url":"https://api.github.com/repos/gradle/gradle/contributors","subscribers_url":"https://api.github.com/repos/gradle/gradle/subscribers","subscription_url":"https://api.github.com/repos/gradle/gradle/subscription","commits_url":"https://api.github.com/repos/gradle/gradle/commits{/sha}","git_commits_url":"https://api.github.com/repos/gradle/gradle/git/commits{/sha}","comments_url":"https://api.github.com/repos/gradle/gradle/comments{/number}","issue_comment_url":"https://api.github.com/repos/gradle/gradle/issues/comments{/number}","contents_url":"https://api.github.com/repos/gradle/gradle/contents/{+path}","compare_url":"https://api.github.com/repos/gradle/gradle/compare/{base}...{head}","merges_url":"https://api.github.com/repos/gradle/gradle/merges","archive_url":"https://api.github.com/repos/gradle/gradle/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/gradle/gradle/downloads","issues_url":"https://api.github.com/repos/gradle/gradle/issues{/number}","pulls_url":"https://api.github.com/repos/gradle/gradle/pulls{/number}","milestones_url":"https://api.github.com/repos/gradle/gradle/milestones{/number}","notifications_url":"https://api.github.com/repos/gradle/gradle/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/gradle/gradle/labels{/name}","releases_url":"https://api.github.com/repos/gradle/gradle/releases{/id}","deployments_url":"https://api.github.com/repos/gradle/gradle/deployments","created_at":"2009-09-09T18:27:19Z","updated_at":"2017-05-21T02:08:23Z","pushed_at":"2017-05-21T07:28:42Z","git_url":"git://github.com/gradle/gradle.git","ssh_url":"git@github.com:gradle/gradle.git","clone_url":"https://github.com/gradle/gradle.git","svn_url":"https://github.com/gradle/gradle","homepage":"https://gradle.org","size":168129,"stargazers_count":4894,"watchers_count":4894,"language":"Java","has_issues":true,"has_projects":false,"has_downloads":false,"has_wiki":true,"has_pages":false,"forks_count":1757,"mirror_url":null,"open_issues_count":599,"forks":1757,"open_issues":599,"watchers":4894,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/gradle/gradle/pulls/2100"},"html":{"href":"https://github.com/gradle/gradle/pull/2100"},"issue":{"href":"https://api.github.com/repos/gradle/gradle/issues/2100"},"comments":{"href":"https://api.github.com/repos/gradle/gradle/issues/2100/comments"},"review_comments":{"href":"https://api.github.com/repos/gradle/gradle/pulls/2100/comments"},"review_comment":{"href":"https://api.github.com/repos/gradle/gradle/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/gradle/gradle/pulls/2100/commits"},"statuses":{"href":"https://api.github.com/repos/gradle/gradle/statuses/b21470cb9434d2fb36b3d743ae1a54deba5c897c"}}}}
{"actor":{"display_login":"oehme"}}