Skip to content

Commit 68988e9

Browse files
Mizuchifacebook-github-bot
authored andcommitted
Only import programs that are needed in runtime
Summary: Currently we import all includes even if they are not referenced later. However, in BuildGraph API, we won't generate runtime dependencies for modules that are not referenced later since it's unnecessary. This discrepancy breaks Configerator since they won't materialize modules not in BuildGraph dependencies. This diff changed the logic and removed unnecessary imports in Patch. Basically it iterates all types in the root program to see whether it referenced programs outside root, and only imports those includes. The same idea should work for thrift-python imports which I am experimenting in the next stacked diff Reviewed By: pranavtbhat Differential Revision: D77348074 fbshipit-source-id: 9686fd975734b2cbeea8ddadb9b5c2422f76169c
1 parent 6f2ceea commit 68988e9

File tree

4 files changed

+61
-4
lines changed

4 files changed

+61
-4
lines changed

third-party/thrift/src/thrift/compiler/generate/python/util.cc

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
#include <boost/algorithm/string.hpp>
2020
#include <boost/algorithm/string/replace.hpp>
21+
#include <thrift/compiler/ast/ast_visitor.h>
2122
#include <thrift/compiler/generate/cpp/util.h>
2223

2324
namespace apache::thrift::compiler {
@@ -36,6 +37,53 @@ bool is_patch_program(const t_program* prog) {
3637
: false;
3738
}
3839

40+
static void get_needed_includes_impl(
41+
const t_program* root,
42+
const t_type& type,
43+
std::unordered_set<const t_type*>& seen,
44+
std::unordered_set<const t_program*>& result) {
45+
if (!seen.insert(&type).second) {
46+
return;
47+
}
48+
if (type.get_program() && type.get_program() != root) {
49+
// If type is not in root, root needs type's program in runtime.
50+
result.insert(type.get_program());
51+
return;
52+
}
53+
if (const t_typedef* asTypedef = type.try_as<t_typedef>()) {
54+
get_needed_includes_impl(root, *asTypedef->get_type(), seen, result);
55+
}
56+
if (const t_list* asList = type.try_as<t_list>()) {
57+
get_needed_includes_impl(root, *asList->get_elem_type(), seen, result);
58+
}
59+
if (const t_set* asSet = type.try_as<t_set>()) {
60+
get_needed_includes_impl(root, *asSet->get_elem_type(), seen, result);
61+
}
62+
if (const t_map* asMap = type.try_as<t_map>()) {
63+
get_needed_includes_impl(root, *asMap->get_key_type(), seen, result);
64+
get_needed_includes_impl(root, *asMap->get_val_type(), seen, result);
65+
}
66+
if (const t_structured* asStructured = type.try_as<t_structured>()) {
67+
for (auto& field : asStructured->fields()) {
68+
get_needed_includes_impl(root, *field.type().get_type(), seen, result);
69+
}
70+
}
71+
}
72+
73+
std::unordered_set<const t_program*> needed_includes_in_runtime(
74+
const t_program* root) {
75+
std::unordered_set<const t_program*> programs;
76+
std::unordered_set<const t_type*> seen;
77+
const_ast_visitor visitor;
78+
visitor.add_root_definition_visitor([&](const t_named& def) {
79+
if (auto type = dynamic_cast<const t_type*>(&def)) {
80+
get_needed_includes_impl(root, *type, seen, programs);
81+
}
82+
});
83+
visitor(*root);
84+
return programs;
85+
}
86+
3987
bool type_contains_patch(const t_type* type) {
4088
if (auto typedf = dynamic_cast<t_typedef const*>(type)) {
4189
return type_contains_patch(typedf->get_type());

third-party/thrift/src/thrift/compiler/generate/python/util.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ std::string get_py3_namespace_with_name_and_prefix(
4848
const std::string& prefix,
4949
const std::string& sep = ".");
5050

51+
// List of programs that `root` needed in runtime
52+
std::unordered_set<const t_program*> needed_includes_in_runtime(
53+
const t_program* root);
54+
5155
inline const std::unordered_set<std::string>& get_python_reserved_names() {
5256
static const std::unordered_set<std::string> keywords = {
5357
"False", "None", "True", "and", "as", "assert", "async",

third-party/thrift/src/thrift/compiler/generate/t_mstch_python_generator.cc

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,8 @@ class python_mstch_program : public mstch_program {
248248
{"included_module_mangle", it->ns_mangle},
249249
{"has_services?", it->has_services},
250250
{"has_types?", it->has_types},
251-
{"is_patch?", it->is_patch}});
251+
{"is_patch?", it->is_patch},
252+
{"needed_in_runtime?", it->needed_in_runtime}});
252253
}
253254
return a;
254255
}
@@ -330,9 +331,11 @@ class python_mstch_program : public mstch_program {
330331
bool has_services;
331332
bool has_types;
332333
bool is_patch;
334+
bool needed_in_runtime;
333335
};
334336

335337
void gather_included_program_namespaces() {
338+
auto needed_includes = needed_includes_in_runtime(program_);
336339
for (const t_program* included_program :
337340
program_->get_includes_for_codegen()) {
338341
bool has_types =
@@ -347,7 +350,8 @@ class python_mstch_program : public mstch_program {
347350
included_program, get_option("root_module_prefix")),
348351
!included_program->services().empty(),
349352
has_types,
350-
is_patch_program(included_program)};
353+
is_patch_program(included_program),
354+
static_cast<bool>(needed_includes.count(included_program))};
351355
}
352356
}
353357

@@ -366,6 +370,7 @@ class python_mstch_program : public mstch_program {
366370
ns.has_services = false;
367371
ns.has_types = true;
368372
ns.is_patch = is_patch_program(prog);
373+
ns.needed_in_runtime = true;
369374
include_namespaces_[path] = std::move(ns);
370375
}
371376
}

third-party/thrift/src/thrift/compiler/generate/templates/patch/thrift_patch.py.mustache

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,11 @@ import {{program:safe_patch_module_path}}.thrift_types as _fbthrift_safe_patch_t
5656
{{/if (not program:safe_patch?)}}
5757

5858
{{#program:include_namespaces}}
59-
{{#if has_types?}}{{#if (not is_patch?)}}
59+
{{#if has_types?}}{{#if (not is_patch?)}}{{#if needed_in_runtime?}}
6060

6161
import {{included_module_path}}.{{> ../python/types/types_import_path}} as {{included_module_mangle}}__{{> ../python/types/types_import_path}}
6262
import {{included_module_path}}.thrift_patch
63-
{{/if (not is_patch?)}}{{/if has_types?}}
63+
{{/if needed_in_runtime?}}{{/if (not is_patch?)}}{{/if has_types?}}
6464
{{/program:include_namespaces}}
6565

6666
{{#program:structs}}

0 commit comments

Comments
 (0)