Skip to content

ARROW-9156: [C++] Reducing the code size of the tensor module - #7539

Closed
mrkn wants to merge 22 commits into
apache:masterfrom
mrkn:ARROW-9156
Closed

ARROW-9156: [C++] Reducing the code size of the tensor module#7539
mrkn wants to merge 22 commits into
apache:masterfrom
mrkn:ARROW-9156

Conversation

@mrkn

@mrknmrkn commented Jun 25, 2020

Copy link
Copy Markdown
Member

To reduce the size of libarrow.so, I'd like to reduce the size of tensor and sparse tensor codes.

TODO:

  • Stop using template parameters in Tensor to SparseTensor converters
  • Stop using template parameters in SparseTensor to Tensor converters

@mrkn
mrkn marked this pull request as draft June 25, 2020 06:28
@github-actions

Copy link
Copy Markdown

@wesm

wesm commented Jun 25, 2020

Copy link
Copy Markdown
Member

This removes already more than 2MB of code from libarrow.so on Linux: great. I'll keep an eye on this

@mrkn
mrknforce-pushed the ARROW-9156 branch 2 times, most recently from af20396 to 1e54d34CompareJune 29, 2020 05:33
@mrkn
mrkn marked this pull request as ready for review June 29, 2020 06:48
@mrkn
mrkn requested a review from wesmJune 29, 2020 06:49
@mrkn

mrkn commented Jun 29, 2020

Copy link
Copy Markdown
MemberAuthor

@wesm Could you please review this?

@wesmwesm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this, this is definitely very beneficial both for compilation time and code size. I left some minor comments about reducing logic duplication in a few places by factoring some things out into a helper function but otherwise this looks good, and the existing test suite gives good evidence that things are working as expected

I'm sensitive to the possible performance issues that this refactor may introduce (by changing inline functions to C function calls or functions with switch statements in them but per my comments within I think the best approach would be to identify the performance problems (if any), write benchmarks to illustrate them, and then pursue optimizations on a case by case basis.

Comment threadcpp/src/arrow/tensor/csx_converter.cc Outdated
Comment threadcpp/src/arrow/tensor/csf_converter.cc Outdated
Comment threadcpp/src/arrow/tensor/coo_converter.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use an std::function for this, benchmarks would help show which is better. Either solution is going to be slower than the inlined version but if it is demonstrated to be a performance problem then we can revisit the template instantiation on a case by case basis to decide whether the tradeoff between code size and performance makes sense.

Additionally, in the future I think it could be beneficial to package the tensor support code in a libarrow_tensor in the future so that we could also "spin off" the tensor support into a separate pyarrow-tensor Python package so that users who do not need tensor support are not having to always carry around the compiled code that they may never use.

Comment threadcpp/src/arrow/tensor/csf_converter.cc Outdated
wesm
wesm previously approved these changes Jun 30, 2020

@wesmwesm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, modulo CI. Thank you for using the GetByteWidth helper function in so many places =)

@mrkn

mrkn commented Jun 30, 2020

Copy link
Copy Markdown
MemberAuthor

@wesm Is it better to work for benchmarking in other pull-request?

@wesm

wesm commented Jun 30, 2020

Copy link
Copy Markdown
Member

@mrkn it's up to you, it's fine with me if you work on performance tuning (or at least measurement) in another PR or this one

@mrkn

mrkn commented Jun 30, 2020

Copy link
Copy Markdown
MemberAuthor

@wesm OK. I continue to work for benchmarking in this pull-request. If I need more time to tune etc., I'll split the issue.

@wesm
wesm dismissed their stale reviewJune 30, 2020 01:26

Awaiting benchmarking

@mrkn
mrknforce-pushed the ARROW-9156 branch 2 times, most recently from ca93bc0 to bcc2ee3CompareJuly 1, 2020 08:58
@mrkn

mrkn commented Jul 1, 2020

Copy link
Copy Markdown
MemberAuthor

I wrote a benchmark code that measures the performance of conversion from Tensor to SparseTensor.
And I run this code with --repetitions=10 and got the following result.

  • Converting to SparseCOOTensor is 1.4x-1.8x slower than the original.
  • Converting to SparseCSRMatrix and SparseCSCMatrix is 1.0x-1.2x faster than the original.
  • Converting to SparseCSFTensor is 1.0-1.3x slower than the original.

