Skip to content

Commit 374db36

Browse files
authored
GH-50615: [C++] Reduce code generation for string kernels (#50616)
### Rationale for this change Some string kernels are generated twice for the corresponding binary and string type (for example binary/utf8, large_binary, large_utf8). Most of the time we can cut down on the code duplication to reduce compile times and binary size slightly. ### What changes are included in this PR? Only instantiate string kernels for base binary types, not the string derived types. This makes `libarrow_compute` somehow smaller on my system: * before: ```console $ size `find /build/ -name libarrow_compute.so.2600.0.0` text data bss dec hex filename 13551567 113096 46432 13711095 d136f7 /build/build-release/relwithdebinfo/libarrow_compute.so.2600.0.0 ``` * after: ```console $ size `find /build/ -name libarrow_compute.so.2600.0.0` text data bss dec hex filename 13363474 112128 44968 13520570 ce4eba /build/build-release/relwithdebinfo/libarrow_compute.so.2600.0.0 ``` ### Are these changes tested? Yes, including additional tests to check that utf8-ness of the input is still propagated correctly. ### Are there any user-facing changes? No. * GitHub Issue: #50615 Authored-by: Antoine Pitrou <antoine@python.org> Signed-off-by: Antoine Pitrou <antoine@python.org>
1 parent eb19b7f commit 374db36

9 files changed

Lines changed: 357 additions & 176 deletions

cpp/src/arrow/compute/kernels/codegen_internal.h

Lines changed: 8 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1314,10 +1314,14 @@ KernelType GenerateTypeAgnosticPrimitive(detail::GetTypeId get_id) {
13141314
}
13151315
}
13161316

