From 9f3f644b83ec002cf4e7b58c1a7b79f803757d7c Mon Sep 17 00:00:00 2001 From: Matt Mitchell Date: Mon, 16 May 2016 13:33:53 -0700 Subject: [PATCH 1/3] Add some level of synchronization to the root of the API This adds some synchronization to the maps at the root of the API to avoid duplicated calls to the actual GH REST API. Specifically this is targeted around the two maps, orgs and users. This fix makes the GHPRB jenkins plugin behave much better when there are lots of projects that could build for a specific repo (even if only a few are actually triggered) There are also a few fixes around GHUser and GHPullRequest * GHPullRequest was checking a field that may be null (merged_by) when determining whether to fetch details. An unmerged PR would make a bunch of Github API calls for each property accessed. * Where GHUser was returned in various objects, we weren't going through the caching mechanism at the root, so calls to APIs on GHUSer often resulted in new REST calls. Instead, return from the cache wherever possible. --- .../org/kohsuke/github/GHCommitPointer.java | 3 +- .../org/kohsuke/github/GHCommitStatus.java | 3 +- .../java/org/kohsuke/github/GHDeployment.java | 5 +- src/main/java/org/kohsuke/github/GHGist.java | 4 +- src/main/java/org/kohsuke/github/GHIssue.java | 11 ++- .../java/org/kohsuke/github/GHMilestone.java | 4 +- .../org/kohsuke/github/GHPullRequest.java | 2 +- src/main/java/org/kohsuke/github/GitHub.java | 94 +++++++++++++------ 8 files changed, 83 insertions(+), 43 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GHCommitPointer.java b/src/main/java/org/kohsuke/github/GHCommitPointer.java index b9716366b..6b3f414f4 100644 --- a/src/main/java/org/kohsuke/github/GHCommitPointer.java +++ b/src/main/java/org/kohsuke/github/GHCommitPointer.java @@ -39,7 +39,8 @@ public class GHCommitPointer { * This points to the user who owns * the {@link #getRepository()}. */ - public GHUser getUser() { + public GHUser getUser() throws IOException { + if (user != null) return user.root.getUser(user.getLogin()); return user; } diff --git a/src/main/java/org/kohsuke/github/GHCommitStatus.java b/src/main/java/org/kohsuke/github/GHCommitStatus.java index 7b876fc5a..241f8b98b 100644 --- a/src/main/java/org/kohsuke/github/GHCommitStatus.java +++ b/src/main/java/org/kohsuke/github/GHCommitStatus.java @@ -45,7 +45,8 @@ public class GHCommitStatus extends GHObject { return description; } - public GHUser getCreator() { + public GHUser getCreator() throws IOException { + if (creator != null) return creator.root.getUser(creator.getLogin()); return creator; } diff --git a/src/main/java/org/kohsuke/github/GHDeployment.java b/src/main/java/org/kohsuke/github/GHDeployment.java index 51a7843d0..ff8b4d6af 100644 --- a/src/main/java/org/kohsuke/github/GHDeployment.java +++ b/src/main/java/org/kohsuke/github/GHDeployment.java @@ -1,6 +1,6 @@ package org.kohsuke.github; - +import java.io.IOException; import java.net.URL; public class GHDeployment extends GHObject { @@ -41,7 +41,8 @@ public class GHDeployment extends GHObject { public String getEnvironment() { return environment; } - public GHUser getCreator() { + public GHUser getCreator() throws IOException { + if(creator != null) return root.getUser(creator.getLogin()); return creator; } public String getRef() { diff --git a/src/main/java/org/kohsuke/github/GHGist.java b/src/main/java/org/kohsuke/github/GHGist.java index eb105b40c..e0954175d 100644 --- a/src/main/java/org/kohsuke/github/GHGist.java +++ b/src/main/java/org/kohsuke/github/GHGist.java @@ -38,8 +38,8 @@ public class GHGist extends GHObject { /** * User that owns this Gist. */ - public GHUser getOwner() { - return owner; + public GHUser getOwner() throws IOException { + return root.getUser(owner.getLogin()); } public String getForksUrl() { diff --git a/src/main/java/org/kohsuke/github/GHIssue.java b/src/main/java/org/kohsuke/github/GHIssue.java index a5afad6d2..76a4e0e39 100644 --- a/src/main/java/org/kohsuke/github/GHIssue.java +++ b/src/main/java/org/kohsuke/github/GHIssue.java @@ -225,15 +225,16 @@ public class GHIssue extends GHObject { return "/repos/"+owner.getOwnerName()+"/"+owner.getName()+"/issues/"+number; } - public GHUser getAssignee() { + public GHUser getAssignee() throws IOException { + if (assignee != null) return owner.root.getUser(assignee.getLogin()); return assignee; } /** * User who submitted the issue. */ - public GHUser getUser() { - return user; + public GHUser getUser() throws IOException { + return owner.root.getUser(user.getLogin()); } /** @@ -244,9 +245,9 @@ public class GHIssue extends GHObject { * even for an issue that's already closed. See * https://github.com/kohsuke/github-api/issues/60. */ - public GHUser getClosedBy() { + public GHUser getClosedBy() throws IOException { if(!"closed".equals(state)) return null; - if(closed_by != null) return closed_by; + if(closed_by != null) return owner.root.getUser(closed_by.getLogin());; //TODO closed_by = owner.getIssue(number).getClosed_by(); return closed_by; diff --git a/src/main/java/org/kohsuke/github/GHMilestone.java b/src/main/java/org/kohsuke/github/GHMilestone.java index 48f8364bb..950dea257 100644 --- a/src/main/java/org/kohsuke/github/GHMilestone.java +++ b/src/main/java/org/kohsuke/github/GHMilestone.java @@ -27,8 +27,8 @@ public class GHMilestone extends GHObject { return owner; } - public GHUser getCreator() { - return creator; + public GHUser getCreator() throws IOException { + return root.getUser(creator.getLogin()); } public Date getDueOn() { diff --git a/src/main/java/org/kohsuke/github/GHPullRequest.java b/src/main/java/org/kohsuke/github/GHPullRequest.java index dd3d93b75..4f549975f 100644 --- a/src/main/java/org/kohsuke/github/GHPullRequest.java +++ b/src/main/java/org/kohsuke/github/GHPullRequest.java @@ -200,7 +200,7 @@ public class GHPullRequest extends GHIssue { * Depending on the original API call where this object is created, it may not contain everything. */ private void populate() throws IOException { - if (merged_by!=null) return; // already populated + if (mergeable_state != null) return; // already populated by id root.retrieve().to(url, this).wrapUp(owner); } diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java index 22792a879..12e24157c 100644 --- a/src/main/java/org/kohsuke/github/GitHub.java +++ b/src/main/java/org/kohsuke/github/GitHub.java @@ -77,9 +77,10 @@ public class GitHub { */ /*package*/ final String encodedAuthorization; - private final Map users = new Hashtable(); - private final Map orgs = new Hashtable(); - + private final Map users; + private final Map orgs; + // Cache of myself object. + private GHMyself myself; private final String apiUrl; /*package*/ final RateLimitHandler rateLimitHandler; @@ -139,6 +140,8 @@ public class GitHub { } } + users = new Hashtable(); + orgs = new Hashtable(); this.rateLimitHandler = rateLimitHandler; if (login==null && encodedAuthorization!=null) @@ -226,7 +229,7 @@ public class GitHub { /** * Sets the custom connector used to make requests to GitHub. */ - public void setConnector(HttpConnector connector) { + public synchronized void setConnector(HttpConnector connector) { this.connector = connector; } @@ -276,56 +279,89 @@ public class GitHub { public GHMyself getMyself() throws IOException { requireCredential(); - GHMyself u = retrieve().to("/user", GHMyself.class); + // This entire block is under synchronization to avoid the relatively common case + // where a bunch of threads try to enter this code simultaneously. While we could + // scope the synchronization separately around the map retrieval and update (or use a concurrent hash) + // map, the point is to avoid making unnecessary GH API calls, which are expensive from + // an API rate standpoint + synchronized (this) { + if (this.myself != null) return myself; + + GHMyself u = retrieve().to("/user", GHMyself.class); - u.root = this; - users.put(u.getLogin(), u); - - return u; + u.root = this; + this.myself = u; + return u; + } } /** * Obtains the object that represents the named user. */ public GHUser getUser(String login) throws IOException { - GHUser u = users.get(login); - if (u == null) { - u = retrieve().to("/users/" + login, GHUser.class); - u.root = this; - users.put(u.getLogin(), u); + // This entire block is under synchronization to avoid the relatively common case + // where a bunch of threads try to enter this code simultaneously. While we could + // scope the synchronization separately around the map retrieval and update (or use a concurrent hash + // map), the point is to avoid making unnecessary GH API calls, which are expensive from + // an API rate standpoint + synchronized (users) { + GHUser u = users.get(login); + if (u == null) { + u = retrieve().to("/users/" + login, GHUser.class); + u.root = this; + users.put(u.getLogin(), u); + } + return u; } - return u; } - + /** * clears all cached data in order for external changes (modifications and del */ public void refreshCache() { - users.clear(); - orgs.clear(); + synchronized (users) { + users.clear(); + } + synchronized (orgs) { + orgs.clear(); + } } /** * Interns the given {@link GHUser}. */ protected GHUser getUser(GHUser orig) throws IOException { - GHUser u = users.get(orig.getLogin()); - if (u==null) { - orig.root = this; - users.put(orig.getLogin(),orig); - return orig; + // This entire block is under synchronization to avoid the relatively common case + // where a bunch of threads try to enter this code simultaneously. While we could + // scope the synchronization separately around the map retrieval and update (or use a concurrent hash + // map), the point is to avoid making unnecessary GH API calls, which are expensive from + // an API rate standpoint + synchronized (users) { + GHUser u = users.get(orig.getLogin()); + if (u==null) { + orig.root = this; + users.put(orig.getLogin(),orig); + return orig; + } + return u; } - return u; } public GHOrganization getOrganization(String name) throws IOException { - GHOrganization o = orgs.get(name); - if (o==null) { - o = retrieve().to("/orgs/" + name, GHOrganization.class).wrapUp(this); - orgs.put(name,o); + // This entire block is under synchronization to avoid the relatively common case + // where a bunch of threads try to enter this code simultaneously. While we could + // scope the synchronization separately around the map retrieval and update (or use a concurrent hash + // map), the point is to avoid making unnecessary GH API calls, which are expensive from + // an API rate standpoint + synchronized (orgs) { + GHOrganization o = orgs.get(name); + if (o==null) { + o = retrieve().to("/orgs/" + name, GHOrganization.class).wrapUp(this); + orgs.put(name,o); + } + return o; } - return o; } /** From f2a2ad90b7a8ed334444c7523f829b9892d440c1 Mon Sep 17 00:00:00 2001 From: Matt Mitchell Date: Tue, 29 Aug 2017 11:30:25 -0700 Subject: [PATCH 2/3] Switch to a concurrent hash map --- .../org/kohsuke/github/GHCommitStatus.java | 1 + src/main/java/org/kohsuke/github/GitHub.java | 69 ++++++------------- 2 files changed, 23 insertions(+), 47 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GHCommitStatus.java b/src/main/java/org/kohsuke/github/GHCommitStatus.java index 241f8b98b..f71be9962 100644 --- a/src/main/java/org/kohsuke/github/GHCommitStatus.java +++ b/src/main/java/org/kohsuke/github/GHCommitStatus.java @@ -1,5 +1,6 @@ package org.kohsuke.github; +import java.io.IOException; import java.net.URL; /** diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java index f787a2db4..bde74b126 100644 --- a/src/main/java/org/kohsuke/github/GitHub.java +++ b/src/main/java/org/kohsuke/github/GitHub.java @@ -47,6 +47,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.TimeZone; +import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Logger; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; @@ -147,8 +148,8 @@ public class GitHub { } } - users = new Hashtable(); - orgs = new Hashtable(); + users = new ConcurrentHashMap(); + orgs = new ConcurrentHashMap(); this.rateLimitHandler = rateLimitHandler; this.abuseLimitHandler = abuseLimitHandler; @@ -360,12 +361,6 @@ public class GitHub { @WithBridgeMethods(GHUser.class) public GHMyself getMyself() throws IOException { requireCredential(); - - // This entire block is under synchronization to avoid the relatively common case - // where a bunch of threads try to enter this code simultaneously. While we could - // scope the synchronization separately around the map retrieval and update (or use a concurrent hash) - // map, the point is to avoid making unnecessary GH API calls, which are expensive from - // an API rate standpoint synchronized (this) { if (this.myself != null) return myself; @@ -381,20 +376,13 @@ public class GitHub { * Obtains the object that represents the named user. */ public GHUser getUser(String login) throws IOException { - // This entire block is under synchronization to avoid the relatively common case - // where a bunch of threads try to enter this code simultaneously. While we could - // scope the synchronization separately around the map retrieval and update (or use a concurrent hash - // map), the point is to avoid making unnecessary GH API calls, which are expensive from - // an API rate standpoint - synchronized (users) { - GHUser u = users.get(login); - if (u == null) { - u = retrieve().to("/users/" + login, GHUser.class); - u.root = this; - users.put(u.getLogin(), u); - } - return u; + GHUser u = users.get(login); + if (u == null) { + u = retrieve().to("/users/" + login, GHUser.class); + u.root = this; + users.put(u.getLogin(), u); } + return u; } @@ -402,43 +390,30 @@ public class GitHub { * clears all cached data in order for external changes (modifications and del */ public void refreshCache() { - synchronized (users) { - users.clear(); - } - synchronized (orgs) { - orgs.clear(); - } + users.clear(); + orgs.clear(); } /** * Interns the given {@link GHUser}. */ protected GHUser getUser(GHUser orig) { - synchronized (users) { - GHUser u = users.get(orig.getLogin()); - if (u==null) { - orig.root = this; - users.put(orig.getLogin(),orig); - return orig; - } - return u; + GHUser u = users.get(orig.getLogin()); + if (u==null) { + orig.root = this; + users.put(orig.getLogin(),orig); + return orig; } + return u; } public GHOrganization getOrganization(String name) throws IOException { - // This entire block is under synchronization to avoid the relatively common case - // where a bunch of threads try to enter this code simultaneously. While we could - // scope the synchronization separately around the map retrieval and update (or use a concurrent hash - // map), the point is to avoid making unnecessary GH API calls, which are expensive from - // an API rate standpoint - synchronized (orgs) { - GHOrganization o = orgs.get(name); - if (o==null) { - o = retrieve().to("/orgs/" + name, GHOrganization.class).wrapUp(this); - orgs.put(name,o); - } - return o; + GHOrganization o = orgs.get(name); + if (o==null) { + o = retrieve().to("/orgs/" + name, GHOrganization.class).wrapUp(this); + orgs.put(name,o); } + return o; } /** From 2d3557e049c8c365dcaf44991ba42d5a394668c4 Mon Sep 17 00:00:00 2001 From: Kohsuke Kawaguchi Date: Sat, 9 Sep 2017 12:44:12 -0700 Subject: [PATCH 3/3] Improved the intern logic if the user record does not exist yet, there's no need to fetch that eagerly, as they are fetched on demand via the populate() method. --- .../org/kohsuke/github/GHCommitPointer.java | 2 +- .../java/org/kohsuke/github/GHCommitStatus.java | 3 +-- .../java/org/kohsuke/github/GHDeployment.java | 3 +-- src/main/java/org/kohsuke/github/GHGist.java | 2 +- src/main/java/org/kohsuke/github/GHIssue.java | 17 ++++++++++------- .../java/org/kohsuke/github/GHMilestone.java | 2 +- src/main/java/org/kohsuke/github/GitHub.java | 17 +++++++++++++++-- 7 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/main/java/org/kohsuke/github/GHCommitPointer.java b/src/main/java/org/kohsuke/github/GHCommitPointer.java index 6b3f414f4..b6c347864 100644 --- a/src/main/java/org/kohsuke/github/GHCommitPointer.java +++ b/src/main/java/org/kohsuke/github/GHCommitPointer.java @@ -40,7 +40,7 @@ public class GHCommitPointer { * the {@link #getRepository()}. */ public GHUser getUser() throws IOException { - if (user != null) return user.root.getUser(user.getLogin()); + if (user != null) return user.root.intern(user); return user; } diff --git a/src/main/java/org/kohsuke/github/GHCommitStatus.java b/src/main/java/org/kohsuke/github/GHCommitStatus.java index f71be9962..63a62ae8d 100644 --- a/src/main/java/org/kohsuke/github/GHCommitStatus.java +++ b/src/main/java/org/kohsuke/github/GHCommitStatus.java @@ -47,8 +47,7 @@ public class GHCommitStatus extends GHObject { } public GHUser getCreator() throws IOException { - if (creator != null) return creator.root.getUser(creator.getLogin()); - return creator; + return root.intern(creator); } public String getContext() { diff --git a/src/main/java/org/kohsuke/github/GHDeployment.java b/src/main/java/org/kohsuke/github/GHDeployment.java index ff8b4d6af..0cef10ee5 100644 --- a/src/main/java/org/kohsuke/github/GHDeployment.java +++ b/src/main/java/org/kohsuke/github/GHDeployment.java @@ -42,8 +42,7 @@ public class GHDeployment extends GHObject { return environment; } public GHUser getCreator() throws IOException { - if(creator != null) return root.getUser(creator.getLogin()); - return creator; + return root.intern(creator); } public String getRef() { return ref; diff --git a/src/main/java/org/kohsuke/github/GHGist.java b/src/main/java/org/kohsuke/github/GHGist.java index e0954175d..a9bf3fef4 100644 --- a/src/main/java/org/kohsuke/github/GHGist.java +++ b/src/main/java/org/kohsuke/github/GHGist.java @@ -39,7 +39,7 @@ public class GHGist extends GHObject { * User that owns this Gist. */ public GHUser getOwner() throws IOException { - return root.getUser(owner.getLogin()); + return root.intern(owner); } public String getForksUrl() { diff --git a/src/main/java/org/kohsuke/github/GHIssue.java b/src/main/java/org/kohsuke/github/GHIssue.java index 9a3b31f11..6b416ca1d 100644 --- a/src/main/java/org/kohsuke/github/GHIssue.java +++ b/src/main/java/org/kohsuke/github/GHIssue.java @@ -289,8 +289,7 @@ public class GHIssue extends GHObject implements Reactable{ } public GHUser getAssignee() throws IOException { - if (assignee != null) return owner.root.getUser(assignee.getLogin()); - return assignee; + return root.intern(assignee); } public List getAssignees() { @@ -301,7 +300,7 @@ public class GHIssue extends GHObject implements Reactable{ * User who submitted the issue. */ public GHUser getUser() throws IOException { - return owner.root.getUser(user.getLogin()); + return root.intern(user); } /** @@ -314,10 +313,14 @@ public class GHIssue extends GHObject implements Reactable{ */ public GHUser getClosedBy() throws IOException { if(!"closed".equals(state)) return null; - if(closed_by != null) return owner.root.getUser(closed_by.getLogin());; - - //TODO closed_by = owner.getIssue(number).getClosed_by(); - return closed_by; + + //TODO + /* + if (closed_by==null) { + closed_by = owner.getIssue(number).getClosed_by(); + } + */ + return root.intern(closed_by); } public int getCommentsCount(){ diff --git a/src/main/java/org/kohsuke/github/GHMilestone.java b/src/main/java/org/kohsuke/github/GHMilestone.java index 950dea257..50ad549e8 100644 --- a/src/main/java/org/kohsuke/github/GHMilestone.java +++ b/src/main/java/org/kohsuke/github/GHMilestone.java @@ -28,7 +28,7 @@ public class GHMilestone extends GHObject { } public GHUser getCreator() throws IOException { - return root.getUser(creator.getLogin()); + return root.intern(creator); } public Date getDueOn() { diff --git a/src/main/java/org/kohsuke/github/GitHub.java b/src/main/java/org/kohsuke/github/GitHub.java index bde74b126..201ccaff3 100644 --- a/src/main/java/org/kohsuke/github/GitHub.java +++ b/src/main/java/org/kohsuke/github/GitHub.java @@ -48,6 +48,7 @@ import java.util.Map; import java.util.Set; import java.util.TimeZone; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.logging.Logger; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; @@ -80,8 +81,8 @@ public class GitHub { */ /*package*/ final String encodedAuthorization; - private final Map users; - private final Map orgs; + private final ConcurrentMap users; + private final ConcurrentMap orgs; // Cache of myself object. private GHMyself myself; private final String apiUrl; @@ -647,6 +648,18 @@ public class GitHub { } } + /*package*/ GHUser intern(GHUser user) throws IOException { + if (user==null) return user; + + // if we already have this user in our map, use it + GHUser u = users.get(user.getLogin()); + if (u!=null) return u; + + // if not, remember this new user + users.putIfAbsent(user.getLogin(),user); + return user; + } + private static class GHApiInfo { private String rate_limit_url;