I don't think this result, especially SparseCOOTensor's case can be acceptable.
Now I'm trying to resolve this performance regression.

The full result is shown below:
FormatIndexTypeValueTypeChange %BaselineContender
COOInt32TypeInt856.4262459.3970903847.138775
COOInt16TypeInt843.0622619.8480283748.014603
COOInt64TypeInt840.1372850.8261363995.057821
COOInt8TypeInt835.4782776.7166943761.850287
COOInt16TypeInt1666.8122394.0119283993.489611
COOInt32TypeInt1659.2812728.5773374346.096797
COOInt64TypeInt1652.0432596.4040053947.663313
COOInt8TypeInt1644.5102768.5679924000.853968
COOInt32TypeFloat93.2832287.3432914421.039474
COOInt16TypeFloat71.1002577.6895694410.437858
COOInt64TypeFloat61.0352821.3009964543.277655
COOInt8TypeFloat53.3032666.8015594088.291362
COOInt64TypeDouble88.2302616.0007384924.096764
COOInt16TypeDouble70.1882698.3354954592.253023
COOInt32TypeDouble69.3682579.7539024369.267270
COOInt8TypeDouble62.7812747.1798064471.895533
CSRInt8TypeInt8-1.6604626.1817914549.389657
CSRInt32TypeInt8-4.7184708.0445564485.938002
CSRInt64TypeInt8-4.9054687.9611324458.024004
CSRInt16TypeInt8-7.1644851.8034794504.201161
CSRInt32TypeInt16-1.4814722.6767954652.711030
CSRInt64TypeInt16-5.8534573.1243144305.462839
CSRInt8TypeInt16-11.8904631.2903894080.628990
CSRInt16TypeInt16-12.3724737.3206004151.197839
CSRInt64TypeFloat-4.9994711.5755654476.032868
CSRInt16TypeFloat-5.0624811.4570374567.909440
CSRInt32TypeFloat-7.6514814.8130844446.432341
CSRInt8TypeFloat-12.1965158.8683304529.676101
CSRInt32TypeDouble0.3844631.0055064648.789489
CSRInt64TypeDouble-0.2734816.6697654803.522209
CSRInt8TypeDouble-3.4534646.6527744486.217718
CSRInt16TypeDouble-8.9634952.0321104508.161865
CSCInt8TypeInt8-4.2074871.7933384666.848429
CSCInt64TypeInt8-10.5824571.2516134087.509465
CSCInt16TypeInt8-13.1894865.6662954223.934008
CSCInt32TypeInt8-16.5464999.5775064172.360711
CSCInt32TypeInt16-0.6194842.5157154812.531287
CSCInt8TypeInt16-4.1284689.7372354496.135413
CSCInt16TypeInt16-8.1104643.4134414266.836346
CSCInt64TypeInt16-10.3024971.8794494459.696887
CSCInt8TypeFloat-0.5474694.3479704668.667817
CSCInt32TypeFloat-0.6194718.2075934688.994670
CSCInt64TypeFloat-3.3604847.7538934684.849545
CSCInt16TypeFloat-7.4084956.7953484589.607722
CSCInt32TypeDouble0.4914917.1727604941.321519
CSCInt64TypeDouble-0.7545069.5892905031.376333
CSCInt16TypeDouble-2.0234762.4259104666.071367
CSCInt8TypeDouble-5.9874934.0162684638.636384
CSFInt16TypeInt825.3947885.9863129888.571309
CSFInt64TypeInt822.6059083.44455911136.716151
CSFInt32TypeInt813.4868228.3770649338.031561
CSFInt8TypeInt811.9007687.3479678602.168534
CSFInt64TypeInt1628.0948247.25577210564.280045
CSFInt8TypeInt1620.2768172.4114869829.454135
CSFInt32TypeInt1615.2408776.21372710113.751856
CSFInt16TypeInt1613.7078396.3223269547.223214
CSFInt16TypeFloat25.9898340.99645910508.758442
CSFInt32TypeFloat20.5628882.07529910708.400993
CSFInt8TypeFloat19.0218507.16002510125.299339
CSFInt64TypeFloat1.7699657.0003239827.819671
CSFInt8TypeDouble20.3828380.18378210088.211026
CSFInt32TypeDouble14.3539512.40369610877.711426
CSFInt16TypeDouble13.7378536.2699319708.870939
CSFInt64TypeDouble12.4139737.55521710946.296736