1317-
// similar to GenerateTypeAgnosticPrimitive, but for base variable binary types
1318-
template <template <typename...> class Generator, typename KernelType = ArrayKernelExec,
1319-
typename... Args>
1320-
KernelType GenerateTypeAgnosticVarBinaryBase(detail::GetTypeId get_id) {
1317+
// Similar to GenerateTypeAgnosticPrimitive, but for base variable binary types
1318+
//
1319+
// Note that we don't offer to generate separate code for String types, because
1320+
// the utf8-ness of a type can be retrieved and handled efficiently at runtime.
1321+
// This helps cut down on code generation (see GH-50615).
1322+
template <template <typename...> class Generator, typename... Args>
1323+
auto GenerateTypeAgnosticVarBinaryBase(detail::GetTypeId get_id) {
1324+
using KernelType = decltype(&Generator<BinaryType, Args...>::Exec);
13211325
switch (get_id.id) {
13221326
case Type::BINARY:
13231327
case Type::STRING:
@@ -1331,24 +1335,6 @@ KernelType GenerateTypeAgnosticVarBinaryBase(detail::GetTypeId get_id) {
13311335
}
13321336
}
13331337

1334-
// Generate a kernel given a templated functor for binary and string types
1335-
template <template <typename...> class Generator, typename... Args>
1336-
ArrayKernelExec GenerateVarBinaryToVarBinary(detail::GetTypeId get_id) {
1337-
switch (get_id.id) {
1338-
case Type::BINARY:
1339-
return Generator<BinaryType, Args...>::Exec;
1340-
case Type::STRING:
1341-
return Generator<StringType, Args...>::Exec;
1342-
case Type::LARGE_BINARY:
1343-
return Generator<LargeBinaryType, Args...>::Exec;
1344-
case Type::LARGE_STRING:
1345-
return Generator<LargeStringType, Args...>::Exec;
1346-
default:
1347-
ARROW_DCHECK(false);
1348-
return nullptr;
1349-
}
1350-
}
1351-
13521338
// Generate a kernel given a templated functor for base binary types. Generates
13531339
// a single kernel for binary/string and large binary/large string. If your kernel
13541340
// implementation needs access to the specific type at compile time, please use
@@ -1370,24 +1356,6 @@ ArrayKernelExec GenerateVarBinaryBase(detail::GetTypeId get_id) {
13701356
}
13711357
}
13721358

1373-
// See BaseBinary documentation
1374-
template <template <typename...> class Generator, typename Type0, typename... Args>
1375-
ArrayKernelExec GenerateVarBinary(detail::GetTypeId get_id) {
1376-
switch (get_id.id) {
1377-
case Type::BINARY:
1378-
return Generator<Type0, BinaryType, Args...>::Exec;
1379-
case Type::STRING:
1380-
return Generator<Type0, StringType, Args...>::Exec;
1381-
case Type::LARGE_BINARY:
1382-
return Generator<Type0, LargeBinaryType, Args...>::Exec;
1383-
case Type::LARGE_STRING:
1384-
return Generator<Type0, LargeStringType, Args...>::Exec;
1385-
default:
1386-
ARROW_DCHECK(false);
1387-
return nullptr;
1388-
}
1389-
}
1390-
13911359
// Generate a kernel given a templated functor for binary-view types. Generates a
13921360
// single kernel for binary/string-view.
13931361
//

cpp/src/arrow/compute/kernels/scalar_compare.cc

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -825,8 +825,7 @@ std::shared_ptr<ScalarFunction> MakeScalarMinMax(std::string name, FunctionDoc d
825825
DCHECK_OK(func->AddKernel(std::move(kernel)));
826826
}
827827
for (const auto& ty : BaseBinaryTypes()) {
828-
auto exec =
829-
GenerateTypeAgnosticVarBinaryBase<BinaryScalarMinMax, ArrayKernelExec, Op>(ty);
828+
auto exec = GenerateTypeAgnosticVarBinaryBase<BinaryScalarMinMax, Op>(ty);
830829
ScalarKernel kernel{KernelSignature::Make({ty}, ty, /*is_varargs=*/true), exec,
831830
MinMaxState::Init};
832831
kernel.null_handling = NullHandling::COMPUTED_NO_PREALLOCATE;

cpp/src/arrow/compute/kernels/scalar_if_else.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1328,7 +1328,7 @@ void AddBinaryIfElseKernels(const std::shared_ptr<IfElseFunction>& scalar_functi
13281328
const std::vector<std::shared_ptr<DataType>>& types) {
13291329
for (auto&& type : types) {
13301330
auto exec =
1331-
internal::GenerateTypeAgnosticVarBinaryBase<ResolveIfElseExec, ArrayKernelExec,
1331+
internal::GenerateTypeAgnosticVarBinaryBase<ResolveIfElseExec,
13321332
/*AllocateMem=*/std::true_type>(
13331333
*type);
13341334
// cond array needs to be boolean always

cpp/src/arrow/compute/kernels/scalar_string_ascii.cc

Lines changed: 147 additions & 101 deletions
Large diffs are not rendered by default.

cpp/src/arrow/compute/kernels/scalar_string_internal.h

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
#include "arrow/compute/api_scalar.h"
2323
#include "arrow/compute/kernels/common_internal.h"
24+
#include "arrow/type_traits.h"
2425

2526
namespace arrow {
2627
namespace compute {
@@ -68,10 +69,13 @@ static int64_t GetVarBinaryValuesLength(const ArraySpan& span) {
6869
///
6970
/// and returns the number of codeunits of the `output` sequence or a negative
7071
/// value if an invalid input sequence is detected.
71-
template <typename Type, typename StringTransform>
72+
template <typename PhysicalType, typename StringTransform>
7273
struct StringTransformExecBase {
73-
using offset_type = typename Type::offset_type;
74-
using ArrayType = typename TypeTraits<Type>::ArrayType;
74+
using offset_type = typename PhysicalType::offset_type;
75+
using ArrayType = typename TypeTraits<PhysicalType>::ArrayType;
76+
77+
static_assert(!is_string_or_string_view(PhysicalType::type_id),
78+
"should only codegen on physical types");
7579

7680
static Status Execute(KernelContext* ctx, StringTransform* transform,
7781
const ExecSpan& batch, ExecResult* out) {
@@ -121,9 +125,11 @@ struct StringTransformExecBase {
121125
}
122126
};
123127

124-
template <typename Type, typename StringTransform>
125-
struct StringTransformExec : public StringTransformExecBase<Type, StringTransform> {
126-
using StringTransformExecBase<Type, StringTransform>::Execute;
128+
template <typename Type, typename StringTransform,
129+
typename PhysicalType = typename Type::PhysicalType>
130+
struct StringTransformExec
131+
: public StringTransformExecBase<PhysicalType, StringTransform> {
132+
using StringTransformExecBase<PhysicalType, StringTransform>::Execute;
127133

128134
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
129135
StringTransform transform;
@@ -132,11 +138,12 @@ struct StringTransformExec : public StringTransformExecBase<Type, StringTransfor
132138
}
133139
};
134140

135-
template <typename Type, typename StringTransform>
141+
template <typename Type, typename StringTransform,
142+
typename PhysicalType = typename Type::PhysicalType>
136143
struct StringTransformExecWithState
137-
: public StringTransformExecBase<Type, StringTransform> {
144+
: public StringTransformExecBase<PhysicalType, StringTransform> {
138145
using State = typename StringTransform::State;
139-
using StringTransformExecBase<Type, StringTransform>::Execute;
146+
using StringTransformExecBase<PhysicalType, StringTransform>::Execute;
140147

141148
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
142149
StringTransform transform(State::Get(ctx));
@@ -151,7 +158,7 @@ void MakeUnaryStringBatchKernel(
151158
MemAllocation::type mem_allocation = MemAllocation::PREALLOCATE) {
152159
auto func = std::make_shared<ScalarFunction>(name, Arity::Unary(), std::move(doc));
153160
for (const auto& ty : StringTypes()) {
154-
auto exec = GenerateVarBinaryToVarBinary<ExecFunctor>(ty);
161+
auto exec = GenerateTypeAgnosticVarBinaryBase<ExecFunctor>(ty);
155162
ScalarKernel kernel{{ty}, ty, std::move(exec)};
156163
kernel.mem_allocation = mem_allocation;
157164
ARROW_DCHECK_OK(func->AddKernel(std::move(kernel)));
@@ -216,6 +223,9 @@ static inline FunctionDoc StringClassifyDoc(std::string class_summary,
216223

217224
template <typename Type, typename Predicate>
218225
struct StringPredicateFunctor {
226+
static_assert(!is_string_or_string_view(Type::type_id),
227+
"should only codegen on physical types");
228+
219229
static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
220230
Status st = Status::OK();
221231
EnsureUtf8LookupTablesFilled();
@@ -237,7 +247,7 @@ void AddUnaryStringPredicate(std::string name, FunctionRegistry* registry,
237247
FunctionDoc doc) {
238248
auto func = std::make_shared<ScalarFunction>(name, Arity::Unary(), std::move(doc));
239249
for (const auto& ty : StringTypes()) {
240-
auto exec = GenerateVarBinaryToVarBinary<StringPredicateFunctor, Predicate>(ty);
250+
auto exec = GenerateTypeAgnosticVarBinaryBase<StringPredicateFunctor, Predicate>(ty);
241251
ARROW_DCHECK_OK(func->AddKernel({ty}, boolean(), std::move(exec)));
242252
}
243253
ARROW_DCHECK_OK(registry->AddFunction(std::move(func)));
@@ -281,7 +291,7 @@ struct ReplaceStringSliceTransformBase : public StringTransformBase {
281291
template <typename Options>
282292
struct StringSplitFinderBase {
283293
virtual ~StringSplitFinderBase() = default;
284-
virtual Status PreExec(const Options& options) { return Status::OK(); }
294+
virtual Status PreExec(const Options& options, bool is_utf8) { return Status::OK(); }
285295

286296
// Derived classes should also define these methods:
287297
// static bool Find(const uint8_t* begin, const uint8_t* end,
@@ -319,8 +329,9 @@ struct StringSplitExec {
319329
}
320330

321331
Status Execute(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) {
332+
const bool is_utf8 = is_string_or_string_view(batch[0].type()->id());
322333
SplitFinder finder;
323-
RETURN_NOT_OK(finder.PreExec(options));
334+
RETURN_NOT_OK(finder.PreExec(options, is_utf8));
324335
// TODO(wesm): refactor to not require creating ArrayData
325336
const ArrayType input(batch[0].array.ToArrayData());
326337

@@ -347,9 +358,11 @@ struct StringSplitExec {
347358
*list_offsets++ = static_cast<list_offset_type>(builder.length());
348359
}
349360
// Assign string array to list child data
350-
std::shared_ptr<Array> string_array;
351-
RETURN_NOT_OK(builder.Finish(&string_array));
352-
output_list->child_data.push_back(string_array->data());
361+
ARROW_ASSIGN_OR_RAISE(auto physical_array, builder.Finish());
362+
// We got the physical type (e.g. binary instead of utf8), need to patch it
363+
auto child_data = physical_array->data()->Copy();
364+
child_data->type = batch[0].type()->GetSharedPtr();
365+
output_list->child_data.push_back(std::move(child_data));
353366
return Status::OK();
354367
}
355368

0 commit comments

Comments
 (0)