Skip to content

Commit d646a31

Browse files
mparkmeta-codesync[bot]
authored andcommitted
Support explicit template instantiations for AssignPatch.
Summary: This diff builds on top of D85391082 which got `gen_patch_ConfigScopes.thrift.cpp` from **128s** -> **78s** in `opt` mode and **86s** -> **61s** in `dev` mode. While the `extern template` declarations added in D85391082 avoided the nested template instantiations for non-`AssignPatch` cases, it did not help with the `AssignPatch` cases. `gen_patch_ConfigScopes.thrift.cpp` remained still quite expensive, exactly because one of the 3 fields, contains `ClientConfigWrapper.ClientConfigPatchWrapper` which is an `AssignPatch`. This diff takes the following few steps: 1. Moves `AssignPatch` into `detail`, next to `StructPatch`. 2. Declare `AssignPatch`'s `apply` and `merge` out-of-line, in `AssignPatchImpl.h`, in-line with `StructPatchImpl.h`. 3. Update the gen-patch codegen to explicitly declare and define the instantiations for `apply` and `merge`. --- `fbcode//configerator/structs/servicerouter/configs/if:gen_patch_config_scopes-cpp2-types` was the #1 in the sum of `Action Execution Total Time` as well as `Action Potential Improvement Time` for Thrift targets in critical path of user builds, over the last 2 weeks. [Scuba query](https://fburl.com/scuba/buck2_critical_path_actions_user/t43267f1) The file that causes this is `gen_patch_ConfigScopes.thrift.cpp`, as can be seen [this query](https://fburl.com/scuba/buck2_critical_path_actions_user/dmwzj03u). With this diff, the 3 costly identifiers improve like this: |**Identifier**|**Before**|**D85391082**| **After** | | `gen_patch_ConfigScopes.thrift.cpp (pic) (optimized)` | 128s | 78s | 16s | | `gen_patch_ConfigScopes.thrift.cpp` | 86s | 61s | 14.5s | | `gen_patch_ConfigScopes.thrift.cpp (pic)` | 86s | 61s | 15s | Combined with D85391082, we're looking at ~87.5% improvement in the pic/opt case, which is about ~70% of the time among these 3 files, and ~82.5% improvement in the other 2 cases. Reviewed By: vitaut Differential Revision: D85415239 fbshipit-source-id: 0eef1d86ba6ed4bde4a68907f6b5e425a38bbd77
1 parent 35fe670 commit d646a31

File tree

2 files changed

+68
-39
lines changed

2 files changed

+68
-39
lines changed

third-party/thrift/src/thrift/lib/cpp2/op/AssignPatch.h renamed to third-party/thrift/src/thrift/lib/cpp2/op/detail/AssignPatch.h

Lines changed: 2 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -74,46 +74,9 @@ class AssignPatch : public BaseAssignPatch<Patch, AssignPatch<Patch>> {
7474
return *this;
7575
}
7676

77-
void apply(T& val) const {
78-
if (dynPatch_) {
79-
auto value = protocol::asValueStruct<Tag>(val);
80-
dynPatch_->apply(value);
81-
val = protocol::fromValueStruct<Tag>(value);
82-
} else if (auto p = data_.assign()) {
83-
val = data_.assign().value();
84-
}
85-
}
77+
void apply(T& val) const;
8678

87-
void merge(AssignPatch other) {
88-
if (dynPatch_ && other.dynPatch_) {
89-
XLOG_EVERY_MS(CRITICAL, 10000)
90-
<< "Merging dynamic patch is not implemented. "
91-
"The merged result will be incorrect.\n"
92-
"First Patch = "
93-
<< folly::toPrettyJson(
94-
apache::thrift::protocol::toDynamic(dynPatch_->toObject()))
95-
<< "\nSecond Patch = "
96-
<< folly::toPrettyJson(
97-
apache::thrift::protocol::toDynamic(
98-
other.dynPatch_->toObject()));
99-
100-
// Do nothing, which is the old behavior
101-
return;
102-
}
103-
104-
if (!other.dynPatch_) {
105-
if (auto p = other.data_.assign()) {
106-
assign(std::move(*p));
107-
}
108-
return;
109-
}
110-
111-
if (auto p = data_.assign()) {
112-
other.apply(*p);
113-
} else {
114-
dynPatch_ = std::move(other.dynPatch_);
115-
}
116-
}
79+
void merge(AssignPatch other);
11780

11881
/// @cond
11982
template <class Protocol>
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
#pragma once
18+
19+
#include <thrift/lib/cpp2/op/detail/AssignPatch.h>
20+
21+
namespace apache::thrift::op::detail {
22+
23+
template <typename Patch>
24+
void AssignPatch<Patch>::apply(T& val) const {
25+
if (dynPatch_) {
26+
auto value = protocol::asValueStruct<Tag>(val);
27+
dynPatch_->apply(value);
28+
val = protocol::fromValueStruct<Tag>(value);
29+
} else if (auto p = data_.assign()) {
30+
val = data_.assign().value();
31+
}
32+
}
33+
34+
template <typename Patch>
35+
void AssignPatch<Patch>::merge(AssignPatch other) {
36+
if (dynPatch_ && other.dynPatch_) {
37+
XLOG_EVERY_MS(CRITICAL, 10000)
38+
<< "Merging dynamic patch is not implemented. "
39+
"The merged result will be incorrect.\n"
40+
"First Patch = "
41+
<< folly::toPrettyJson(
42+
apache::thrift::protocol::toDynamic(dynPatch_->toObject()))
43+
<< "\nSecond Patch = "
44+
<< folly::toPrettyJson(
45+
apache::thrift::protocol::toDynamic(
46+
other.dynPatch_->toObject()));
47+
48+
// Do nothing, which is the old behavior
49+
return;
50+
}
51+
52+
if (!other.dynPatch_) {
53+
if (auto p = other.data_.assign()) {
54+
assign(std::move(*p));
55+
}
56+
return;
57+
}
58+
59+
if (auto p = data_.assign()) {
60+
other.apply(*p);
61+
} else {
62+
dynPatch_ = std::move(other.dynPatch_);
63+
}
64+
}
65+
66+
} // namespace apache::thrift::op::detail

0 commit comments

Comments
 (0)