You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/04/26 17:56:09 UTC
[GitHub] [tvm] areusch commented on a change in pull request #7918: [RUNTIME][BUGFIX] Fix DSO module problem when its parent get destructed.
areusch commented on a change in pull request #7918:
URL: https://github.com/apache/tvm/pull/7918#discussion_r620461701
##########
File path: src/runtime/library_module.cc
##########
@@ -143,14 +144,20 @@ runtime::Module ProcessModuleBlob(const char* mblob, ObjectPtr<Library> lib) {
std::vector<Module> modules;
std::vector<uint64_t> import_tree_row_ptr;
std::vector<uint64_t> import_tree_child_indices;
+ int num_dso_module = 0;
+
for (uint64_t i = 0; i < size; ++i) {
std::string tkey;
ICHECK(stream->Read(&tkey));
// Currently, _lib is for DSOModule, but we
- // don't have loadbinary function for it currently
+ // need to be handled specially
if (tkey == "_lib") {
auto dso_module = Module(make_object<LibraryModuleNode>(lib));
+ *dso_ctx_addr = dso_module.operator->();
+ ++num_dso_module;
modules.emplace_back(dso_module);
+ ICHECK_EQ(num_dso_module, 1U) << "Multiple dso module detected, please upgrade tvm "
Review comment:
how come you do signed-unsigned comparison here? also, maybe a slightly better error message: "Multiple DSO-exportable modules detected, please re-export your module with the latest TVM.
does it makes sense to make this a warning rather than a hard fail? it means we will break old modules, and we took some steps to work around that with GraphExecutor change.
##########
File path: src/runtime/library_module.cc
##########
@@ -143,14 +144,20 @@ runtime::Module ProcessModuleBlob(const char* mblob, ObjectPtr<Library> lib) {
std::vector<Module> modules;
std::vector<uint64_t> import_tree_row_ptr;
std::vector<uint64_t> import_tree_child_indices;
+ int num_dso_module = 0;
+
for (uint64_t i = 0; i < size; ++i) {
std::string tkey;
ICHECK(stream->Read(&tkey));
// Currently, _lib is for DSOModule, but we
- // don't have loadbinary function for it currently
+ // need to be handled specially
Review comment:
it would be great to explain "specially:" DSOModule's implementation is provided by the wrapping LibraryModule (supplied as `lib` here). "_lib" serves as a placeholder in the module import tree to indicate where to place the DSOModule.
##########
File path: src/target/codegen.cc
##########
@@ -110,32 +115,100 @@ class ModuleSerializer {
// invariance: root module is always at location 0.
// The module order is collected via DFS
+ // This function merges all the DSO exportable module into
+ // a single one as this is also what happens in the final hierachy
void CreateModuleIndex() {
std::unordered_set<const runtime::ModuleNode*> visited{mod_.operator->()};
std::vector<runtime::ModuleNode*> stack{mod_.operator->()};
uint64_t module_index = 0;
- while (!stack.empty()) {
- runtime::ModuleNode* n = stack.back();
- stack.pop_back();
- mod2index_[n] = module_index++;
- mod_vec_.emplace_back(n);
- for (runtime::Module m : n->imports()) {
+ auto fpush_imports_to_stack = [&](runtime::ModuleNode* node) {
+ for (runtime::Module m : node->imports()) {
runtime::ModuleNode* next = m.operator->();
if (visited.count(next) == 0) {
visited.insert(next);
stack.push_back(next);
}
}
+ };
+
+ std::vector<runtime::ModuleNode*> dso_exportable_boundary;
+
+ // Create module index that merges all dso module into a single group.
+ //
+ // Do a two phase visit, to ensure dso module's index
+ // is always bigger than a parent of any dso module
+ // and smaller than children of any dso module.
+ //
+ // Error will be raised in CreateImportTree
+ // if merging dso module causes a cycle in the import tree
+
+ // Phase 0: only expand non-dso-module and record the boundary.
+ while (!stack.empty()) {
+ runtime::ModuleNode* n = stack.back();
+ stack.pop_back();
+ if (DSOExportable(n)) {
+ // do not recursively expand dso modules
+ // we will expand in phase 1
+ dso_exportable_boundary.emplace_back(n);
+ } else {
+ // expand the non-dso modules
+ mod2index_[n] = module_index++;
+ mod_group_vec_.emplace_back(std::vector<runtime::ModuleNode*>({n}));
+ fpush_imports_to_stack(n);
+ }
+ }
+ if (dso_exportable_boundary.size() == 0) return;
+
+ // create the slot for dso exportable modules
+ uint64_t dso_module_index = module_index++;
Review comment:
it would be great to comment why this particular slot is chosen
##########
File path: src/runtime/library_module.cc
##########
@@ -125,10 +125,11 @@ Module LoadModuleFromBinary(const std::string& type_key, dmlc::Stream* stream) {
* \brief Load and append module blob to module list
* \param mblob The module blob.
* \param lib The library.
- *
- * \return Root Module.
+ * \param root_module the output root module
+ * \param dso_ctx_addr the output dso module
*/
-runtime::Module ProcessModuleBlob(const char* mblob, ObjectPtr<Library> lib) {
+void ProcessModuleBlob(const char* mblob, ObjectPtr<Library> lib, runtime::Module* root_module,
+ runtime::ModuleNode** dso_ctx_addr = nullptr) {
Review comment:
why is this nullptr by default? seems like it should never be nullptr..?
##########
File path: src/target/codegen.cc
##########
@@ -110,32 +115,100 @@ class ModuleSerializer {
// invariance: root module is always at location 0.
// The module order is collected via DFS
+ // This function merges all the DSO exportable module into
+ // a single one as this is also what happens in the final hierachy
void CreateModuleIndex() {
std::unordered_set<const runtime::ModuleNode*> visited{mod_.operator->()};
std::vector<runtime::ModuleNode*> stack{mod_.operator->()};
uint64_t module_index = 0;
- while (!stack.empty()) {
- runtime::ModuleNode* n = stack.back();
- stack.pop_back();
- mod2index_[n] = module_index++;
- mod_vec_.emplace_back(n);
- for (runtime::Module m : n->imports()) {
+ auto fpush_imports_to_stack = [&](runtime::ModuleNode* node) {
+ for (runtime::Module m : node->imports()) {
runtime::ModuleNode* next = m.operator->();
if (visited.count(next) == 0) {
visited.insert(next);
stack.push_back(next);
}
}
+ };
+
+ std::vector<runtime::ModuleNode*> dso_exportable_boundary;
+
+ // Create module index that merges all dso module into a single group.
+ //
+ // Do a two phase visit, to ensure dso module's index
+ // is always bigger than a parent of any dso module
+ // and smaller than children of any dso module.
+ //
+ // Error will be raised in CreateImportTree
+ // if merging dso module causes a cycle in the import tree
+
+ // Phase 0: only expand non-dso-module and record the boundary.
+ while (!stack.empty()) {
+ runtime::ModuleNode* n = stack.back();
+ stack.pop_back();
+ if (DSOExportable(n)) {
+ // do not recursively expand dso modules
+ // we will expand in phase 1
+ dso_exportable_boundary.emplace_back(n);
+ } else {
+ // expand the non-dso modules
+ mod2index_[n] = module_index++;
+ mod_group_vec_.emplace_back(std::vector<runtime::ModuleNode*>({n}));
+ fpush_imports_to_stack(n);
+ }
+ }
+ if (dso_exportable_boundary.size() == 0) return;
+
+ // create the slot for dso exportable modules
+ uint64_t dso_module_index = module_index++;
+ mod_group_vec_.emplace_back(std::vector<runtime::ModuleNode*>());
+
+ stack = std::move(dso_exportable_boundary);
Review comment:
rather than do this, would prefer to avoid the closure and make `fpush_imports_to_stack` take an explicit argument which is the stack to push to. it will make the code more readable.
##########
File path: src/runtime/library_module.cc
##########
@@ -177,11 +186,11 @@ runtime::Module ProcessModuleBlob(const char* mblob, ObjectPtr<Library> lib) {
module_import_addr->emplace_back(modules[child_index]);
}
}
+ ICHECK(!modules.empty());
Review comment:
add an error message
##########
File path: src/target/codegen.cc
##########
@@ -110,32 +115,100 @@ class ModuleSerializer {
// invariance: root module is always at location 0.
// The module order is collected via DFS
+ // This function merges all the DSO exportable module into
+ // a single one as this is also what happens in the final hierachy
void CreateModuleIndex() {
std::unordered_set<const runtime::ModuleNode*> visited{mod_.operator->()};
std::vector<runtime::ModuleNode*> stack{mod_.operator->()};
uint64_t module_index = 0;
- while (!stack.empty()) {
- runtime::ModuleNode* n = stack.back();
- stack.pop_back();
- mod2index_[n] = module_index++;
- mod_vec_.emplace_back(n);
- for (runtime::Module m : n->imports()) {
+ auto fpush_imports_to_stack = [&](runtime::ModuleNode* node) {
+ for (runtime::Module m : node->imports()) {
runtime::ModuleNode* next = m.operator->();
if (visited.count(next) == 0) {
visited.insert(next);
stack.push_back(next);
}
}
+ };
+
+ std::vector<runtime::ModuleNode*> dso_exportable_boundary;
+
+ // Create module index that merges all dso module into a single group.
+ //
+ // Do a two phase visit, to ensure dso module's index
+ // is always bigger than a parent of any dso module
+ // and smaller than children of any dso module.
+ //
+ // Error will be raised in CreateImportTree
+ // if merging dso module causes a cycle in the import tree
+
+ // Phase 0: only expand non-dso-module and record the boundary.
+ while (!stack.empty()) {
+ runtime::ModuleNode* n = stack.back();
+ stack.pop_back();
+ if (DSOExportable(n)) {
+ // do not recursively expand dso modules
+ // we will expand in phase 1
+ dso_exportable_boundary.emplace_back(n);
+ } else {
+ // expand the non-dso modules
+ mod2index_[n] = module_index++;
+ mod_group_vec_.emplace_back(std::vector<runtime::ModuleNode*>({n}));
+ fpush_imports_to_stack(n);
+ }
+ }
+ if (dso_exportable_boundary.size() == 0) return;
+
+ // create the slot for dso exportable modules
+ uint64_t dso_module_index = module_index++;
+ mod_group_vec_.emplace_back(std::vector<runtime::ModuleNode*>());
+
+ stack = std::move(dso_exportable_boundary);
+
+ // Phase 1: expand the children of dso modules.
+ while (!stack.empty()) {
+ runtime::ModuleNode* n = stack.back();
+ stack.pop_back();
+
+ if (DSOExportable(n)) {
+ mod_group_vec_[dso_module_index].emplace_back(n);
+ mod2index_[n] = dso_module_index;
+ } else {
+ mod2index_[n] = module_index++;
+ mod_group_vec_.emplace_back(std::vector<runtime::ModuleNode*>({n}));
+ }
+ fpush_imports_to_stack(n);
}
}
void CreateImportTree() {
- for (auto m : mod_vec_) {
- for (runtime::Module im : m->imports()) {
- uint64_t mod_index = mod2index_[im.operator->()];
- import_tree_child_indices_.push_back(mod_index);
+ std::vector<int64_t> child_indices;
+
+ for (size_t parent_index = 0; parent_index < mod_group_vec_.size(); ++parent_index) {
+ child_indices.clear();
+ for (const auto* m : mod_group_vec_[parent_index]) {
+ for (runtime::Module im : m->imports()) {
+ uint64_t mod_index = mod2index_.at(im.operator->());
+ // skip cycle when dso modules are merged together
+ if (mod_index != parent_index) {
+ child_indices.emplace_back(mod_index);
+ }
+ }
+ }
+ // sort and unique the merged indices
+ std::sort(child_indices.begin(), child_indices.end());
+ auto unique_end = std::unique(child_indices.begin(), child_indices.end());
+
+ // Check cycles due to merging dso exportable modules.
+ if (child_indices.size() != 0) {
+ CHECK_LT(parent_index, child_indices[0])
Review comment:
it's clear if you read the code carefully, but perhaps to help people, would be great if you could:
1) add a comment here explaining explicitly why lower indices are "higher" in the tree (just explain the traversal order)
2) include the offending module instances in the error message, at least for now this will allow their type_key to be shown to the user and it may help them understand the problem in this case or us understand on forum when there are bug reports or questions.
##########
File path: src/target/codegen.cc
##########
@@ -75,21 +75,26 @@ class ModuleSerializer {
if (has_import_tree) {
// we will append one key for _import_tree
// The layout is the same as before: binary_size, key, logic, key, logic...
- sz = mod_vec_.size() + 1;
+ sz = mod_group_vec_.size() + 1;
} else {
// Keep the old behaviour
sz = mod_->imports().size();
}
stream->Write(sz);
- for (auto m : mod_vec_) {
- std::string mod_type_key = m->type_key();
- if (!DSOExportable(m)) {
- stream->Write(mod_type_key);
- m->SaveToBinary(stream);
- } else if (has_import_tree) {
- mod_type_key = "_lib";
+ for (const auto& group : mod_group_vec_) {
+ CHECK_NE(group.size(), 0);
Review comment:
add an error message, here and below
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org