From 25c28f8d3e2a6ee6a2f8587407b6321ae1876f20 Mon Sep 17 00:00:00 2001 From: Chen Gong Date: Sat, 2 Sep 2017 22:50:32 +0800 Subject: [PATCH 1/3] fix(config_compiler): duplicate PendingChild dependencies happen from multiple commands on the same node --- src/rime/config/config_compiler.cc | 57 ++++++++++++++++++++++-------- src/rime/config/config_compiler.h | 2 ++ src/rime/config/config_data.cc | 1 + 3 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/rime/config/config_compiler.cc b/src/rime/config/config_compiler.cc index f2b2830de..8f98c1cd2 100644 --- a/src/rime/config/config_compiler.cc +++ b/src/rime/config/config_compiler.cc @@ -11,9 +11,15 @@ struct Dependency { an target; virtual bool blocking() const = 0; + virtual string repr() const = 0; virtual bool Resolve(ConfigCompiler* compiler) = 0; }; +template +StreamT& operator<< (StreamT& stream, const Dependency& dep) { + return stream << dep.repr() << (dep.blocking() ? "(blocking)" : ""); +} + struct PendingChild : Dependency { string child_path; an child_ref; @@ -24,12 +30,19 @@ struct PendingChild : Dependency { bool blocking() const override { return false; } + string repr() const override { + return "PendingChild(" + child_path + ")"; + } bool Resolve(ConfigCompiler* compiler) override; }; +string Reference::repr() const { + return resource_id + ":" + local_path; +} + template StreamT& operator<< (StreamT& stream, const Reference& reference) { - return stream << reference.resource_id << ":" << reference.local_path; + return stream << reference.repr(); } struct IncludeReference : Dependency { @@ -38,6 +51,9 @@ struct IncludeReference : Dependency { bool blocking() const override { return true; } + string repr() const override { + return "Include(" + reference.repr() + ")"; + } bool Resolve(ConfigCompiler* compiler) override; Reference reference; @@ -49,6 +65,9 @@ struct PatchReference : Dependency { bool blocking() const override { return true; } + string repr() const override { + return "Patch(" + reference.repr() + ")"; + } bool Resolve(ConfigCompiler* compiler) override; Reference reference; @@ -62,6 +81,9 @@ struct PatchLiteral : Dependency { bool blocking() const override { return true; } + string repr() const override { + return "Patch()"; + } bool Resolve(ConfigCompiler* compiler) override; }; @@ -107,6 +129,7 @@ bool IncludeReference::Resolve(ConfigCompiler* compiler) { } bool PatchReference::Resolve(ConfigCompiler* compiler) { + LOG(INFO) << "PatchReference::Resolve(reference = " << reference << ")"; auto item = ResolveReference(compiler, reference); if (!item) { return false; @@ -125,10 +148,12 @@ bool TraverseCopyOnWrite(an root, const string& path, an item); bool PatchLiteral::Resolve(ConfigCompiler* compiler) { + LOG(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; @@ -143,25 +168,29 @@ void ConfigDependencyGraph::Add(an dependency) { 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(); + auto& target_deps = deps[target_path]; + bool target_was_pending = !target_deps.empty(); + target_deps.push_back(dependency); + LOG(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 dependency of parent node - auto child = target; 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(parent_path + "/" + last_key, child)); + parent_deps.push_front(New(parent_path + "/" + last_key, *child)); LOG(INFO) << "parent_path = " << parent_path << ", #deps = " << parent_deps.size(); - if (parent_was_pending) { - // so was all ancestors - break; + if (parent_was_pending || // so was all ancestors + keys.size() == 1) { // this parent is the progenitor + return; } - child = *prev; } } @@ -244,7 +273,7 @@ static bool ResolveBlockingDependencies(ConfigCompiler* compiler, static an GetResolvedItem(ConfigCompiler* compiler, an resource, const string& path) { - LOG(INFO) << "GetResolvedItem(" << resource->resource_id << ":/" << path << ")"; + LOG(INFO) << "GetResolvedItem(" << resource->resource_id << ":" << path << ")"; string node_path = resource->resource_id + ":"; if (!resource || compiler->blocking(node_path)) { return nullptr; @@ -364,7 +393,7 @@ static bool ParsePatch(ConfigCompiler* compiler, } bool ConfigCompiler::Parse(const string& key, const an& item) { - LOG(INFO) << "ConfigCompiler::Parse(" << key << ")"; + DLOG(INFO) << "ConfigCompiler::Parse(" << key << ")"; if (key == INCLUDE_DIRECTIVE) { return ParseInclude(this, item); } @@ -389,10 +418,10 @@ bool ConfigCompiler::ResolveDependencies(const string& path) { auto& deps = graph_->deps[path]; for (auto iter = deps.begin(); iter != deps.end(); ) { if (!(*iter)->Resolve(this)) { - LOG(INFO) << "unesolved dependency!"; + LOG(ERROR) << "unesolved dependency:" << **iter; return false; } - LOG(INFO) << "resolved."; + LOG(INFO) << "resolved " << **iter; iter = deps.erase(iter); } LOG(INFO) << "all dependencies resolved."; diff --git a/src/rime/config/config_compiler.h b/src/rime/config/config_compiler.h index 975b82053..d243fe17f 100644 --- a/src/rime/config/config_compiler.h +++ b/src/rime/config/config_compiler.h @@ -29,6 +29,8 @@ struct ConfigResource : ConfigItemRef { struct Reference { string resource_id; string local_path; + + string repr() const; }; class ResourceResolver; diff --git a/src/rime/config/config_data.cc b/src/rime/config/config_data.cc index 2ceb4c604..3653b6d32 100644 --- a/src/rime/config/config_data.cc +++ b/src/rime/config/config_data.cc @@ -207,6 +207,7 @@ class ConfigListEntryCowRef : public ConfigMapEntryCowRef { bool TraverseCopyOnWrite(an root, const string& path, an item) { + LOG(INFO) << "TraverseCopyOnWrite(" << path << ")"; if (path.empty() || path == "/") { *root = item; return true; From 69a6f3e71f6215d430f3696a54f2e43b4a0e884e Mon Sep 17 00:00:00 2001 From: Chen Gong Date: Sun, 3 Sep 2017 16:45:53 +0800 Subject: [PATCH 2/3] fix(config_compiler): enforce dependency priorities --- data/test/config_compiler_test.yaml | 10 +++++ src/rime/config/config_compiler.cc | 62 ++++++++++++++++++++--------- test/config_compiler_test.cc | 14 +++++++ 3 files changed, 67 insertions(+), 19 deletions(-) diff --git a/data/test/config_compiler_test.yaml b/data/test/config_compiler_test.yaml index 9a29bab78..408f67224 100644 --- a/data/test/config_compiler_test.yaml +++ b/data/test/config_compiler_test.yaml @@ -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 diff --git a/src/rime/config/config_compiler.cc b/src/rime/config/config_compiler.cc index 8f98c1cd2..00851c335 100644 --- a/src/rime/config/config_compiler.cc +++ b/src/rime/config/config_compiler.cc @@ -7,17 +7,26 @@ namespace rime { +enum DependencyPriority { + kPendingChild = 0, + kInclude = 1, + kPatch = 2, +}; + struct Dependency { an 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 StreamT& operator<< (StreamT& stream, const Dependency& dep) { - return stream << dep.repr() << (dep.blocking() ? "(blocking)" : ""); + return stream << dep.repr(); } struct PendingChild : Dependency { @@ -27,8 +36,8 @@ struct PendingChild : Dependency { PendingChild(const string& path, const an& 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 + ")"; @@ -48,8 +57,8 @@ StreamT& operator<< (StreamT& stream, const Reference& reference) { 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() + ")"; @@ -62,8 +71,8 @@ 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() + ")"; @@ -78,8 +87,8 @@ struct PatchLiteral : Dependency { PatchLiteral(an map) : patch(map) { } - bool blocking() const override { - return true; + DependencyPriority priority() const override { + return kPatch; } string repr() const override { return "Patch()"; @@ -91,7 +100,7 @@ struct ConfigDependencyGraph { map> resources; vector> node_stack; vector key_stack; - map>> deps; + map>> deps; void Add(an dependency); @@ -162,21 +171,34 @@ bool PatchLiteral::Resolve(ConfigCompiler* compiler) { return success; } +static void InsertByPriority(vector>& list, + const an& value) { + auto upper = std::upper_bound( + list.begin(), list.end(), value, + [](const an& lhs, const an& rhs) { + return lhs->priority() < rhs->priority(); + }); + list.insert(upper, value); +} + void ConfigDependencyGraph::Add(an dependency) { - LOG(INFO) << "ConfigDependencyGraph::Add(), node_stack.size() = " << node_stack.size(); + LOG(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); auto& target_deps = deps[target_path]; bool target_was_pending = !target_deps.empty(); - target_deps.push_back(dependency); - LOG(INFO) << "target_path = " << target_path << ", #deps = " << target_deps.size(); + InsertByPriority(target_deps, dependency); + LOG(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 dependency of parent node + // 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 child = node_stack.rbegin(); child != node_stack.rend(); ++child) { auto last_key = keys.back(); @@ -185,8 +207,10 @@ void ConfigDependencyGraph::Add(an dependency) { 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(parent_path + "/" + last_key, *child)); - LOG(INFO) << "parent_path = " << parent_path << ", #deps = " << parent_deps.size(); + InsertByPriority(parent_deps, + New(parent_path + "/" + last_key, *child)); + LOG(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; @@ -418,10 +442,10 @@ bool ConfigCompiler::ResolveDependencies(const string& path) { auto& deps = graph_->deps[path]; for (auto iter = deps.begin(); iter != deps.end(); ) { if (!(*iter)->Resolve(this)) { - LOG(ERROR) << "unesolved dependency:" << **iter; + LOG(ERROR) << "unresolved dependency: " << **iter; return false; } - LOG(INFO) << "resolved " << **iter; + LOG(INFO) << "resolved: " << **iter; iter = deps.erase(iter); } LOG(INFO) << "all dependencies resolved."; diff --git a/test/config_compiler_test.cc b/test/config_compiler_test.cc index f7043d005..69990ddbd 100644 --- a/test/config_compiler_test.cc +++ b/test/config_compiler_test.cc @@ -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 From d503513ddc482fdc6551e5d6d83dffb64c4af0cc Mon Sep 17 00:00:00 2001 From: Chen Gong Date: Mon, 4 Sep 2017 09:01:32 +0800 Subject: [PATCH 3/3] chore(config): less logs --- src/rime/config/config_compiler.cc | 40 +++++++++++++++--------------- src/rime/config/config_data.cc | 4 +-- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/rime/config/config_compiler.cc b/src/rime/config/config_compiler.cc index 00851c335..e33a7c39d 100644 --- a/src/rime/config/config_compiler.cc +++ b/src/rime/config/config_compiler.cc @@ -128,7 +128,7 @@ static an 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; @@ -138,7 +138,7 @@ bool IncludeReference::Resolve(ConfigCompiler* compiler) { } bool PatchReference::Resolve(ConfigCompiler* compiler) { - LOG(INFO) << "PatchReference::Resolve(reference = " << reference << ")"; + DLOG(INFO) << "PatchReference::Resolve(reference = " << reference << ")"; auto item = ResolveReference(compiler, reference); if (!item) { return false; @@ -157,7 +157,7 @@ bool TraverseCopyOnWrite(an root, const string& path, an item); bool PatchLiteral::Resolve(ConfigCompiler* compiler) { - LOG(INFO) << "PatchLiteral::Resolve()"; + DLOG(INFO) << "PatchLiteral::Resolve()"; bool success = true; for (const auto& entry : *patch) { const auto& path = entry.first; @@ -182,8 +182,8 @@ static void InsertByPriority(vector>& list, } void ConfigDependencyGraph::Add(an 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; @@ -191,8 +191,8 @@ void ConfigDependencyGraph::Add(an dependency) { auto& target_deps = deps[target_path]; bool target_was_pending = !target_deps.empty(); InsertByPriority(target_deps, dependency); - LOG(INFO) << "target_path = " << target_path - << ", #deps = " << target_deps.size(); + 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; @@ -209,8 +209,8 @@ void ConfigDependencyGraph::Add(an dependency) { // Pending children should be resolved before applying __include or __patch InsertByPriority(parent_deps, New(parent_path + "/" + last_key, *child)); - LOG(INFO) << "parent_path = " << parent_path - << ", #deps = " << parent_deps.size(); + 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; @@ -286,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; @@ -297,7 +297,7 @@ static bool ResolveBlockingDependencies(ConfigCompiler* compiler, static an GetResolvedItem(ConfigCompiler* compiler, an 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; @@ -320,7 +320,7 @@ static an GetResolvedItem(ConfigCompiler* compiler, result.reset(); } } else if (Is(result)) { - LOG(INFO) << "advance with key: " << key; + DLOG(INFO) << "advance with key: " << key; (node_path += "/") += key; if (!ResolveBlockingDependencies(compiler, node_path)) { return nullptr; @@ -356,7 +356,7 @@ static an 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); @@ -369,7 +369,7 @@ static bool ParseInclude(ConfigCompiler* compiler, const an& item) { if (Is(item)) { auto path = As(item)->str(); - LOG(INFO) << "ParseInclude(" << path << ")"; + DLOG(INFO) << "ParseInclude(" << path << ")"; compiler->AddDependency( New(compiler->CreateReference(path))); return true; @@ -403,13 +403,13 @@ static bool ParsePatch(ConfigCompiler* compiler, const an& item) { if (Is(item)) { auto path = As(item)->str(); - LOG(INFO) << "ParsePatch(" << path << ")"; + DLOG(INFO) << "ParsePatch(" << path << ")"; compiler->AddDependency( New(compiler->CreateReference(path))); return true; } if (Is(item)) { - LOG(INFO) << "ParsePatch()"; + DLOG(INFO) << "ParsePatch()"; compiler->AddDependency(New(As(item))); return true; } @@ -428,17 +428,17 @@ bool ConfigCompiler::Parse(const string& key, const an& item) { } bool ConfigCompiler::Link(an 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)) { diff --git a/src/rime/config/config_data.cc b/src/rime/config/config_data.cc index 3653b6d32..24073bcb2 100644 --- a/src/rime/config/config_data.cc +++ b/src/rime/config/config_data.cc @@ -159,7 +159,7 @@ class ConfigDataRootRef : public ConfigItemRef { template static an Cow(const an& container, const string& key) { if (!container) { - LOG(INFO) << "creating node: " << key; + DLOG(INFO) << "creating node: " << key; return New(); } DLOG(INFO) << "copy on write: " << key; @@ -207,7 +207,7 @@ class ConfigListEntryCowRef : public ConfigMapEntryCowRef { bool TraverseCopyOnWrite(an root, const string& path, an item) { - LOG(INFO) << "TraverseCopyOnWrite(" << path << ")"; + DLOG(INFO) << "TraverseCopyOnWrite(" << path << ")"; if (path.empty() || path == "/") { *root = item; return true;