Skip to content

Commit 4602913

Browse files
ianhattendorfianhattendorf
authored
Merge pull request #1883 from AlexaXs/fix/LFS-checkout-performance-regression
UNSAFE Temporary workaround for LFS checkout performance regression
2 parents 011169f + caec819 commit 4602913

10 files changed

Lines changed: 489 additions & 20 deletions

File tree

‎generate/templates/manual/include/thread_pool.h‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,20 @@
88

99
#include "async_worker.h"
1010

11+
// Temporary workaround for LFS checkout. Comment added to be reverted.
12+
// With the threadpool rewrite, a Worker will execute its callbacks with
13+
// objects temporary unlock (to prevent deadlocks), and we'll wait until
14+
// the callback is done to lock them back again (to make sure it's thread-safe).
15+
// LFS checkout lost performance after this, and the proper way to fix it is
16+
// to integrate nodegit-lfs into nodegit. Until this is implemented, a
17+
// temporary workaround has been applied, which affects only Workers leveraging
18+
// threaded libgit2 functions (at the moment only checkout) and does the
19+
// following:
20+
// - do not wait for the current callback to end, so that it can send the
21+
// next callback to the main JS thread.
22+
// - do not temporary unlock the objects, since they would be locked back
23+
// again before the callback is executed.
24+
1125
namespace nodegit {
1226
class Context;
1327
class AsyncContextCleanupHandle;
@@ -17,7 +31,9 @@ namespace nodegit {
1731
public:
1832
typedef std::function<void()> Callback;
1933
typedef std::function<void(Callback, Callback)> QueueCallbackFn;
20-
typedef std::function<Callback(QueueCallbackFn, Callback)> OnPostCallbackFn;
34+
// Temporary workaround for LFS checkout. Code modified to be reverted.
35+
// typedef std::function<Callback(QueueCallbackFn, Callback)> OnPostCallbackFn;
36+
typedef std::function<Callback(QueueCallbackFn, Callback, bool)> OnPostCallbackFn;
2137

2238
// Initializes thread pool and spins up the requested number of threads
2339
// The provided loop will be used for completion callbacks, whenever

‎generate/templates/manual/src/async_baton.cc‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ namespace nodegit {
4646
ThreadPool::PostCallbackEvent(
4747
[jsCallback, cancelCallback](
4848
ThreadPool::QueueCallbackFn queueCallback,
49-
ThreadPool::Callback callbackCompleted
49+
ThreadPool::Callback callbackCompleted,
50+
bool isThreaded // Temporary workaround for LFS checkout. Code added to be reverted.
5051
) -> ThreadPool::Callback {
5152
queueCallback(jsCallback, cancelCallback);
5253
callbackCompleted();
@@ -58,13 +59,22 @@ namespace nodegit {
5859
ThreadPool::PostCallbackEvent(
5960
[this, jsCallback, cancelCallback](
6061
ThreadPool::QueueCallbackFn queueCallback,
61-
ThreadPool::Callback callbackCompleted
62+
ThreadPool::Callback callbackCompleted,
63+
bool isThreaded // Temporary workaround for LFS checkout. Code added to be reverted.
6264
) -> ThreadPool::Callback {
63-
this->onCompletion = callbackCompleted;
65+
// Temporary workaround for LFS checkout. Code modified to be reverted.
66+
if (!isThreaded) {
67+
this->onCompletion = callbackCompleted;
6468

65-
queueCallback(jsCallback, cancelCallback);
69+
queueCallback(jsCallback, cancelCallback);
6670

67-
return std::bind(&AsyncBaton::SignalCompletion, this);
71+
return std::bind(&AsyncBaton::SignalCompletion, this);
72+
}
73+
else {
74+
this->onCompletion = std::bind(&AsyncBaton::SignalCompletion, this);
75+
queueCallback(jsCallback, cancelCallback);
76+
return []() {};
77+
}
6878
}
6979
);
7080

‎generate/templates/manual/src/thread_pool.cc‎

Lines changed: 53 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <queue>
88
#include <thread>
99
#include <utility>
10+
#include <atomic> // Temporary workaround for LFS checkout. Code added to be reverted.
1011

1112
extern "C" {
1213
#include <git2/sys/custom_tls.h>
@@ -81,8 +82,11 @@ namespace nodegit {
8182
: Event(CALLBACK_TYPE), callback(initCallback)
8283
{}
8384

84-
ThreadPool::Callback operator()(ThreadPool::QueueCallbackFn queueCb, ThreadPool::Callback completedCb) {
85-
return callback(queueCb, completedCb);
85+
// Temporary workaround for LFS checkout. Code modified to be reverted.
86+
// ThreadPool::Callback operator()(ThreadPool::QueueCallbackFn queueCb, ThreadPool::Callback completedCb) {
87+
// return callback(queueCb, completedCb);
88+
ThreadPool::Callback operator()(ThreadPool::QueueCallbackFn queueCb, ThreadPool::Callback completedCb, bool isThreaded) {
89+
return callback(queueCb, completedCb, isThreaded);
8690
}
8791

8892
private:
@@ -102,6 +106,10 @@ namespace nodegit {
102106
// the Orchestrator's memory
103107
void WaitForThreadClose();
104108

109+
// Temporary workaround for LFS checkout. Code added to be reverted.
110+
// Returns true if the task running spawned threads within libgit2
111+
bool IsGitThreaded() { return currentGitThreads > kInitialGitThreads; }
112+
105113
static Nan::AsyncResource *GetCurrentAsyncResource();
106114

107115
static const nodegit::Context *GetCurrentContext();
@@ -139,6 +147,12 @@ namespace nodegit {
139147
PostCompletedEventToOrchestratorFn postCompletedEventToOrchestrator;
140148
TakeNextTaskFn takeNextTask;
141149
std::thread thread;
150+
151+
// Temporary workaround for LFS checkout. Code added to be reverted.
152+
static constexpr int kInitialGitThreads {0};
153+
// Number of threads spawned internally by libgit2 to deal with
154+
// the task of this Executor instance. Defaults to kInitialGitThreads.
155+
std::atomic<int> currentGitThreads {kInitialGitThreads};
142156
};
143157

144158
Executor::Executor(
@@ -170,6 +184,9 @@ namespace nodegit {
170184

171185
WorkTask *workTask = static_cast<WorkTask *>(task.get());
172186

187+
// Temporary workaround for LFS checkout. Code added to be reverted.
188+
currentGitThreads = kInitialGitThreads;
189+
173190
currentAsyncResource = workTask->asyncResource;
174191
currentCallbackErrorHandle = workTask->callbackErrorHandle;
175192
workTask->callback();
@@ -221,6 +238,8 @@ namespace nodegit {
221238
}
222239

223240
void *Executor::RetrieveTLSForLibgit2ChildThread() {
241+
// Temporary workaround for LFS checkout. Code added to be reverted.
242+
++Executor::executor->currentGitThreads;
224243
return Executor::executor;
225244
}
226245

@@ -230,6 +249,8 @@ namespace nodegit {
230249

231250
void Executor::TeardownTLSOnLibgit2ChildThread() {
232251
if (!isExecutorThread) {
252+
// Temporary workaround for LFS checkout. Code added to be reverted.
253+
--Executor::executor->currentGitThreads;
233254
Executor::executor = nullptr;
234255
}
235256
}
@@ -378,21 +399,46 @@ namespace nodegit {
378399
std::shared_ptr<std::condition_variable> callbackCondition(new std::condition_variable);
379400
bool hasCompleted = false;
380401

381-
LockMaster::TemporaryUnlock temporaryUnlock;
402+
// Temporary workaround for LFS checkout. Code removed to be reverted.
403+
//LockMaster::TemporaryUnlock temporaryUnlock;
404+
405+
// Temporary workaround for LFS checkout. Code added to be reverted.
406+
bool isWorkerThreaded = executor.IsGitThreaded();
407+
ThreadPool::Callback callbackCompleted = []() {};
408+
if (!isWorkerThreaded) {
409+
callbackCompleted = [callbackCondition, callbackMutex, &hasCompleted]() {
410+
std::lock_guard<std::mutex> lock(*callbackMutex);
411+
hasCompleted = true;
412+
callbackCondition->notify_one();
413+
};
414+
}
415+
std::unique_ptr<LockMaster::TemporaryUnlock> temporaryUnlock {nullptr};
416+
if (!isWorkerThreaded) {
417+
temporaryUnlock = std::make_unique<LockMaster::TemporaryUnlock>();
418+
}
419+
382420
auto onCompletedCallback = (*callbackEvent)(
383421
[this](ThreadPool::Callback callback, ThreadPool::Callback cancelCallback) {
384422
queueCallbackOnJSThread(callback, cancelCallback, false);
385423
},
424+
// Temporary workaround for LFS checkout. Code modified to be reverted.
425+
/*
386426
[callbackCondition, callbackMutex, &hasCompleted]() {
387427
std::lock_guard<std::mutex> lock(*callbackMutex);
388428
hasCompleted = true;
389429
callbackCondition->notify_one();
390430
}
431+
*/
432+
callbackCompleted,
433+
isWorkerThreaded
391434
);
392435

393-
std::unique_lock<std::mutex> lock(*callbackMutex);
394-
while (!hasCompleted) callbackCondition->wait(lock);
395-
onCompletedCallback();
436+
// Temporary workaround for LFS checkout. Code modified to be reverted.
437+
if (!isWorkerThreaded) {
438+
std::unique_lock<std::mutex> lock(*callbackMutex);
439+
while (!hasCompleted) callbackCondition->wait(lock);
440+
onCompletedCallback();
441+
}
396442
}
397443

398444
queueCallbackOnJSThread(
@@ -479,10 +525,6 @@ namespace nodegit {
479525

480526
void QueueCallbackOnJSThread(ThreadPool::Callback callback, ThreadPool::Callback cancelCallback, bool isWork);
481527

482-
static void RunJSThreadCallbacksFromOrchestrator(uv_async_t *handle);
483-
484-
void RunJSThreadCallbacksFromOrchestrator();
485-
486528
static void RunLoopCallbacks(uv_async_t *handle);
487529

488530
void Shutdown(std::unique_ptr<AsyncContextCleanupHandle> cleanupHandle);
@@ -498,7 +540,6 @@ namespace nodegit {
498540

499541
private:
500542
bool isMarkedForDeletion;
501-
nodegit::Context *currentContext;
502543

503544
struct JSThreadCallback {
504545
JSThreadCallback(ThreadPool::Callback callback, ThreadPool::Callback cancelCallback, bool isWork)
@@ -539,9 +580,9 @@ namespace nodegit {
539580
std::vector<Orchestrator> orchestrators;
540581
};
541582

583+
// context required to be passed to Orchestrators, but ThreadPoolImpl doesn't need to keep it
542584
ThreadPoolImpl::ThreadPoolImpl(int numberOfThreads, uv_loop_t *loop, nodegit::Context *context)
543585
: isMarkedForDeletion(false),
544-
currentContext(context),
545586
orchestratorJobMutex(new std::mutex),
546587
jsThreadCallbackMutex(new std::mutex)
547588
{

‎test/tests/filter.js‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,82 @@ describe("Filter", function() {
333333
});
334334
});
335335

336+
it("can run sync callback on checkout without deadlocking", function() { // jshint ignore:line
337+
var test = this;
338+
var syncCallbackResult = 1;
339+
340+
return Registry.register(filterName, {
341+
apply: function() {
342+
syncCallbackResult = test.repository.isEmpty();
343+
},
344+
check: function() {
345+
return NodeGit.Error.CODE.OK;
346+
}
347+
}, 0)
348+
.then(function(result) {
349+
assert.strictEqual(result, NodeGit.Error.CODE.OK);
350+
return fse.writeFile(
351+
packageJsonPath,
352+
"Changing content to trigger checkout",
353+
{ encoding: "utf-8" }
354+
);
355+
})
356+
.then(function() {
357+
var opts = {
358+
checkoutStrategy: Checkout.STRATEGY.FORCE,
359+
paths: "package.json"
360+
};
361+
return Checkout.head(test.repository, opts);
362+
})
363+
.then(function() {
364+
assert.strictEqual(syncCallbackResult, 0);
365+
});
366+
});
367+
368+
// Temporary workaround for LFS checkout. Test skipped.
369+
// To activate when reverting workaround.
370+
// 'Checkout.head' and 'Submodule.lookup' do work with the repo locked.
371+
// They should work together without deadlocking.
372+
it.skip("can run async callback on checkout without deadlocking", function() { // jshint ignore:line
373+
var test = this;
374+
var submoduleNameIn = "vendor/libgit2";
375+
var asyncCallbackResult = "";
376+
377+
return Registry.register(filterName, {
378+
apply: function() {
379+
return NodeGit.Submodule.lookup(test.repository, submoduleNameIn)
380+
.then(function(submodule) {
381+
return submodule.name();
382+
})
383+
.then(function(name) {
384+
asyncCallbackResult = name;
385+
return NodeGit.Error.CODE.OK;
386+
});
387+
},
388+
check: function() {
389+
return NodeGit.Error.CODE.OK;
390+
}
391+
}, 0)
392+
.then(function(result) {
393+
assert.strictEqual(result, NodeGit.Error.CODE.OK);
394+
return fse.writeFile(
395+
packageJsonPath,
396+
"Changing content to trigger checkout",
397+
{ encoding: "utf-8" }
398+
);
399+
})
400+
.then(function() {
401+
var opts = {
402+
checkoutStrategy: Checkout.STRATEGY.FORCE,
403+
paths: "package.json"
404+
};
405+
return Checkout.head(test.repository, opts);
406+
})
407+
.then(function() {
408+
assert.equal(asyncCallbackResult, submoduleNameIn);
409+
});
410+
});
411+
336412
// this test is useless on 32 bit CI, because we cannot construct
337413
// a buffer big enough to test anything of significance :)...
338414
if (process.arch === "x64") {

‎test/tests/submodule.js‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,4 +157,38 @@ describe("Submodule", function() {
157157
assert.equal(entries[1].path, submodulePath);
158158
});
159159
});
160+
161+
it("can run sync callback without deadlocking", function() {
162+
var repo = this.workdirRepository;
163+
var submodules = [];
164+
var submoduleCallback = function(submodule, name, payload) {
165+
var submoduleName = submodule.name();
166+
assert.equal(submoduleName, name);
167+
submodules.push(name);
168+
};
169+
170+
return Submodule.foreach(repo, submoduleCallback).then(function() {
171+
assert.equal(submodules.length, 1);
172+
});
173+
});
174+
175+
// 'Submodule.foreach' and 'Submodule.lookup' do work with the repo locked.
176+
// They should work together without deadlocking.
177+
it("can run async callback without deadlocking", function() {
178+
var repo = this.workdirRepository;
179+
var submodules = [];
180+
var submoduleCallback = function(submodule, name, payload) {
181+
var owner = submodule.owner();
182+
183+
return Submodule.lookup(owner, name)
184+
.then(function(submodule) {
185+
assert.equal(submodule.name(), name);
186+
submodules.push(name);
187+
});
188+
};
189+
190+
return Submodule.foreach(repo, submoduleCallback).then(function() {
191+
assert.equal(submodules.length, 1);
192+
});
193+
});
160194
});

0 commit comments

Comments
 (0)