@mrkn
mrknforce-pushed the ARROW-9156 branch 3 times, most recently from ef7781b to 59df077CompareJuly 2, 2020 05:42
@wesm

wesm commented Jul 2, 2020

Copy link
Copy Markdown
Member

@mrkn to improve the benchmark usefulness I would recommend increasing the size of the data being processed. I ran them locally and the COO benchmarks all run in less than 10 microseconds (some close to 1 microsecond) and at that speed things like destructors show up as non-trivial overhead

+ 91.29% 0.50% arrow-tensor-co arrow-tensor-conversion-benchmark [.] arrow::Int8RowMajorTensorConversionFixture_ConvertToSparseCOOTensorInt32_Benchmark::BenchmarkCase
+ 91.24% 0.00% arrow-tensor-co arrow-tensor-conversion-benchmark [.] _start
+ 91.24% 0.00% arrow-tensor-co libc-2.27.so [.] __libc_start_main
+ 91.24% 0.00% arrow-tensor-co arrow-tensor-conversion-benchmark [.] main
+ 91.24% 0.00% arrow-tensor-co arrow-tensor-conversion-benchmark [.] benchmark::RunSpecifiedBenchmarks
+ 79.50% 3.05% arrow-tensor-co arrow-tensor-conversion-benchmark [.] arrow::SparseTensorImpl<arrow::SparseCOOIndex>::Make
+ 74.47% 2.29% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::internal::MakeSparseCOOTensorFromTensor
+ 74.47% 0.04% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::internal::MakeSparseTensorFromTensor
+ 74.33% 16.65% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::internal::(anonymous namespace)::SparseCOOTensorConverter::Convert
+ 25.47% 25.47% arrow-tensor-co libc-2.27.so [.] __memmove_avx_unaligned_erms
+ 12.64% 7.00% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::internal::ComputeRowMajorStrides
+ 11.35% 1.87% arrow-tensor-co arrow-tensor-conversion-benchmark [.] arrow::SparseTensor::~SparseTensor
+ 9.15% 0.26% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::internal::IsTensorStridesContiguous
+ 7.76% 2.52% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::Tensor::CountNonZero
+ 7.25% 0.57% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::AllocateBuffer
+ 6.99% 6.99% arrow-tensor-co libc-2.27.so [.] cfree@GLIBC_2.2.5
+ 6.99% 0.00% arrow-tensor-co libc-2.27.so [.] __GI___libc_free (inlined)
+ 6.76% 1.07% arrow-tensor-co libstdc++.so.6.0.27 [.] operator new
+ 6.03% 0.00% arrow-tensor-co libc-2.27.so [.] __GI___libc_malloc (inlined)
+ 5.73% 1.71% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::PoolBuffer::~PoolBuffer
+ 5.42% 1.35% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::SparseCOOIndex::SparseCOOIndex
+ 4.99% 0.41% arrow-tensor-co libarrow.so.100.0.0 [.] arrow::SparseCOOIndex::~SparseCOOIndex

@wesm

wesm commented Jul 3, 2020

Copy link
Copy Markdown
Member

What do you think about pursuing the performance optimization work as a follow up?

@mrkn

mrkn commented Jul 3, 2020

Copy link
Copy Markdown
MemberAuthor

@wesm I decided to separate a pull-request for performance optimization because I may need some days to get this work done.
I'll make a new ticket for optimization, and clean up this pull-request.

@wesm

wesm commented Jul 3, 2020

Copy link
Copy Markdown
Member

OK, sounds good, let me know when this is ready to be merged

@mrkn
mrknforce-pushed the ARROW-9156 branch 2 times, most recently from 732e087 to 5523588CompareJuly 4, 2020 01:31
wesm
wesm approved these changes Jul 5, 2020

@wesmwesm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, CI is looking OK now. Thanks @mrkn!

