Skip to content

Commit 81067f5

Browse files
praihanfacebook-github-bot
authored andcommitted
Make object manage its own memory and cheap to copy
Summary: As whisker has evolved, things have become more "dynamic": - `native_function` exists and is commonly used - property lookups happen mostly via `native_object` or `native_handle<>`. All of these APIs deal with `object::ptr` (not `object` directly) because they *may* need to be dynamically allocated on the heap. In future diffs, I plan to move APIs away from `object::ptr` where possible. Because: * Its use has resulted in the proliferation of `manage_as_static`, `manage_derived`, `manage_derived_ref` everywhere because we need "chains" of `shared_ptr` to avoid copying `whisker::object`. It makes the code hard to read. * It is pessimistic for the common case (`boolean`, `i64`, `string`, `native_object::ptr` need to be heap-allocated) while "optimizing" for the uncommon case (`array`, `map`). This diff moves `map` and `array` to use `managed_ptr` to solve the problems above and still avoid deep-copying `map` and `array`. Now, copying `object` is *always* cheap. Since `object` has an immutable API (except move copy/assign), we are fine to share `map` and `array` between instances Reviewed By: iahs Differential Revision: D71095233 fbshipit-source-id: 3089b2687f0396b7cef053800c0a454938a7490a
1 parent 1ff5f5d commit 81067f5

File tree

6 files changed

+139
-72
lines changed

6 files changed

+139
-72
lines changed

third-party/thrift/src/thrift/compiler/whisker/dsl.cc

+6-6
Original file line numberDiff line numberDiff line change
@@ -35,15 +35,15 @@ object::ptr array_like::at(std::size_t index) const {
3535
[&](const native_object::array_like::ptr& arr) -> object::ptr {
3636
return arr->at(index);
3737
},
38-
[&](const managed_ptr<array>& arr) -> object::ptr {
38+
[&](const managed_array& arr) -> object::ptr {
3939
return manage_derived_ref(arr, (*arr)[index]);
4040
});
4141
}
4242

