Demon has submitted this change and it was merged.
Change subject: Performance (& security?) fixes in AdvertiseRefsHookChain
......................................................................
Performance (& security?) fixes in AdvertiseRefsHookChain
Since BaseReceivePack.setAdvertisedRefs() only allows
us to shrink the number of refs and not the advertisedHaves
(objects advertised to the client) a few big issues arises.
First, let's look at ReceiveCommits (filtered out some stuff)
where we build the chain:
if (!projectControl.allRefsAreVisible()) {
rp.setAdvertiseRefsHook(new VisibleRefFilter());
}
List<AdvertiseRefsHook> advHooks = new ArrayList<AdvertiseRefsHook>(3);
advHooks.add(new AdvertiseRefsHook() {
public void advertiseRefs(BaseReceivePack rp)
throws ServiceMayNotContinueException {
allRefs = rp.getAdvertisedRefs();
if (allRefs == null) {
allRefs = rp.getRepository().getAllRefs();
}
rp.setAdvertisedRefs(allRefs, rp.getAdvertisedObjects());
}
});
advHooks.add(rp.getAdvertiseRefsHook());
advHooks.add(new ReceiveCommitsAdvertiseRefsHook());
rp.setAdvertiseRefsHook(AdvertiseRefsHookChain.newChain(advHooks));
All these three AdvertiseRefsHooks will contribute objects
to BaseReceivePack.advertisedHaves through rp.setAdvertisedRefs().
While we filter down our refs, the objects will stay in
BaseReceivePack.advertisedHaves; generating a massive amount of objects
to be marked as "uninteresting" in checkConnectivity(). With the slow
pending queue in the RevWalk, JGit does not scale very well with these
amounts of objects in advertisedHaves.
Part of setAdvertisedRefs() for reference:
refs = allRefs != null ? allRefs : db.getAllRefs();
refs = refFilter.filter(refs);
[...]
for (Ref ref : refs.values()) {
if (ref.getObjectId() != null)
advertisedHaves.add(ref.getObjectId());
}
if (additionalHaves != null)
advertisedHaves.addAll(additionalHaves);
else
advertisedHaves.addAll(db.getAdditionalHaves());
By just pre-filtering the refs and calling setAdvertisedRefs()
just once, we could cut the push time down to 1/3 of what we have
today.
This is the difference between push times when applying this
patch (+ warm caches):
Before patch: 0m41.476s
After patch: 0m13.833s
I'm also wondering if the excessive amount of objects in
advertisedHaves means it can be possible to reference
commits that is not supposed to be seen or reached by the user.
Change-Id: Ice158efbc03d6e80576ad87f499777b7381bd0fa
---
M gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
M
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsAdvertiseRefsHook.java
M gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
3 files changed, 27 insertions(+), 11 deletions(-)
Approvals:
Demon: Verified; Looks good to me, approved
diff --git
a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
index 48f903d..9c1fef8 100644
---
a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
+++
b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -378,12 +378,16 @@
rp.setAllowDeletes(true);
rp.setAllowNonFastForwards(true);
rp.setCheckReceivedObjects(true);
+ Map<String, Ref> refsToFilter = null;
if (!projectControl.allRefsAreVisible()) {
rp.setCheckReferencedObjectsAreReachable(config.checkReferencedObjectsAreReachable);
- rp.setAdvertiseRefsHook(new VisibleRefFilter(tagCache, changeCache,
repo, projectControl, db, false));
+ refsToFilter =
+ new VisibleRefFilter(tagCache, changeCache, repo, projectControl, db,
+ false).filter(repo.getAllRefs());
}
- List<AdvertiseRefsHook> advHooks = new ArrayList<AdvertiseRefsHook>(3);
+ List<AdvertiseRefsHook> advHooks = new ArrayList<AdvertiseRefsHook>(2);
+
advHooks.add(new AdvertiseRefsHook() {
@Override
public void advertiseRefs(BaseReceivePack rp) {
@@ -391,15 +395,14 @@
if (allRefs == null) {
allRefs = rp.getRepository().getAllRefs();
}
- rp.setAdvertisedRefs(allRefs, rp.getAdvertisedObjects());
}
-
@Override
public void advertiseRefs(UploadPack uploadPack) {
}
});
+
advHooks.add(rp.getAdvertiseRefsHook());
- advHooks.add(new ReceiveCommitsAdvertiseRefsHook());
+ advHooks.add(new ReceiveCommitsAdvertiseRefsHook(refsToFilter));
rp.setAdvertiseRefsHook(AdvertiseRefsHookChain.newChain(advHooks));
}
diff --git
a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsAdvertiseRefsHook.java
b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsAdvertiseRefsHook.java
index 0eb9b61..68ddb24 100644
---
a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsAdvertiseRefsHook.java
+++
b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommitsAdvertiseRefsHook.java
@@ -22,10 +22,17 @@
import org.eclipse.jgit.transport.BaseReceivePack;
import org.eclipse.jgit.transport.UploadPack;
+import java.util.HashMap;
import java.util.Map;
/** Exposes only the non refs/changes/ reference names. */
public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook {
+ Map<String, Ref> baseRefs = null;
+
+ public ReceiveCommitsAdvertiseRefsHook(Map<String, Ref> refs) {
+ baseRefs = refs;
+ }
+
@Override
public void advertiseRefs(UploadPack us) {
throw new UnsupportedOperationException(
@@ -34,12 +41,18 @@
@Override
public void advertiseRefs(BaseReceivePack rp) {
- Map<String, Ref> oldRefs = rp.getAdvertisedRefs();
- if (oldRefs == null) {
- oldRefs = rp.getRepository().getAllRefs();
+ Map<String, Ref> refs = rp.getAdvertisedRefs();
+ if (baseRefs != null) {
+ if (refs == null) {
+ refs = baseRefs;
+ } else {
+ refs.putAll(baseRefs);
+ }
+ } else if (refs == null) {
+ refs = rp.getRepository().getAllRefs();
}
- Map<String, Ref> r = Maps.newHashMapWithExpectedSize(oldRefs.size());
- for (Map.Entry<String, Ref> e : oldRefs.entrySet()) {
+ Map<String, Ref> r = Maps.newHashMapWithExpectedSize(refs.size());
+ for (Map.Entry<String, Ref> e : refs.entrySet()) {
String name = e.getKey();
if (!skip(name)) {
r.put(name, e.getValue());
diff --git
a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
index 5cd0def..a8b84e3 100644
---
a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
+++
b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
@@ -125,7 +125,7 @@
return filter(repository.getAllRefs());
}
- private Map<String, Ref> filter(Map<String, Ref> refs) {
+ public Map<String, Ref> filter(Map<String, Ref> refs) {
return filter(refs, false);
}
--
To view, visit https://gerrit.wikimedia.org/r/64311
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ice158efbc03d6e80576ad87f499777b7381bd0fa
Gerrit-PatchSet: 2
Gerrit-Project: gerrit
Gerrit-Branch: wmf
Gerrit-Owner: QChris <[email protected]>
Gerrit-Reviewer: Demon <[email protected]>
Gerrit-Reviewer: QChris <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