@wesmwesm closed this in 16b2a44Jul 5, 2020
@mrkn
mrkn deleted the ARROW-9156 branch July 5, 2020 16:48
mrkn added a commit to mrkn/arrow that referenced this pull request Jul 10, 2020
This change improves the conversion speed for all the cases of row-major
and column-major tensors. For strided tensors, all the cases are improved
except for the combination of int16 value and less than 32-bit index.
The result from `archery benchmark diff` command is below, the baseline is
the commit 8f96d1d (before merging apache#7539) and the contender is this commit:
```
benchmark baseline contender change %
43 Int16StridedTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 141564.498765 182313.374077 28.785
10 Int16StridedTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 140420.265077 153715.618715 9.468
42 Int16StridedTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 167601.944005 170626.538009 1.805
37 Int16StridedTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 143722.048451 141928.779114 -1.248
27 Int8StridedTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 169947.630903 164423.055008 -3.251
24 Int8StridedTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 170153.324442 163898.373534 -3.676
45 Int8StridedTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 170883.542468 164131.618700 -3.951
35 Int8StridedTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 171015.028153 163516.191034 -4.385
9 DoubleStridedTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 200974.675587 191956.688874 -4.487
18 FloatStridedTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 192320.819787 182941.130595 -4.877
12 DoubleStridedTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 175198.892973 166417.452194 -5.012
30 FloatStridedTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 167174.764713 151431.022906 -9.418
29 DoubleStridedTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 173925.990981 157142.110096 -9.650
16 FloatStridedTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 167877.497573 151666.610814 -9.656
26 FloatStridedTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 169705.312801 151885.952803 -10.500
6 DoubleStridedTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 177394.661870 156019.301906 -12.050
5 Int16RowMajorTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 107592.839089 66069.770737 -38.593
41 Int16ColumnMajorTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 114841.700196 68707.073774 -40.172
47 Int16RowMajorTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 107304.436017 63922.898636 -40.428
4 FloatRowMajorTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 112315.965200 66577.854744 -40.723
21 Int16ColumnMajorTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 115090.317912 66527.852021 -42.195
17 FloatColumnMajorTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 121583.540341 70025.614174 -42.405
3 DoubleRowMajorTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 116946.572632 66411.338694 -43.212
15 FloatRowMajorTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 112275.805149 63264.226406 -43.653
13 FloatColumnMajorTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 122085.596559 66569.027159 -45.473
34 Int16RowMajorTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 109888.801628 58860.826009 -46.436
20 Int16ColumnMajorTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 117648.480324 62574.709433 -46.812
19 Int8ColumnMajorTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 137444.576787 71969.132261 -47.638
28 DoubleRowMajorTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 119527.435615 61405.371141 -48.627
40 FloatRowMajorTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 115130.821188 58664.779831 -49.045
39 Int8ColumnMajorTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 137053.503574 69755.112894 -49.104
22 Int8RowMajorTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 136645.576795 69303.266896 -49.282
23 FloatColumnMajorTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 124100.575779 61723.051518 -50.264
31 DoubleColumnMajorTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 140278.467902 69584.530347 -50.395
1 Int16RowMajorTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 135770.669563 67151.922438 -50.540
44 Int16ColumnMajorTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 142625.928542 70315.759868 -50.699
2 Int8ColumnMajorTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 137443.030096 67752.813535 -50.705
46 Int8RowMajorTensorConversionFixture<Int16Type>/ConvertToSparseCOOTensorInt16 135961.160225 66613.351871 -51.006
11 DoubleColumnMajorTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 138857.793332 67315.714410 -51.522
8 FloatRowMajorTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 138992.703542 66847.061004 -51.906
7 Int8RowMajorTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 136298.424804 64520.497064 -52.662
36 FloatColumnMajorTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 149706.883716 69805.958679 -53.372
33 DoubleRowMajorTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 143460.582904 66870.585026 -53.387
38 DoubleColumnMajorTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 138220.367601 64425.776453 -53.389
14 DoubleRowMajorTensorConversionFixture<Int32Type>/ConvertToSparseCOOTensorInt32 136707.421042 63624.050357 -53.460
25 Int8ColumnMajorTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 137303.219403 62528.740787 -54.459
32 Int8RowMajorTensorConversionFixture<Int64Type>/ConvertToSparseCOOTensorInt64 136551.052565 58743.141699 -56.981
0 DoubleColumnMajorTensorConversionFixture<Int8Type>/ConvertToSparseCOOTensorInt8 162895.437265 69676.279783 -57.226
```
wesm pushed a commit that referenced this pull request Jul 11, 2020
…onversion
In this pull-request, the slowing down of the conversion introduced in #7539 is canceled, and the conversion speed is improved than before #7539 in some cases.
Closes#7643 from mrkn/ARROW-9331
Authored-by: Kenta Murata <mrkn@mrkn.jp>
Signed-off-by: Wes McKinney <wesm@apache.org>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mrkn@wesm