4343
/* static */ std::optional<array_like> array_like::try_from(
4444
const object::ptr& o) {
4545
if (o->is_array()) {
46-
return array_like(manage_derived_ref(o, o->as_array()));
46+
return array_like(o->as_array());
4747
}
4848
if (o->is_native_object()) {
4949
if (native_object::array_like::ptr arr =
@@ -60,9 +60,9 @@ object::ptr map_like::lookup_property(std::string_view identifier) const {
6060
[&](const native_object::map_like::ptr& m) -> object::ptr {
6161
return m->lookup_property(identifier);
6262
},
63-
[&](const managed_ptr<map>& m) -> object::ptr {
63+
[&](const managed_map& m) -> object::ptr {
6464
if (auto it = m->find(identifier); it != m->end()) {
65-
return manage_as_static(it->second);
65+
return manage_derived_ref(m, it->second);
6666
}
6767
return nullptr;
6868
});
@@ -75,7 +75,7 @@ std::optional<std::set<std::string>> map_like::keys() const {
7575
[&](const native_object::map_like::ptr& m) -> result {
7676
return m->keys();
7777
},
78-
[&](const managed_ptr<map>& m) -> result {
78+
[&](const managed_map& m) -> result {
7979
std::set<std::string> keys;
8080
for (const auto& [key, _] : *m) {
8181
keys.insert(key);
@@ -86,7 +86,7 @@ std::optional<std::set<std::string>> map_like::keys() const {
8686

8787
/* static */ std::optional<map_like> map_like::try_from(const object::ptr& o) {
8888
if (o->is_map()) {
89-
return map_like(manage_derived_ref(o, o->as_map()));
89+
return map_like(o->as_map());
9090
}
9191
if (o->is_native_object()) {
9292
if (native_object::map_like::ptr m = o->as_native_object()->as_map_like()) {

third-party/thrift/src/thrift/compiler/whisker/dsl.h

+6-7
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
#include <string>
2929
#include <string_view>
3030
#include <type_traits>
31-
#include <vector>
3231

3332
namespace whisker::dsl {
3433

@@ -53,12 +52,12 @@ class array_like final : public native_object::array_like {
5352
*/
5453
static std::optional<array_like> try_from(const object::ptr&);
5554

56-
explicit array_like(native_object::array_like::ptr&& arr)
55+
explicit array_like(native_object::array_like::ptr arr)
5756
: which_(std::move(arr)) {}
58-
explicit array_like(managed_ptr<array>&& arr) : which_(std::move(arr)) {}
57+
explicit array_like(managed_array arr) : which_(std::move(arr)) {}
5958

6059
private:
61-
std::variant<native_object::array_like::ptr, managed_ptr<array>> which_;
60+
std::variant<native_object::array_like::ptr, managed_array> which_;
6261
};
6362
static_assert(std::is_move_constructible_v<array_like>);
6463
static_assert(std::is_copy_constructible_v<array_like>);
@@ -81,11 +80,11 @@ class map_like final : public native_object::map_like {
8180
*/
8281
static std::optional<map_like> try_from(const object::ptr&);
8382

84-
explicit map_like(native_object::map_like::ptr&& m) : which_(std::move(m)) {}
85-
explicit map_like(managed_ptr<map>&& m) : which_(std::move(m)) {}
83+
explicit map_like(native_object::map_like::ptr m) : which_(std::move(m)) {}
84+
explicit map_like(managed_map m) : which_(std::move(m)) {}
8685

8786
private:
88-
std::variant<native_object::map_like::ptr, managed_ptr<map>> which_;
87+
std::variant<native_object::map_like::ptr, managed_map> which_;
8988
};
9089
static_assert(std::is_move_constructible_v<map_like>);
9190
static_assert(std::is_copy_constructible_v<map_like>);

third-party/thrift/src/thrift/compiler/whisker/eval_context.cc

+3-3
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ object::ptr find_property(
3838
[](f64) -> result { return nullptr; },
3939
[](const string&) -> result { return nullptr; },
4040
[](boolean) -> result { return nullptr; },
41-
[](const array&) -> result { return nullptr; },
41+
[](const managed_array&) -> result { return nullptr; },
4242
[&](const native_object::ptr& o) -> result {
4343
if (auto map_like = o->as_map_like()) {
4444
return map_like->lookup_property(identifier.name);
@@ -72,8 +72,8 @@ object::ptr find_property(
7272
}
7373
return nullptr;
7474
},
75-
[&](const map& m) -> result {
76-
if (auto it = m.find(identifier.name); it != m.end()) {
75+
[&](const managed_map& m) -> result {
76+
if (auto it = m->find(identifier.name); it != m->end()) {
7777
return manage_as_static(it->second);
7878
}
7979
return nullptr;

third-party/thrift/src/thrift/compiler/whisker/object.cc

+15-11
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,12 @@ class to_string_visitor {
4848
typename = std::enable_if_t<std::is_same_v<T, object>>>
4949
void visit(const T& value, tree_printer::scope scope) const {
5050
value.visit(
51-
[&](const array& a) { visit_maybe_truncate(a, std::move(scope)); },
52-
[&](const map& m) { visit_maybe_truncate(m, std::move(scope)); },
51+
[&](const managed_array& a) {
52+
visit_maybe_truncate(a, std::move(scope));
53+
},
54+
[&](const managed_map& m) {
55+
visit_maybe_truncate(m, std::move(scope));
56+
},
5357
[&](const native_object::ptr& o) {
5458
visit_maybe_truncate(o, std::move(scope));
5559
},
@@ -98,20 +102,20 @@ class to_string_visitor {
98102
scope.println("null");
99103
}
100104

101-
void visit(const array& arr, tree_printer::scope scope) const {
105+
void visit(const managed_array& arr, tree_printer::scope scope) const {
102106
require_within_max_depth(scope);
103-
scope.println("array (size={})", arr.size());
104-
for (std::size_t i = 0; i < arr.size(); ++i) {
107+
scope.println("array (size={})", arr->size());
108+
for (std::size_t i = 0; i < arr->size(); ++i) {
105109
auto element_scope = scope.open_transparent_property();
106110
element_scope.println("[{}]", i);
107-
visit(arr[i], element_scope.open_node());
111+
visit((*arr)[i], element_scope.open_node());
108112
}
109113
}
110114

111-
void visit(const map& m, tree_printer::scope scope) const {
115+
void visit(const managed_map& m, tree_printer::scope scope) const {
112116
require_within_max_depth(scope);
113-
scope.println("map (size={})", m.size());
114-
for (const auto& [key, value] : m) {
117+
scope.println("map (size={})", m->size());
118+
for (const auto& [key, value] : *m) {
115119
auto element_scope = scope.open_transparent_property();
116120
element_scope.println("'{}'", key);
117121
visit(value, element_scope.open_node());
@@ -354,8 +358,8 @@ std::string object::describe_type() const {
354358
[](const string&) -> std::string { return "string"; },
355359
[](boolean) -> std::string { return "boolean"; },
356360
[](null) -> std::string { return "null"; },
357-
[](const array&) -> std::string { return "array"; },
358-
[](const map&) -> std::string { return "map"; },
361+
[](const managed_array&) -> std::string { return "array"; },
362+
[](const managed_map&) -> std::string { return "map"; },
359363
[](const native_object::ptr& o) -> std::string {
360364
return o->describe_type();
361365
},

0 commit comments

Comments
 (0)