Skip to content

Commit d73f111

Browse files
dmbrysondmbryson
committed
[Core] Simplify task registration
Removes registerTask() method, takes ownership directly during the rule action call.
1 parent e20ed2c commit d73f111

File tree

13 files changed

+44
-88
lines changed

13 files changed

+44
-88
lines changed

‎bindings/python/llbuild.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ def _rule_create_task(context, engine_context):
9090
delegate.inputs_available = _task_inputs_available
9191

9292
task._task = libllbuild.llb_task_create(delegate[0])
93-
return libllbuild.llb_buildengine_register_task(engine._engine, task._task)
93+
return task._task
9494

9595
@ffi.callback("bool(void*, void*, llb_rule_t*, llb_data_t*)")
9696
def _rule_is_result_valid(context, engine_context, rule, value):

‎include/llbuild/Core/BuildEngine.h

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -356,14 +356,6 @@ class BuildEngine {
356356
/// @name Task Management APIs
357357
/// @{
358358

359-
/// Register the given task, in response to a Rule evaluation.
360-
///
361-
/// The engine tasks ownership of the \arg Task, and it is expected to
362-
/// subsequently be returned as the task to execute for a Rule evaluation.
363-
///
364-
/// \returns The provided task, for the convenience of the client.
365-
Task* registerTask(Task* task);
366-
367359
/// The maximum allowed input ID.
368360
static const uintptr_t kMaximumInputID = ~(uintptr_t)0xFF;
369361

‎lib/BuildSystem/BuildSystem.cpp

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1530,7 +1530,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
15301530
keyData,
15311531
/*signature=*/{},
15321532
/*Action=*/ [](BuildEngine& engine) -> Task* {
1533-
return engine.registerTask(new MissingCommandTask());
1533+
return new MissingCommandTask();
15341534
},
15351535
/*IsValid=*/ [](BuildEngine&, const Rule&, const ValueType&) -> bool {
15361536
// The cached result for a missing command is never valid.
@@ -1545,7 +1545,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
15451545
keyData,
15461546
command->getSignature(),
15471547
/*Action=*/ [command](BuildEngine& engine) -> Task* {
1548-
return engine.registerTask(new CommandTask(*command));
1548+
return new CommandTask(*command);
15491549
},
15501550
/*IsValid=*/ [command](BuildEngine& engine, const Rule& rule,
15511551
const ValueType& value) -> bool {
@@ -1578,7 +1578,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
15781578
keyData,
15791579
command->getSignature(),
15801580
/*Action=*/ [command](BuildEngine& engine) -> Task* {
1581-
return engine.registerTask(new CommandTask(*command));
1581+
return new CommandTask(*command);
15821582
},
15831583
/*IsValid=*/ [command](BuildEngine& engine, const Rule& rule,
15841584
const ValueType& value) -> bool {
@@ -1594,7 +1594,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
15941594
keyData,
15951595
/*signature=*/{},
15961596
/*Action=*/ [](BuildEngine& engine) -> Task* {
1597-
return engine.registerTask(new MissingCommandTask());
1597+
return new MissingCommandTask();
15981598
},
15991599
/*IsValid=*/ [](BuildEngine&, const Rule&, const ValueType&) -> bool {
16001600
// The cached result for a missing command is never valid.
@@ -1609,7 +1609,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
16091609
keyData,
16101610
/*signature=*/{},
16111611
/*Action=*/ [path](BuildEngine& engine) -> Task* {
1612-
return engine.registerTask(new DirectoryContentsTask(path));
1612+
return new DirectoryContentsTask(path);
16131613
},
16141614
/*IsValid=*/ [path](BuildEngine& engine, const Rule& rule,
16151615
const ValueType& value) mutable -> bool {
@@ -1627,8 +1627,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
16271627
/*signature=*/{},
16281628
/*Action=*/ [path, patterns](BuildEngine& engine) -> Task* {
16291629
BinaryDecoder decoder(patterns);
1630-
return engine.registerTask(new FilteredDirectoryContentsTask(path,
1631-
StringList(decoder)));
1630+
return new FilteredDirectoryContentsTask(path, StringList(decoder));
16321631
},
16331632
/*IsValid=*/ nullptr
16341633
};
@@ -1643,8 +1642,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
16431642
/*Action=*/ [path, filters](
16441643
BuildEngine& engine) mutable -> Task* {
16451644
BinaryDecoder decoder(filters);
1646-
return engine.registerTask(new DirectoryTreeSignatureTask(
1647-
path, StringList(decoder)));
1645+
return new DirectoryTreeSignatureTask(path, StringList(decoder));
16481646
},
16491647
// Directory signatures don't require any validation outside of their
16501648
// concrete dependencies.
@@ -1659,7 +1657,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
16591657
/*signature=*/{},
16601658
/*Action=*/ [path](
16611659
BuildEngine& engine) mutable -> Task* {
1662-
return engine.registerTask(new DirectoryTreeStructureSignatureTask(path));
1660+
return new DirectoryTreeStructureSignatureTask(path);
16631661
},
16641662
// Directory signatures don't require any validation outside of their
16651663
// concrete dependencies.
@@ -1700,7 +1698,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
17001698
keyData,
17011699
node->getSignature(),
17021700
/*Action=*/ [](BuildEngine& engine) -> Task* {
1703-
return engine.registerTask(new VirtualInputNodeTask());
1701+
return new VirtualInputNodeTask();
17041702
},
17051703
/*IsValid=*/ [node](BuildEngine& engine, const Rule& rule,
17061704
const ValueType& value) -> bool {
@@ -1715,7 +1713,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
17151713
keyData,
17161714
node->getSignature(),
17171715
/*Action=*/ [node](BuildEngine& engine) -> Task* {
1718-
return engine.registerTask(new DirectoryInputNodeTask(*node));
1716+
return new DirectoryInputNodeTask(*node);
17191717
},
17201718
// Directory nodes don't require any validation outside of their
17211719
// concrete dependencies.
@@ -1728,8 +1726,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
17281726
keyData,
17291727
node->getSignature(),
17301728
/*Action=*/ [node](BuildEngine& engine) -> Task* {
1731-
return engine.registerTask(
1732-
new DirectoryStructureInputNodeTask(*node));
1729+
return new DirectoryStructureInputNodeTask(*node);
17331730
},
17341731
// Directory nodes don't require any validation outside of their
17351732
// concrete dependencies.
@@ -1741,7 +1738,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
17411738
keyData,
17421739
node->getSignature(),
17431740
/*Action=*/ [node](BuildEngine& engine) -> Task* {
1744-
return engine.registerTask(new FileInputNodeTask(*node));
1741+
return new FileInputNodeTask(*node);
17451742
},
17461743
/*IsValid=*/ [node](BuildEngine& engine, const Rule& rule,
17471744
const ValueType& value) -> bool {
@@ -1756,7 +1753,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
17561753
keyData,
17571754
node->getSignature(),
17581755
/*Action=*/ [node](BuildEngine& engine) -> Task* {
1759-
return engine.registerTask(new ProducedNodeTask(*node));
1756+
return new ProducedNodeTask(*node);
17601757
},
17611758
/*IsValid=*/ [node](BuildEngine& engine, const Rule& rule,
17621759
const ValueType& value) -> bool {
@@ -1783,7 +1780,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
17831780
keyData,
17841781
/*signature=*/{},
17851782
/*Action=*/ [statnode](BuildEngine& engine) -> Task* {
1786-
return engine.registerTask(new StatTask(*statnode));
1783+
return new StatTask(*statnode);
17871784
},
17881785
/*IsValid=*/ [statnode](BuildEngine& engine, const Rule& rule,
17891786
const ValueType& value) -> bool {
@@ -1807,7 +1804,7 @@ Rule BuildSystemEngineDelegate::lookupRule(const KeyType& keyData) {
18071804
keyData,
18081805
/*signature=*/{},
18091806
/*Action=*/ [target](BuildEngine& engine) -> Task* {
1810-
return engine.registerTask(new TargetTask(*target));
1807+
return new TargetTask(*target);
18111808
},
18121809
/*IsValid=*/ [target](BuildEngine& engine, const Rule& rule,
18131810
const ValueType& value) -> bool {

‎lib/Commands/BuildEngineCommand.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,6 @@ struct AckermannTask : core::Task {
139139
AckermannValue recursiveResultB = {};
140140

141141
AckermannTask(core::BuildEngine& engine, int m, int n) : m(m), n(n) {
142-
engine.registerTask(this);
143142
}
144143

145144
/// Called when the task is started.

‎lib/Commands/NinjaBuildCommand.cpp

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1437,7 +1437,7 @@ buildCommand(BuildContext& context, ninja::Command* command) {
14371437
}
14381438
};
14391439

1440-
return context.engine.registerTask(new NinjaCommandTask(context, command));
1440+
return new NinjaCommandTask(context, command);
14411441
}
14421442

14431443
static core::Task* buildInput(BuildContext& context, ninja::Node* input) {
@@ -1471,7 +1471,7 @@ static core::Task* buildInput(BuildContext& context, ninja::Node* input) {
14711471
}
14721472
};
14731473

1474-
return context.engine.registerTask(new NinjaInputTask(context, input));
1474+
return new NinjaInputTask(context, input);
14751475
}
14761476

14771477
static core::Task*
@@ -1511,7 +1511,7 @@ buildTargets(BuildContext& context,
15111511
}
15121512
};
15131513

1514-
return context.engine.registerTask(new TargetsTask(context, targetsToBuild));
1514+
return new TargetsTask(context, targetsToBuild);
15151515
}
15161516

15171517
static core::Task*
@@ -1573,8 +1573,7 @@ selectCompositeBuildResult(BuildContext& context, ninja::Command* command,
15731573
}
15741574
};
15751575

1576-
return context.engine.registerTask(
1577-
new SelectResultTask(context, command, inputIndex, compositeRuleName));
1576+
return new SelectResultTask(context, command, inputIndex, compositeRuleName);
15781577
}
15791578

15801579
static bool buildInputIsResultValid(ninja::Node* node,

‎lib/Core/BuildEngine.cpp

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -516,9 +516,12 @@ class BuildEngineImpl : public BuildDBDelegate {
516516
Task* task = ruleInfo.rule.action(buildEngine);
517517
assert(task && "rule action returned null task");
518518

519-
// Find the task info for this task.
520-
auto taskInfo = getTaskInfo(task);
521-
assert(taskInfo && "rule action returned an unregistered task");
519+
// register the task
520+
taskInfosMutex.lock();
521+
auto result = taskInfos.emplace(task, TaskInfo(task));
522+
assert(result.second && "task already registered");
523+
auto taskInfo = &(result.first)->second;
524+
taskInfosMutex.unlock();
522525
taskInfo->forRuleInfo = &ruleInfo;
523526

524527
if (trace)
@@ -1583,16 +1586,6 @@ class BuildEngineImpl : public BuildDBDelegate {
15831586
/// @name Task Management Client APIs
15841587
/// @{
15851588

1586-
Task* registerTask(Task* task) {
1587-
{
1588-
std::lock_guard<std::mutex> guard(taskInfosMutex);
1589-
auto result = taskInfos.emplace(task, TaskInfo(task));
1590-
assert(result.second && "task already registered");
1591-
(void)result;
1592-
}
1593-
return task;
1594-
}
1595-
15961589
void taskNeedsInput(Task* task, const KeyType& key, uintptr_t inputID) {
15971590
// Validate the InputID.
15981591
if (inputID > BuildEngine::kMaximumInputID) {
@@ -1720,10 +1713,6 @@ bool BuildEngine::enableTracing(const std::string& path,
17201713
return static_cast<BuildEngineImpl*>(impl)->enableTracing(path, error_out);
17211714
}
17221715

1723-
Task* BuildEngine::registerTask(Task* task) {
1724-
return static_cast<BuildEngineImpl*>(impl)->registerTask(task);
1725-
}
1726-
17271716
void BuildEngine::taskNeedsInput(Task* task, const KeyType& key,
17281717
uintptr_t inputID) {
17291718
static_cast<BuildEngineImpl*>(impl)->taskNeedsInput(task, key, inputID);

‎perftests/Xcode/PerfTests/CorePerfTests.mm

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,7 @@ virtual void inputsAvailable(core::BuildEngine& Engine) override {
9191

9292
static ActionFn simpleAction(const std::vector<KeyType>& Inputs,
9393
SimpleTask::ComputeFnType Compute) {
94-
return [=] (BuildEngine& engine) {
95-
return engine.registerTask(new SimpleTask(Inputs, Compute)); };
94+
return [=] (BuildEngine& engine) { return new SimpleTask(Inputs, Compute); };
9695
}
9796

9897
}

‎products/libllbuild/Core-C-API.cpp

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -204,13 +204,6 @@ void llb_buildengine_build(llb_buildengine_t* engine_p, const llb_data_t* key,
204204
*result_out = llb_data_t{ result.size(), result.data() };
205205
}
206206

207-
llb_task_t* llb_buildengine_register_task(llb_buildengine_t* engine_p,
208-
llb_task_t* task) {
209-
auto& engine = ((CAPIBuildEngine*) engine_p)->engine;
210-
engine->registerTask((Task*)task);
211-
return task;
212-
}
213-
214207
void llb_buildengine_task_needs_input(llb_buildengine_t* engine_p,
215208
llb_task_t* task,
216209
const llb_data_t* key,

‎products/libllbuild/include/llbuild/core.h

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -154,15 +154,6 @@ LLBUILD_EXPORT void
154154
llb_buildengine_build(llb_buildengine_t* engine, const llb_data_t* key,
155155
llb_data_t* result_out);
156156

157-
/// Register the given task, in response to a Rule evaluation.
158-
///
159-
/// The engine tasks ownership of the \arg task, and it is expected to
160-
/// subsequently be returned as the task to execute for a rule evaluation.
161-
///
162-
/// \returns The provided task, for the convenience of the client.
163-
LLBUILD_EXPORT llb_task_t*
164-
llb_buildengine_register_task(llb_buildengine_t* engine, llb_task_t* task);
165-
166157
/// Specify the given \arg Task depends upon the result of computing \arg Key.
167158
///
168159
/// The result, when available, will be provided to the task via \see

‎products/llbuildSwift/CoreBindings.swift

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -521,12 +521,9 @@ public class BuildEngine {
521521
}
522522

523523
// Create the internal task.
524-
taskWrapper.taskInternal = llb_task_create(taskDelegate)
525-
526-
// FIXME: Why do we have both of these, it is kind of annoying. It makes
527-
// some amount of sense in the C++ API, but the C API should probably just
528-
// collapse them.
529-
return llb_buildengine_register_task(self._engine, taskWrapper.taskInternal)
524+
let lltask = llb_task_create(taskDelegate)
525+
taskWrapper.taskInternal = lltask
526+
return lltask!
530527
}
531528
}
532529

0 commit comments

Comments
 (0)