Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-12763] Reduce lock contention while updating caches #21

Merged
merged 4 commits into from Jun 15, 2012
Merged

[JENKINS-12763] Reduce lock contention while updating caches #21

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eed5978
Reduce lock contention while updating caches
nimeacuerdo Jan 16, 2012
7cf5ae7
Traces updated
nimeacuerdo Jan 17, 2012
b8b61ff
Renaming of local bundle files and hg branch parameter
nimeacuerdo May 2, 2012
af0eb07
Changes towards the smallest diff for review
nimeacuerdo May 22, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
69 changes: 56 additions & 13 deletions src/main/java/hudson/plugins/mercurial/Cache.java
View file
Open in desktop
Expand Up @@ -43,7 +43,8 @@ class Cache {
/**
* Mutual exclusion to the access to the cache.
*/
private final ReentrantLock lock = new ReentrantLock(true);
private final ReentrantLock masterLock = new ReentrantLock(true);
private final Map<String, ReentrantLock> slaveNodesLocksMap = new HashMap<String, ReentrantLock>();

private Cache(String remote, String hash) {
this.remote = remote;
Expand All @@ -61,6 +62,21 @@ private Cache(String remote, String hash) {
return cache;
}

/**
* Gets a lock for the given slave node.
* @param node Name of the slave node.
* @return The {@link ReentrantLock} instance.
*/
private synchronized ReentrantLock getLockForSlaveNode(String node) {
ReentrantLock lock = slaveNodesLocksMap.get(node);
if (lock == null) {
slaveNodesLocksMap.put(node, lock = new ReentrantLock(true));
}

return lock;
}


/**
* Returns a local hg repository cache of the remote repository specified in the given {@link MercurialSCM}
* on the given {@link Node}, fully updated to the tip of the current remote repository.
Expand All @@ -73,16 +89,11 @@ private Cache(String remote, String hash) {
*/
@CheckForNull FilePath repositoryCache(MercurialSCM config, Node node, Launcher launcher, TaskListener listener, boolean fromPolling)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to make the diff not touch any more lines than it absolutely has to, so it can be more easily reviewed and is less likely to conflict with other changes. I think you are only really changing a couple things here but it is hard to see that. (OK for indentation to not quite match brace structure.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been 4 months since I pushed this, so I will try to sum up the whole idea again in a single additional comment in this review. It's not that easy to analyze it by looking at some lines on isolation, as basically the change from the single lock scheme to a double one changes many parts of the affected method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But looking at the diff, it seems like a lot of lines changed only in indentation, i.e. you are wrapping some areas of code in try-blocks differently. It would just be easier to see what those changes are if they were not mixed up with unnecessary whitespace changes. Also if there is ever a need to examine history of those interior lines, annotate is going to be useless if their indentation changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in indentation is a consequence of the actual change in the way the master lock is handled: operations that in the original implementation were performed after obtaining the only lock used are now safely performed before obtaining the lock for the master. That is why they refer to master* variables and are placed before the try block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that the block nesting changes as part of the change in behavior. What I am requesting is that indentation of code lines be left untouched wherever possible, even if that no longer matches block nesting. In other words

-doOneThing();
-doAnotherThing();
+lock();
+try {
+    doOneThing();
+    doAnotherThing();
+} finally {
+    unlock();
+}

should be avoided in favor of the minimized diff

+lock();
+try {
 doOneThing();
 doAnotherThing();
+} finally {
+    unlock();
+}

which merges better, annotates better, and is reviewed more readily (since it is obvious where the interior block was modified if at all). Ideally SCMs like Git would understand block structure in programming languages and behave more intelligently, but as a rule they do not.

throws IOException, InterruptedException {
boolean wasLocked = lock.isLocked();
if (wasLocked) {
listener.getLogger().println("Waiting for lock on hgcache/" + hash + " " + lock + "...");
boolean masterWasLocked = masterLock.isLocked();
if (masterWasLocked) {
listener.getLogger().println("Waiting for master lock on hgcache/" + hash + " " + masterLock + "...");
}

lock.lockInterruptibly();
try {
if (wasLocked) {
listener.getLogger().println("...acquired cache lock.");
}
// Always update master cache first.
Node master = Hudson.getInstance();
FilePath masterCaches = master.getRootPath().child("hgcache");
Expand All @@ -93,6 +104,12 @@ private Cache(String remote, String hash) {
// do we need to pass in EnvVars from a build too?
HgExe masterHg = new HgExe(config,masterLauncher,master,listener,new EnvVars());

// Lock the block used to verify we end up having a cloned repo in the master,
// whether if it was previously cloned in a different build or if it's
// going to be cloned right now.
masterLock.lockInterruptibly();
try {
listener.getLogger().println("Acquired master cache lock.");
if (masterCache.isDirectory()) {
if (MercurialSCM.joinWithPossibleTimeout(masterHg.pull().pwd(masterCache), true, listener) != 0) {
listener.error("Failed to update " + masterCache);
Expand All @@ -105,13 +122,38 @@ private Cache(String remote, String hash) {
return null;
}
}
} finally {
masterLock.unlock();
listener.getLogger().println("Master cache lock released.");
}
if (node == master) {
return masterCache;
}
// Not on master, so need to create/update local cache as well.

// We are in a slave node that will need also an updated local cache: clone it or
// pull pending changes, if any. This can be safely done in parallel in
// different slave nodes for a given repo, so we'll use different
// node-specific locks to achieve this.
ReentrantLock slaveNodeLock = getLockForSlaveNode(node.getNodeName());

boolean slaveNodeWasLocked = slaveNodeLock.isLocked();
if (slaveNodeWasLocked) {
listener.getLogger().println("Waiting for slave node cache lock in " + node.getNodeName() + " on hgcache/" + hash + " " + slaveNodeWasLocked + "...");
}

slaveNodeLock.lockInterruptibly();
try {
listener.getLogger().println("Acquired slave node cache lock for node " + node.getNodeName() + ".");

FilePath localCaches = node.getRootPath().child("hgcache");
FilePath localCache = localCaches.child(hash);
FilePath masterTransfer = masterCache.child("xfer.hg");

// Bundle name is node-specific, as we may have more than one
// node being updated in parallel, and each one will use its own
// bundle.
String bundleFileName = "xfer-" + node.getNodeName() + ".hg";
FilePath masterTransfer = masterCache.child(bundleFileName);
FilePath localTransfer = localCache.child("xfer.hg");
try {
// hg invocation on the slave
Expand All @@ -131,15 +173,15 @@ private Cache(String remote, String hash) {
// to actually exclude those head sets, but not a big deal. (Hg 1.5 fixes that but leaves
// a major bug that if no csets are selected, the whole repo will be bundled; fortunately
// this case should be caught by equality check above.)
if (MercurialSCM.joinWithPossibleTimeout(masterHg.bundle(localHeads,"xfer.hg").
if (MercurialSCM.joinWithPossibleTimeout(masterHg.bundle(localHeads,bundleFileName).
pwd(masterCache), fromPolling, listener) != 0) {
listener.error("Failed to send outgoing changes");
return null;
}
}
} else {
// Need to transfer entire repo.
if (MercurialSCM.joinWithPossibleTimeout(masterHg.bundleAll("xfer.hg").pwd(masterCache), fromPolling, listener) != 0) {
if (MercurialSCM.joinWithPossibleTimeout(masterHg.bundleAll(bundleFileName).pwd(masterCache), fromPolling, listener) != 0) {
listener.error("Failed to bundle repo");
return null;
}
Expand All @@ -162,7 +204,8 @@ private Cache(String remote, String hash) {
}
return localCache;
} finally {
lock.unlock();
slaveNodeLock.unlock();
listener.getLogger().println("Slave node cache lock released for node " + node.getNodeName() + ".");
}
}

Expand Down