Skip to content

Commit

Permalink
Merge pull request #143 from rime/bugfix
Browse files Browse the repository at this point in the history 
Fixes #141: enforce dependency priorities in config_compiler
  • Loading branch information
@lotem
lotem authored Sep 4, 2017
2 parents 632cf4b + d503513 commit e3389ff
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 39 deletions.
10 changes: 10 additions & 0 deletions data/test/config_compiler_test.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ dependency_chaining:
__include: /dependency_chaining/beta
epsilon: success

dependency_priorities:
terrans:
__include: /starcraft/terrans
__patch:
player: nada
protoss:
__patch:
player: bisu
__include: /starcraft/protoss

local:
patch:
battlefields/@next: match point
Expand Down
129 changes: 91 additions & 38 deletions src/rime/config/config_compiler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,36 +7,61 @@

namespace rime {

enum DependencyPriority {
kPendingChild = 0,
kInclude = 1,
kPatch = 2,
};

struct Dependency {
an<ConfigItemRef> target;

virtual bool blocking() const = 0;
virtual DependencyPriority priority() const = 0;
bool blocking() const {
return priority() > kPendingChild;
}
virtual string repr() const = 0;
virtual bool Resolve(ConfigCompiler* compiler) = 0;
};

template <class StreamT>
StreamT& operator<< (StreamT& stream, const Dependency& dep) {
return stream << dep.repr();
}

struct PendingChild : Dependency {
string child_path;
an<ConfigItemRef> child_ref;

PendingChild(const string& path, const an<ConfigItemRef>& ref)
: child_path(path), child_ref(ref) {
}
bool blocking() const override {
return false;
DependencyPriority priority() const override {
return kPendingChild;
}
string repr() const override {
return "PendingChild(" + child_path + ")";
}
bool Resolve(ConfigCompiler* compiler) override;
};

string Reference::repr() const {
return resource_id + ":" + local_path;
}

template <class StreamT>
StreamT& operator<< (StreamT& stream, const Reference& reference) {
return stream << reference.resource_id << ":" << reference.local_path;
return stream << reference.repr();
}

struct IncludeReference : Dependency {
IncludeReference(const Reference& r) : reference(r) {
}
bool blocking() const override {
return true;
DependencyPriority priority() const override {
return kInclude;
}
string repr() const override {
return "Include(" + reference.repr() + ")";
}
bool Resolve(ConfigCompiler* compiler) override;

Expand All @@ -46,8 +71,11 @@ struct IncludeReference : Dependency {
struct PatchReference : Dependency {
PatchReference(const Reference& r) : reference(r) {
}
bool blocking() const override {
return true;
DependencyPriority priority() const override {
return kPatch;
}
string repr() const override {
return "Patch(" + reference.repr() + ")";
}
bool Resolve(ConfigCompiler* compiler) override;

Expand All @@ -59,8 +87,11 @@ struct PatchLiteral : Dependency {

PatchLiteral(an<ConfigMap> map) : patch(map) {
}
bool blocking() const override {
return true;
DependencyPriority priority() const override {
return kPatch;
}
string repr() const override {
return "Patch(<literal>)";
}
bool Resolve(ConfigCompiler* compiler) override;
};
Expand All @@ -69,7 +100,7 @@ struct ConfigDependencyGraph {
map<string, of<ConfigResource>> resources;
vector<of<ConfigItemRef>> node_stack;
vector<string> key_stack;
map<string, list<of<Dependency>>> deps;
map<string, vector<of<Dependency>>> deps;

void Add(an<Dependency> dependency);

Expand Down Expand Up @@ -97,7 +128,7 @@ static an<ConfigItem> ResolveReference(ConfigCompiler* compiler,
const Reference& reference);

bool IncludeReference::Resolve(ConfigCompiler* compiler) {
LOG(INFO) << "IncludeReference::Resolve(reference = " << reference << ")";
DLOG(INFO) << "IncludeReference::Resolve(reference = " << reference << ")";
auto item = ResolveReference(compiler, reference);
if (!item) {
return false;
Expand All @@ -107,6 +138,7 @@ bool IncludeReference::Resolve(ConfigCompiler* compiler) {
}

bool PatchReference::Resolve(ConfigCompiler* compiler) {
DLOG(INFO) << "PatchReference::Resolve(reference = " << reference << ")";
auto item = ResolveReference(compiler, reference);
if (!item) {
return false;
Expand All @@ -125,10 +157,12 @@ bool TraverseCopyOnWrite(an<ConfigItemRef> root, const string& path,
an<ConfigItem> item);

bool PatchLiteral::Resolve(ConfigCompiler* compiler) {
DLOG(INFO) << "PatchLiteral::Resolve()";
bool success = true;
for (const auto& entry : *patch) {
const auto& path = entry.first;
const auto& value = entry.second;
LOG(INFO) << "patching " << path;
if (!TraverseCopyOnWrite(target, path, value)) {
LOG(ERROR) << "error applying patch to " << path;
success = false;
Expand All @@ -137,31 +171,50 @@ bool PatchLiteral::Resolve(ConfigCompiler* compiler) {
return success;
}

static void InsertByPriority(vector<of<Dependency>>& list,
const an<Dependency>& value) {
auto upper = std::upper_bound(
list.begin(), list.end(), value,
[](const an<Dependency>& lhs, const an<Dependency>& rhs) {
return lhs->priority() < rhs->priority();
});
list.insert(upper, value);
}

void ConfigDependencyGraph::Add(an<Dependency> dependency) {
LOG(INFO) << "ConfigDependencyGraph::Add(), node_stack.size() = " << node_stack.size();
DLOG(INFO) << "ConfigDependencyGraph::Add(), node_stack.size() = "
<< node_stack.size();
if (node_stack.empty()) return;
const auto& target = node_stack.back();
dependency->target = target;
auto target_path = ConfigData::JoinPath(key_stack);
deps[target_path].push_back(dependency);
LOG(INFO) << "target_path = " << target_path << ", #deps = " << deps[target_path].size();
// The current pending node becomes a prioritized dependency of parent node
auto child = target;
auto& target_deps = deps[target_path];
bool target_was_pending = !target_deps.empty();
InsertByPriority(target_deps, dependency);
DLOG(INFO) << "target_path = " << target_path
<< ", #deps = " << target_deps.size();
if (target_was_pending || // so was all ancestors
key_stack.size() == 1) { // this is the progenitor
return;
}
// The current pending node becomes a prioritized non-blocking dependency of
// its parent node; spread the pending state to its non-pending ancestors
auto keys = key_stack;
for (auto prev = node_stack.rbegin() + 1; prev != node_stack.rend(); ++prev) {
for (auto child = node_stack.rbegin(); child != node_stack.rend(); ++child) {
auto last_key = keys.back();
keys.pop_back();
auto parent_path = ConfigData::JoinPath(keys);
auto& parent_deps = deps[parent_path];
bool parent_was_pending = !parent_deps.empty();
// Pending children should be resolved before applying __include or __patch
parent_deps.push_front(New<PendingChild>(parent_path + "/" + last_key, child));
LOG(INFO) << "parent_path = " << parent_path << ", #deps = " << parent_deps.size();
if (parent_was_pending) {
// so was all ancestors
break;
InsertByPriority(parent_deps,
New<PendingChild>(parent_path + "/" + last_key, *child));
DLOG(INFO) << "parent_path = " << parent_path
<< ", #deps = " << parent_deps.size();
if (parent_was_pending || // so was all ancestors
keys.size() == 1) { // this parent is the progenitor
return;
}
child = *prev;
}
}

Expand Down Expand Up @@ -233,9 +286,9 @@ static bool ResolveBlockingDependencies(ConfigCompiler* compiler,
if (!compiler->blocking(path)) {
return true;
}
LOG(INFO) << "blocking node: " << path;
DLOG(INFO) << "blocking node: " << path;
if (compiler->ResolveDependencies(path)) {
LOG(INFO) << "resolved blocking node:" << path;
DLOG(INFO) << "resolved blocking node:" << path;
return true;
}
return false;
Expand All @@ -244,7 +297,7 @@ static bool ResolveBlockingDependencies(ConfigCompiler* compiler,
static an<ConfigItem> GetResolvedItem(ConfigCompiler* compiler,
an<ConfigResource> resource,
const string& path) {
LOG(INFO) << "GetResolvedItem(" << resource->resource_id << ":/" << path << ")";
DLOG(INFO) << "GetResolvedItem(" << resource->resource_id << ":" << path << ")";
string node_path = resource->resource_id + ":";
if (!resource || compiler->blocking(node_path)) {
return nullptr;
Expand All @@ -267,7 +320,7 @@ static an<ConfigItem> GetResolvedItem(ConfigCompiler* compiler,
result.reset();
}
} else if (Is<ConfigMap>(result)) {
LOG(INFO) << "advance with key: " << key;
DLOG(INFO) << "advance with key: " << key;
(node_path += "/") += key;
if (!ResolveBlockingDependencies(compiler, node_path)) {
return nullptr;
Expand Down Expand Up @@ -303,7 +356,7 @@ static an<ConfigItem> ResolveReference(ConfigCompiler* compiler,
const Reference& reference) {
auto resource = compiler->GetCompiledResource(reference.resource_id);
if (!resource) {
LOG(INFO) << "resource not found, compiling: " << reference.resource_id;
LOG(INFO) << "resource not loaded, compiling: " << reference.resource_id;
resource = compiler->Compile(reference.resource_id);
}
return GetResolvedItem(compiler, resource, reference.local_path);
Expand All @@ -316,7 +369,7 @@ static bool ParseInclude(ConfigCompiler* compiler,
const an<ConfigItem>& item) {
if (Is<ConfigValue>(item)) {
auto path = As<ConfigValue>(item)->str();
LOG(INFO) << "ParseInclude(" << path << ")";
DLOG(INFO) << "ParseInclude(" << path << ")";
compiler->AddDependency(
New<IncludeReference>(compiler->CreateReference(path)));
return true;
Expand Down Expand Up @@ -350,21 +403,21 @@ static bool ParsePatch(ConfigCompiler* compiler,
const an<ConfigItem>& item) {
if (Is<ConfigValue>(item)) {
auto path = As<ConfigValue>(item)->str();
LOG(INFO) << "ParsePatch(" << path << ")";
DLOG(INFO) << "ParsePatch(" << path << ")";
compiler->AddDependency(
New<PatchReference>(compiler->CreateReference(path)));
return true;
}
if (Is<ConfigMap>(item)) {
LOG(INFO) << "ParsePatch(<literal>)";
DLOG(INFO) << "ParsePatch(<literal>)";
compiler->AddDependency(New<PatchLiteral>(As<ConfigMap>(item)));
return true;
}
return false;
}

bool ConfigCompiler::Parse(const string& key, const an<ConfigItem>& item) {
LOG(INFO) << "ConfigCompiler::Parse(" << key << ")";
DLOG(INFO) << "ConfigCompiler::Parse(" << key << ")";
if (key == INCLUDE_DIRECTIVE) {
return ParseInclude(this, item);
}
Expand All @@ -375,24 +428,24 @@ bool ConfigCompiler::Parse(const string& key, const an<ConfigItem>& item) {
}

bool ConfigCompiler::Link(an<ConfigResource> target) {
LOG(INFO) << "Link(" << target->resource_id << ")";
DLOG(INFO) << "Link(" << target->resource_id << ")";
auto found = graph_->resources.find(target->resource_id);
if (found == graph_->resources.end()) {
LOG(INFO) << "resource not found: " << target->resource_id;
LOG(ERROR) << "resource not found: " << target->resource_id;
return false;
}
return ResolveDependencies(found->first + ":");
}

bool ConfigCompiler::ResolveDependencies(const string& path) {
LOG(INFO) << "ResolveDependencies(" << path << ")";
DLOG(INFO) << "ResolveDependencies(" << path << ")";
auto& deps = graph_->deps[path];
for (auto iter = deps.begin(); iter != deps.end(); ) {
if (!(*iter)->Resolve(this)) {
LOG(INFO) << "unesolved dependency!";
LOG(ERROR) << "unresolved dependency: " << **iter;
return false;
}
LOG(INFO) << "resolved.";
LOG(INFO) << "resolved: " << **iter;
iter = deps.erase(iter);
}
LOG(INFO) << "all dependencies resolved.";
Expand Down
2 changes: 2 additions & 0 deletions src/rime/config/config_compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ struct ConfigResource : ConfigItemRef {
struct Reference {
string resource_id;
string local_path;

string repr() const;
};

class ResourceResolver;
Expand Down
3 changes: 2 additions & 1 deletion src/rime/config/config_data.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class ConfigDataRootRef : public ConfigItemRef {
template <class T>
static an<T> Cow(const an<T>& container, const string& key) {
if (!container) {
LOG(INFO) << "creating node: " << key;
DLOG(INFO) << "creating node: " << key;
return New<T>();
}
DLOG(INFO) << "copy on write: " << key;
Expand Down Expand Up @@ -207,6 +207,7 @@ class ConfigListEntryCowRef : public ConfigMapEntryCowRef {

bool TraverseCopyOnWrite(an<ConfigItemRef> root, const string& path,
an<ConfigItem> item) {
DLOG(INFO) << "TraverseCopyOnWrite(" << path << ")";
if (path.empty() || path == "/") {
*root = item;
return true;
Expand Down
14 changes: 14 additions & 0 deletions test/config_compiler_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,18 @@ TEST_F(RimeConfigCompilerTest, DependencyChaining) {
EXPECT_EQ("success", value);
}

// Unit test for https://github.com/rime/librime/issues/141
TEST_F(RimeConfigCompilerTest, DependencyPriorities) {
const string& prefix = "dependency_priorities/";
EXPECT_TRUE(config_->IsNull(prefix + "terrans/__include"));
EXPECT_TRUE(config_->IsNull(prefix + "terrans/__patch"));
EXPECT_TRUE(config_->IsNull(prefix + "protoss/__include"));
EXPECT_TRUE(config_->IsNull(prefix + "protoss/__patch"));
string player;
EXPECT_TRUE(config_->GetString(prefix + "terrans/player", &player));
EXPECT_EQ("nada", player);
EXPECT_TRUE(config_->GetString(prefix + "protoss/player", &player));
EXPECT_EQ("bisu", player);
}

// TODO: test failure cases

0 comments on commit e3389ff

Please sign in to comment.