You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/06/25 06:28:15 UTC
[GitHub] [arrow] mrkn opened a new pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
mrkn opened a new pull request #7539:
URL: https://github.com/apache/arrow/pull/7539
To reduce the size of libarrow.so, I'd like to reduce the size of tensor and sparse tensor codes.
TODO:
- [x] Stop using template parameters in Tensor to SparseTensor converters
- [ ] Stop using template parameters in SparseTensor to Tensor converters
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-651458470
OK. I continue to work for benchmarking in this pull-request. If I need more time to tune etc., I'll split the issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-653266493
What do you think about pursuing the performance optimization work as a follow up?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-651456329
@wesm Is it better to work for benchmarking in other pull-request?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-653560364
OK, sounds good, let me know when this is ready to be merged
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#discussion_r447030010
##########
File path: cpp/src/arrow/tensor/csf_converter.cc
##########
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector<int64_t>& coord, const std::vector<int64_
// ----------------------------------------------------------------------
// SparseTensorConverter for SparseCSFIndex
-template <typename TYPE>
-class SparseCSFTensorConverter {
- public:
- using NumericTensorType = NumericTensor<TYPE>;
- using value_type = typename NumericTensorType::value_type;
+class SparseCSFTensorConverter : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::AssignIndex;
+ using SparseTensorConverterMixin::IsNonZero;
- SparseCSFTensorConverter(const NumericTensorType& tensor,
+ public:
+ SparseCSFTensorConverter(const Tensor& tensor,
const std::shared_ptr<DataType>& index_value_type,
MemoryPool* pool)
: tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
- template <typename IndexValueType>
Status Convert() {
- using c_index_value_type = typename IndexValueType::c_type;
- RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits<c_index_value_type>::max()));
+ RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, tensor_.shape()));
+
+ const int index_elsize =
+ checked_cast<const IntegerType&>(*index_value_type_).bit_width() / CHAR_BIT;
+ const int value_elsize =
+ checked_cast<const FixedWidthType&>(*tensor_.type()).bit_width() / CHAR_BIT;
Review comment:
Could be helper function, maybe we can put an `internal::GetByteWidth` function in `arrow/type.h` that does this?
##########
File path: cpp/src/arrow/tensor/csx_converter.cc
##########
@@ -0,0 +1,245 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/tensor/converter.h"
+
+#include <cstdint>
+#include <limits>
+#include <memory>
+#include <vector>
+
+#include "arrow/buffer.h"
+#include "arrow/status.h"
+#include "arrow/type.h"
+#include "arrow/util/checked_cast.h"
+#include "arrow/visitor_inline.h"
+
+namespace arrow {
+
+class MemoryPool;
+
+namespace internal {
+namespace {
+
+// ----------------------------------------------------------------------
+// SparseTensorConverter for SparseCSRIndex
+
+class SparseCSXMatrixConverter : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::AssignIndex;
+ using SparseTensorConverterMixin::IsNonZero;
+
+ public:
+ SparseCSXMatrixConverter(SparseMatrixCompressedAxis axis, const Tensor& tensor,
+ const std::shared_ptr<DataType>& index_value_type,
+ MemoryPool* pool)
+ : axis_(axis), tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
+
+ Status Convert() {
+ RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, tensor_.shape()));
+
+ const int index_elsize =
+ checked_cast<const IntegerType&>(*index_value_type_).bit_width() / CHAR_BIT;
Review comment:
Can you factor this "get byte width" operation into a helper function?
##########
File path: cpp/src/arrow/tensor/csf_converter.cc
##########
@@ -148,84 +161,130 @@ class SparseCSFTensorConverter {
return Status::OK();
}
-#define CALL_TYPE_SPECIFIC_CONVERT(TYPE_CLASS) \
- case TYPE_CLASS##Type::type_id: \
- return Convert<TYPE_CLASS##Type>();
-
- Status Convert() {
- switch (index_value_type_->id()) {
- ARROW_GENERATE_FOR_ALL_INTEGER_TYPES(CALL_TYPE_SPECIFIC_CONVERT);
- // LCOV_EXCL_START: The following invalid causes program failure.
- default:
- return Status::TypeError("Unsupported SparseTensor index value type");
- // LCOV_EXCL_STOP
- }
- }
-
-#undef CALL_TYPE_SPECIFIC_CONVERT
-
std::shared_ptr<SparseCSFIndex> sparse_index;
std::shared_ptr<Buffer> data;
private:
- const NumericTensorType& tensor_;
+ const Tensor& tensor_;
const std::shared_ptr<DataType>& index_value_type_;
MemoryPool* pool_;
+};
- template <typename c_value_type>
- inline Status CheckMaximumValue(const c_value_type type_max) const {
- auto max_dimension =
- *std::max_element(tensor_.shape().begin(), tensor_.shape().end());
- if (static_cast<int64_t>(type_max) < max_dimension) {
- // LCOV_EXCL_START: The following invalid causes program failure.
- return Status::Invalid("The bit width of the index value type is too small");
- // LCOV_EXCL_STOP
- }
- return Status::OK();
+class TensorBuilderFromSparseCSFTensor : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::GetIndexValue;
+
+ MemoryPool* pool_;
+ const SparseCSFTensor* sparse_tensor_;
+ const SparseCSFIndex* sparse_index_;
+ const std::vector<std::shared_ptr<Tensor>>& indptr_;
+ const std::vector<std::shared_ptr<Tensor>>& indices_;
+ const std::vector<int64_t>& axis_order_;
+ const std::vector<int64_t>& shape_;
+ const int64_t non_zero_length_;
+ const int ndim_;
+ const int64_t tensor_size_;
+ const FixedWidthType& value_type_;
+ const int value_elsize_;
+ const uint8_t* raw_data_;
+ std::vector<int64_t> strides_;
+ std::shared_ptr<Buffer> values_buffer_;
+ uint8_t* values_;
+
+ public:
+ TensorBuilderFromSparseCSFTensor(const SparseCSFTensor* sparse_tensor, MemoryPool* pool)
+ : pool_(pool),
+ sparse_tensor_(sparse_tensor),
+ sparse_index_(
+ checked_cast<const SparseCSFIndex*>(sparse_tensor->sparse_index().get())),
+ indptr_(sparse_index_->indptr()),
+ indices_(sparse_index_->indices()),
+ axis_order_(sparse_index_->axis_order()),
+ shape_(sparse_tensor->shape()),
+ non_zero_length_(sparse_tensor->non_zero_length()),
+ ndim_(sparse_tensor->ndim()),
+ tensor_size_(sparse_tensor->size()),
+ value_type_(checked_cast<const FixedWidthType&>(*sparse_tensor->type())),
+ value_elsize_(value_type_.bit_width() / CHAR_BIT),
+ raw_data_(sparse_tensor->raw_data()) {}
+
+ int ElementSize(const std::shared_ptr<Tensor>& tensor) const {
+ return checked_cast<const FixedWidthType&>(*tensor->type()).bit_width() / CHAR_BIT;
Review comment:
helper function
##########
File path: cpp/src/arrow/tensor/coo_converter.cc
##########
@@ -51,133 +51,185 @@ inline void IncrementIndex(std::vector<int64_t>& coord,
// ----------------------------------------------------------------------
// SparseTensorConverter for SparseCOOIndex
-template <typename TYPE>
-class SparseCOOTensorConverter {
- public:
- using NumericTensorType = NumericTensor<TYPE>;
- using value_type = typename NumericTensorType::value_type;
+class SparseCOOTensorConverter : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::AssignIndex;
+ using SparseTensorConverterMixin::IsNonZero;
- SparseCOOTensorConverter(const NumericTensorType& tensor,
+ public:
+ SparseCOOTensorConverter(const Tensor& tensor,
const std::shared_ptr<DataType>& index_value_type,
MemoryPool* pool)
: tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
- template <typename IndexValueType>
Status Convert() {
- using c_index_value_type = typename IndexValueType::c_type;
- const int64_t indices_elsize = sizeof(c_index_value_type);
+ RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, tensor_.shape()));
+
+ const int index_elsize =
+ checked_cast<const IntegerType&>(*index_value_type_).bit_width() / CHAR_BIT;
+ const int value_elsize =
+ checked_cast<const FixedWidthType&>(*tensor_.type()).bit_width() / CHAR_BIT;
const int64_t ndim = tensor_.ndim();
- int64_t nonzero_count = -1;
- RETURN_NOT_OK(tensor_.CountNonZero(&nonzero_count));
+ ARROW_ASSIGN_OR_RAISE(int64_t nonzero_count, tensor_.CountNonZero());
ARROW_ASSIGN_OR_RAISE(auto indices_buffer,
- AllocateBuffer(indices_elsize * ndim * nonzero_count, pool_));
- c_index_value_type* indices =
- reinterpret_cast<c_index_value_type*>(indices_buffer->mutable_data());
+ AllocateBuffer(index_elsize * ndim * nonzero_count, pool_));
+ uint8_t* indices = indices_buffer->mutable_data();
ARROW_ASSIGN_OR_RAISE(auto values_buffer,
- AllocateBuffer(sizeof(value_type) * nonzero_count, pool_));
- value_type* values = reinterpret_cast<value_type*>(values_buffer->mutable_data());
+ AllocateBuffer(value_elsize * nonzero_count, pool_));
+ uint8_t* values = values_buffer->mutable_data();
+ const uint8_t* tensor_data = tensor_.raw_data();
if (ndim <= 1) {
- const value_type* data = reinterpret_cast<const value_type*>(tensor_.raw_data());
const int64_t count = ndim == 0 ? 1 : tensor_.shape()[0];
- for (int64_t i = 0; i < count; ++i, ++data) {
- if (*data != 0) {
- *indices++ = static_cast<c_index_value_type>(i);
- *values++ = *data;
+ for (int64_t i = 0; i < count; ++i) {
+ if (std::any_of(tensor_data, tensor_data + value_elsize, IsNonZero)) {
+ AssignIndex(indices, i, index_elsize);
+ std::copy_n(tensor_data, value_elsize, values);
+
+ indices += index_elsize;
+ values += value_elsize;
}
+ tensor_data += value_elsize;
}
} else {
const std::vector<int64_t>& shape = tensor_.shape();
std::vector<int64_t> coord(ndim, 0); // The current logical coordinates
for (int64_t n = tensor_.size(); n > 0; n--) {
- const value_type x = tensor_.Value(coord);
- if (tensor_.Value(coord) != 0) {
- *values++ = x;
+ int64_t offset = tensor_.CalculateValueOffset(coord);
+ if (std::any_of(tensor_data + offset, tensor_data + offset + value_elsize,
+ IsNonZero)) {
+ std::copy_n(tensor_data + offset, value_elsize, values);
+ values += value_elsize;
+
// Write indices in row-major order.
for (int64_t i = 0; i < ndim; ++i) {
- *indices++ = static_cast<c_index_value_type>(coord[i]);
+ AssignIndex(indices, coord[i], index_elsize);
+ indices += index_elsize;
}
}
+
IncrementIndex(coord, shape);
}
}
// make results
const std::vector<int64_t> indices_shape = {nonzero_count, ndim};
- const std::vector<int64_t> indices_strides = {indices_elsize * ndim, indices_elsize};
+ const std::vector<int64_t> indices_strides = {index_elsize * ndim, index_elsize};
sparse_index = std::make_shared<SparseCOOIndex>(std::make_shared<Tensor>(
index_value_type_, std::move(indices_buffer), indices_shape, indices_strides));
data = std::move(values_buffer);
return Status::OK();
}
-#define CALL_TYPE_SPECIFIC_CONVERT(TYPE_CLASS) \
- case TYPE_CLASS##Type::type_id: \
- return Convert<TYPE_CLASS##Type>();
-
- Status Convert() {
- switch (index_value_type_->id()) {
- ARROW_GENERATE_FOR_ALL_INTEGER_TYPES(CALL_TYPE_SPECIFIC_CONVERT);
- // LCOV_EXCL_START: The following invalid causes program failure.
- default:
- return Status::TypeError("Unsupported SparseTensor index value type");
- // LCOV_EXCL_STOP
- }
- }
-
-#undef CALL_TYPE_SPECIFIC_CONVERT
-
std::shared_ptr<SparseCOOIndex> sparse_index;
std::shared_ptr<Buffer> data;
private:
- const NumericTensorType& tensor_;
+ const Tensor& tensor_;
const std::shared_ptr<DataType>& index_value_type_;
MemoryPool* pool_;
};
-template <typename TYPE>
+} // namespace
+
+void SparseTensorConverterMixin::AssignIndex(uint8_t* indices, int64_t val,
+ const int elsize) {
+ switch (elsize) {
+ case 1:
+ *indices = static_cast<uint8_t>(val);
+ break;
+ case 2:
+ *reinterpret_cast<uint16_t*>(indices) = static_cast<uint16_t>(val);
+ break;
+ case 4:
+ *reinterpret_cast<uint32_t*>(indices) = static_cast<uint32_t>(val);
+ break;
+ case 8:
+ *reinterpret_cast<int64_t*>(indices) = val;
+ break;
+ default:
+ break;
+ }
+}
+
+int64_t SparseTensorConverterMixin::GetIndexValue(const uint8_t* value_ptr,
+ const int elsize) {
+ switch (elsize) {
+ case 1:
+ return *value_ptr;
+
+ case 2:
+ return *reinterpret_cast<const uint16_t*>(value_ptr);
+
+ case 4:
+ return *reinterpret_cast<const uint32_t*>(value_ptr);
+
+ case 8:
+ return *reinterpret_cast<const int64_t*>(value_ptr);
+
+ default:
+ return 0;
+ }
+}
+
Status MakeSparseCOOTensorFromTensor(const Tensor& tensor,
const std::shared_ptr<DataType>& index_value_type,
MemoryPool* pool,
std::shared_ptr<SparseIndex>* out_sparse_index,
std::shared_ptr<Buffer>* out_data) {
- NumericTensor<TYPE> numeric_tensor(tensor.data(), tensor.shape(), tensor.strides());
- SparseCOOTensorConverter<TYPE> converter(numeric_tensor, index_value_type, pool);
+ SparseCOOTensorConverter converter(tensor, index_value_type, pool);
RETURN_NOT_OK(converter.Convert());
*out_sparse_index = checked_pointer_cast<SparseIndex>(converter.sparse_index);
*out_data = converter.data;
return Status::OK();
}
-} // namespace
-
-#define MAKE_SPARSE_TENSOR_FROM_TENSOR(TYPE_CLASS) \
- case TYPE_CLASS##Type::type_id: \
- return MakeSparseCOOTensorFromTensor<TYPE_CLASS##Type>( \
- tensor, index_value_type, pool, out_sparse_index, out_data);
+Result<std::shared_ptr<Tensor>> MakeTensorFromSparseCOOTensor(
+ MemoryPool* pool, const SparseCOOTensor* sparse_tensor) {
+ const auto& sparse_index =
+ checked_cast<const SparseCOOIndex&>(*sparse_tensor->sparse_index());
+ const auto& coords = sparse_index.indices();
+ const auto* coords_data = coords->raw_data();
+
+ const auto& index_type = checked_cast<const FixedWidthType&>(*coords->type());
+ const int index_elsize = index_type.bit_width() / CHAR_BIT;
+
+ const auto& value_type = checked_cast<const FixedWidthType&>(*sparse_tensor->type());
+ const int value_elsize = value_type.bit_width() / CHAR_BIT;
Review comment:
See comments re: having a helper function below
##########
File path: cpp/src/arrow/tensor/coo_converter.cc
##########
@@ -51,133 +51,185 @@ inline void IncrementIndex(std::vector<int64_t>& coord,
// ----------------------------------------------------------------------
// SparseTensorConverter for SparseCOOIndex
-template <typename TYPE>
-class SparseCOOTensorConverter {
- public:
- using NumericTensorType = NumericTensor<TYPE>;
- using value_type = typename NumericTensorType::value_type;
+class SparseCOOTensorConverter : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::AssignIndex;
+ using SparseTensorConverterMixin::IsNonZero;
- SparseCOOTensorConverter(const NumericTensorType& tensor,
+ public:
+ SparseCOOTensorConverter(const Tensor& tensor,
const std::shared_ptr<DataType>& index_value_type,
MemoryPool* pool)
: tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
- template <typename IndexValueType>
Status Convert() {
- using c_index_value_type = typename IndexValueType::c_type;
- const int64_t indices_elsize = sizeof(c_index_value_type);
+ RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, tensor_.shape()));
+
+ const int index_elsize =
+ checked_cast<const IntegerType&>(*index_value_type_).bit_width() / CHAR_BIT;
+ const int value_elsize =
+ checked_cast<const FixedWidthType&>(*tensor_.type()).bit_width() / CHAR_BIT;
const int64_t ndim = tensor_.ndim();
- int64_t nonzero_count = -1;
- RETURN_NOT_OK(tensor_.CountNonZero(&nonzero_count));
+ ARROW_ASSIGN_OR_RAISE(int64_t nonzero_count, tensor_.CountNonZero());
ARROW_ASSIGN_OR_RAISE(auto indices_buffer,
- AllocateBuffer(indices_elsize * ndim * nonzero_count, pool_));
- c_index_value_type* indices =
- reinterpret_cast<c_index_value_type*>(indices_buffer->mutable_data());
+ AllocateBuffer(index_elsize * ndim * nonzero_count, pool_));
+ uint8_t* indices = indices_buffer->mutable_data();
ARROW_ASSIGN_OR_RAISE(auto values_buffer,
- AllocateBuffer(sizeof(value_type) * nonzero_count, pool_));
- value_type* values = reinterpret_cast<value_type*>(values_buffer->mutable_data());
+ AllocateBuffer(value_elsize * nonzero_count, pool_));
+ uint8_t* values = values_buffer->mutable_data();
+ const uint8_t* tensor_data = tensor_.raw_data();
if (ndim <= 1) {
- const value_type* data = reinterpret_cast<const value_type*>(tensor_.raw_data());
const int64_t count = ndim == 0 ? 1 : tensor_.shape()[0];
- for (int64_t i = 0; i < count; ++i, ++data) {
- if (*data != 0) {
- *indices++ = static_cast<c_index_value_type>(i);
- *values++ = *data;
+ for (int64_t i = 0; i < count; ++i) {
+ if (std::any_of(tensor_data, tensor_data + value_elsize, IsNonZero)) {
+ AssignIndex(indices, i, index_elsize);
+ std::copy_n(tensor_data, value_elsize, values);
+
+ indices += index_elsize;
+ values += value_elsize;
}
+ tensor_data += value_elsize;
}
} else {
const std::vector<int64_t>& shape = tensor_.shape();
std::vector<int64_t> coord(ndim, 0); // The current logical coordinates
for (int64_t n = tensor_.size(); n > 0; n--) {
- const value_type x = tensor_.Value(coord);
- if (tensor_.Value(coord) != 0) {
- *values++ = x;
+ int64_t offset = tensor_.CalculateValueOffset(coord);
+ if (std::any_of(tensor_data + offset, tensor_data + offset + value_elsize,
+ IsNonZero)) {
+ std::copy_n(tensor_data + offset, value_elsize, values);
+ values += value_elsize;
+
// Write indices in row-major order.
for (int64_t i = 0; i < ndim; ++i) {
- *indices++ = static_cast<c_index_value_type>(coord[i]);
+ AssignIndex(indices, coord[i], index_elsize);
+ indices += index_elsize;
}
}
+
IncrementIndex(coord, shape);
}
}
// make results
const std::vector<int64_t> indices_shape = {nonzero_count, ndim};
- const std::vector<int64_t> indices_strides = {indices_elsize * ndim, indices_elsize};
+ const std::vector<int64_t> indices_strides = {index_elsize * ndim, index_elsize};
sparse_index = std::make_shared<SparseCOOIndex>(std::make_shared<Tensor>(
index_value_type_, std::move(indices_buffer), indices_shape, indices_strides));
data = std::move(values_buffer);
return Status::OK();
}
-#define CALL_TYPE_SPECIFIC_CONVERT(TYPE_CLASS) \
- case TYPE_CLASS##Type::type_id: \
- return Convert<TYPE_CLASS##Type>();
-
- Status Convert() {
- switch (index_value_type_->id()) {
- ARROW_GENERATE_FOR_ALL_INTEGER_TYPES(CALL_TYPE_SPECIFIC_CONVERT);
- // LCOV_EXCL_START: The following invalid causes program failure.
- default:
- return Status::TypeError("Unsupported SparseTensor index value type");
- // LCOV_EXCL_STOP
- }
- }
-
-#undef CALL_TYPE_SPECIFIC_CONVERT
-
std::shared_ptr<SparseCOOIndex> sparse_index;
std::shared_ptr<Buffer> data;
private:
- const NumericTensorType& tensor_;
+ const Tensor& tensor_;
const std::shared_ptr<DataType>& index_value_type_;
MemoryPool* pool_;
};
-template <typename TYPE>
+} // namespace
+
+void SparseTensorConverterMixin::AssignIndex(uint8_t* indices, int64_t val,
+ const int elsize) {
+ switch (elsize) {
+ case 1:
+ *indices = static_cast<uint8_t>(val);
+ break;
+ case 2:
+ *reinterpret_cast<uint16_t*>(indices) = static_cast<uint16_t>(val);
+ break;
+ case 4:
+ *reinterpret_cast<uint32_t*>(indices) = static_cast<uint32_t>(val);
+ break;
+ case 8:
+ *reinterpret_cast<int64_t*>(indices) = val;
+ break;
+ default:
+ break;
+ }
+}
+
+int64_t SparseTensorConverterMixin::GetIndexValue(const uint8_t* value_ptr,
+ const int elsize) {
+ switch (elsize) {
+ case 1:
+ return *value_ptr;
+
+ case 2:
+ return *reinterpret_cast<const uint16_t*>(value_ptr);
+
+ case 4:
+ return *reinterpret_cast<const uint32_t*>(value_ptr);
+
+ case 8:
+ return *reinterpret_cast<const int64_t*>(value_ptr);
+
+ default:
+ return 0;
+ }
+}
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm closed pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm closed pull request #7539:
URL: https://github.com/apache/arrow/pull/7539
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-650967222
@wesm Could you please review this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-649256419
https://issues.apache.org/jira/browse/ARROW-9156
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on a change in pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#discussion_r447259489
##########
File path: cpp/src/arrow/tensor/csf_converter.cc
##########
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector<int64_t>& coord, const std::vector<int64_
// ----------------------------------------------------------------------
// SparseTensorConverter for SparseCSFIndex
-template <typename TYPE>
-class SparseCSFTensorConverter {
- public:
- using NumericTensorType = NumericTensor<TYPE>;
- using value_type = typename NumericTensorType::value_type;
+class SparseCSFTensorConverter : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::AssignIndex;
+ using SparseTensorConverterMixin::IsNonZero;
- SparseCSFTensorConverter(const NumericTensorType& tensor,
+ public:
+ SparseCSFTensorConverter(const Tensor& tensor,
const std::shared_ptr<DataType>& index_value_type,
MemoryPool* pool)
: tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
- template <typename IndexValueType>
Status Convert() {
- using c_index_value_type = typename IndexValueType::c_type;
- RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits<c_index_value_type>::max()));
+ RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, tensor_.shape()));
+
+ const int index_elsize =
+ checked_cast<const IntegerType&>(*index_value_type_).bit_width() / CHAR_BIT;
+ const int value_elsize =
+ checked_cast<const FixedWidthType&>(*tensor_.type()).bit_width() / CHAR_BIT;
Review comment:
I agreed with you. I felt it is better to extract this idiom as a function, too.
`internal::GetByteWith` looks good to me, but how about the member function of `FixedWidthType`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on a change in pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#discussion_r447268862
##########
File path: cpp/src/arrow/tensor/csf_converter.cc
##########
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector<int64_t>& coord, const std::vector<int64_
// ----------------------------------------------------------------------
// SparseTensorConverter for SparseCSFIndex
-template <typename TYPE>
-class SparseCSFTensorConverter {
- public:
- using NumericTensorType = NumericTensor<TYPE>;
- using value_type = typename NumericTensorType::value_type;
+class SparseCSFTensorConverter : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::AssignIndex;
+ using SparseTensorConverterMixin::IsNonZero;
- SparseCSFTensorConverter(const NumericTensorType& tensor,
+ public:
+ SparseCSFTensorConverter(const Tensor& tensor,
const std::shared_ptr<DataType>& index_value_type,
MemoryPool* pool)
: tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
- template <typename IndexValueType>
Status Convert() {
- using c_index_value_type = typename IndexValueType::c_type;
- RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits<c_index_value_type>::max()));
+ RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, tensor_.shape()));
+
+ const int index_elsize =
+ checked_cast<const IntegerType&>(*index_value_type_).bit_width() / CHAR_BIT;
+ const int value_elsize =
+ checked_cast<const FixedWidthType&>(*tensor_.type()).bit_width() / CHAR_BIT;
Review comment:
Oops, I don't need to return `-1` because `checked_cast` is used in this function. Sorry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn edited a comment on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn edited a comment on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-651458470
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-649561148
This removes already more than 2MB of code from libarrow.so on Linux: great. I'll keep an eye on this
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm commented on a change in pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#discussion_r447264419
##########
File path: cpp/src/arrow/tensor/csf_converter.cc
##########
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector<int64_t>& coord, const std::vector<int64_
// ----------------------------------------------------------------------
// SparseTensorConverter for SparseCSFIndex
-template <typename TYPE>
-class SparseCSFTensorConverter {
- public:
- using NumericTensorType = NumericTensor<TYPE>;
- using value_type = typename NumericTensorType::value_type;
+class SparseCSFTensorConverter : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::AssignIndex;
+ using SparseTensorConverterMixin::IsNonZero;
- SparseCSFTensorConverter(const NumericTensorType& tensor,
+ public:
+ SparseCSFTensorConverter(const Tensor& tensor,
const std::shared_ptr<DataType>& index_value_type,
MemoryPool* pool)
: tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
- template <typename IndexValueType>
Status Convert() {
- using c_index_value_type = typename IndexValueType::c_type;
- RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits<c_index_value_type>::max()));
+ RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, tensor_.shape()));
+
+ const int index_elsize =
+ checked_cast<const IntegerType&>(*index_value_type_).bit_width() / CHAR_BIT;
+ const int value_elsize =
+ checked_cast<const FixedWidthType&>(*tensor_.type()).bit_width() / CHAR_BIT;
Review comment:
As long as the signature of the function is `int(const DataType&)` it sounds good to me. I don't think it should be a non-internal API because it does not need to do error-handling. If it's a non-static member of FixedWidthType, then a `checked_cast` is still necessary which seems against the spirit of making the code simpler
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-652298958
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.
<details>
<summary><b>The full result is shown below:</b></summary>
<table role="table">
<thead>
<tr>
<th>Format</th>
<th>IndexType</th>
<th>ValueType</th>
<th align="right">Change %</th>
<th align="right">Baseline</th>
<th align="right">Contender</th>
</tr>
</thead>
<tbody>
<tr>
<td>COO</td>
<td>Int32Type</td>
<td>Int8</td>
<td align="right">56.426</td>
<td align="right">2459.397090</td>
<td align="right">3847.138775</td>
</tr>
<tr>
<td>COO</td>
<td>Int16Type</td>
<td>Int8</td>
<td align="right">43.062</td>
<td align="right">2619.848028</td>
<td align="right">3748.014603</td>
</tr>
<tr>
<td>COO</td>
<td>Int64Type</td>
<td>Int8</td>
<td align="right">40.137</td>
<td align="right">2850.826136</td>
<td align="right">3995.057821</td>
</tr>
<tr>
<td>COO</td>
<td>Int8Type</td>
<td>Int8</td>
<td align="right">35.478</td>
<td align="right">2776.716694</td>
<td align="right">3761.850287</td>
</tr>
<tr>
<td>COO</td>
<td>Int16Type</td>
<td>Int16</td>
<td align="right">66.812</td>
<td align="right">2394.011928</td>
<td align="right">3993.489611</td>
</tr>
<tr>
<td>COO</td>
<td>Int32Type</td>
<td>Int16</td>
<td align="right">59.281</td>
<td align="right">2728.577337</td>
<td align="right">4346.096797</td>
</tr>
<tr>
<td>COO</td>
<td>Int64Type</td>
<td>Int16</td>
<td align="right">52.043</td>
<td align="right">2596.404005</td>
<td align="right">3947.663313</td>
</tr>
<tr>
<td>COO</td>
<td>Int8Type</td>
<td>Int16</td>
<td align="right">44.510</td>
<td align="right">2768.567992</td>
<td align="right">4000.853968</td>
</tr>
<tr>
<td>COO</td>
<td>Int32Type</td>
<td>Float</td>
<td align="right">93.283</td>
<td align="right">2287.343291</td>
<td align="right">4421.039474</td>
</tr>
<tr>
<td>COO</td>
<td>Int16Type</td>
<td>Float</td>
<td align="right">71.100</td>
<td align="right">2577.689569</td>
<td align="right">4410.437858</td>
</tr>
<tr>
<td>COO</td>
<td>Int64Type</td>
<td>Float</td>
<td align="right">61.035</td>
<td align="right">2821.300996</td>
<td align="right">4543.277655</td>
</tr>
<tr>
<td>COO</td>
<td>Int8Type</td>
<td>Float</td>
<td align="right">53.303</td>
<td align="right">2666.801559</td>
<td align="right">4088.291362</td>
</tr>
<tr>
<td>COO</td>
<td>Int64Type</td>
<td>Double</td>
<td align="right">88.230</td>
<td align="right">2616.000738</td>
<td align="right">4924.096764</td>
</tr>
<tr>
<td>COO</td>
<td>Int16Type</td>
<td>Double</td>
<td align="right">70.188</td>
<td align="right">2698.335495</td>
<td align="right">4592.253023</td>
</tr>
<tr>
<td>COO</td>
<td>Int32Type</td>
<td>Double</td>
<td align="right">69.368</td>
<td align="right">2579.753902</td>
<td align="right">4369.267270</td>
</tr>
<tr>
<td>COO</td>
<td>Int8Type</td>
<td>Double</td>
<td align="right">62.781</td>
<td align="right">2747.179806</td>
<td align="right">4471.895533</td>
</tr>
<tr>
<td>CSR</td>
<td>Int8Type</td>
<td>Int8</td>
<td align="right">-1.660</td>
<td align="right">4626.181791</td>
<td align="right">4549.389657</td>
</tr>
<tr>
<td>CSR</td>
<td>Int32Type</td>
<td>Int8</td>
<td align="right">-4.718</td>
<td align="right">4708.044556</td>
<td align="right">4485.938002</td>
</tr>
<tr>
<td>CSR</td>
<td>Int64Type</td>
<td>Int8</td>
<td align="right">-4.905</td>
<td align="right">4687.961132</td>
<td align="right">4458.024004</td>
</tr>
<tr>
<td>CSR</td>
<td>Int16Type</td>
<td>Int8</td>
<td align="right">-7.164</td>
<td align="right">4851.803479</td>
<td align="right">4504.201161</td>
</tr>
<tr>
<td>CSR</td>
<td>Int32Type</td>
<td>Int16</td>
<td align="right">-1.481</td>
<td align="right">4722.676795</td>
<td align="right">4652.711030</td>
</tr>
<tr>
<td>CSR</td>
<td>Int64Type</td>
<td>Int16</td>
<td align="right">-5.853</td>
<td align="right">4573.124314</td>
<td align="right">4305.462839</td>
</tr>
<tr>
<td>CSR</td>
<td>Int8Type</td>
<td>Int16</td>
<td align="right">-11.890</td>
<td align="right">4631.290389</td>
<td align="right">4080.628990</td>
</tr>
<tr>
<td>CSR</td>
<td>Int16Type</td>
<td>Int16</td>
<td align="right">-12.372</td>
<td align="right">4737.320600</td>
<td align="right">4151.197839</td>
</tr>
<tr>
<td>CSR</td>
<td>Int64Type</td>
<td>Float</td>
<td align="right">-4.999</td>
<td align="right">4711.575565</td>
<td align="right">4476.032868</td>
</tr>
<tr>
<td>CSR</td>
<td>Int16Type</td>
<td>Float</td>
<td align="right">-5.062</td>
<td align="right">4811.457037</td>
<td align="right">4567.909440</td>
</tr>
<tr>
<td>CSR</td>
<td>Int32Type</td>
<td>Float</td>
<td align="right">-7.651</td>
<td align="right">4814.813084</td>
<td align="right">4446.432341</td>
</tr>
<tr>
<td>CSR</td>
<td>Int8Type</td>
<td>Float</td>
<td align="right">-12.196</td>
<td align="right">5158.868330</td>
<td align="right">4529.676101</td>
</tr>
<tr>
<td>CSR</td>
<td>Int32Type</td>
<td>Double</td>
<td align="right">0.384</td>
<td align="right">4631.005506</td>
<td align="right">4648.789489</td>
</tr>
<tr>
<td>CSR</td>
<td>Int64Type</td>
<td>Double</td>
<td align="right">-0.273</td>
<td align="right">4816.669765</td>
<td align="right">4803.522209</td>
</tr>
<tr>
<td>CSR</td>
<td>Int8Type</td>
<td>Double</td>
<td align="right">-3.453</td>
<td align="right">4646.652774</td>
<td align="right">4486.217718</td>
</tr>
<tr>
<td>CSR</td>
<td>Int16Type</td>
<td>Double</td>
<td align="right">-8.963</td>
<td align="right">4952.032110</td>
<td align="right">4508.161865</td>
</tr>
<tr>
<td>CSC</td>
<td>Int8Type</td>
<td>Int8</td>
<td align="right">-4.207</td>
<td align="right">4871.793338</td>
<td align="right">4666.848429</td>
</tr>
<tr>
<td>CSC</td>
<td>Int64Type</td>
<td>Int8</td>
<td align="right">-10.582</td>
<td align="right">4571.251613</td>
<td align="right">4087.509465</td>
</tr>
<tr>
<td>CSC</td>
<td>Int16Type</td>
<td>Int8</td>
<td align="right">-13.189</td>
<td align="right">4865.666295</td>
<td align="right">4223.934008</td>
</tr>
<tr>
<td>CSC</td>
<td>Int32Type</td>
<td>Int8</td>
<td align="right">-16.546</td>
<td align="right">4999.577506</td>
<td align="right">4172.360711</td>
</tr>
<tr>
<td>CSC</td>
<td>Int32Type</td>
<td>Int16</td>
<td align="right">-0.619</td>
<td align="right">4842.515715</td>
<td align="right">4812.531287</td>
</tr>
<tr>
<td>CSC</td>
<td>Int8Type</td>
<td>Int16</td>
<td align="right">-4.128</td>
<td align="right">4689.737235</td>
<td align="right">4496.135413</td>
</tr>
<tr>
<td>CSC</td>
<td>Int16Type</td>
<td>Int16</td>
<td align="right">-8.110</td>
<td align="right">4643.413441</td>
<td align="right">4266.836346</td>
</tr>
<tr>
<td>CSC</td>
<td>Int64Type</td>
<td>Int16</td>
<td align="right">-10.302</td>
<td align="right">4971.879449</td>
<td align="right">4459.696887</td>
</tr>
<tr>
<td>CSC</td>
<td>Int8Type</td>
<td>Float</td>
<td align="right">-0.547</td>
<td align="right">4694.347970</td>
<td align="right">4668.667817</td>
</tr>
<tr>
<td>CSC</td>
<td>Int32Type</td>
<td>Float</td>
<td align="right">-0.619</td>
<td align="right">4718.207593</td>
<td align="right">4688.994670</td>
</tr>
<tr>
<td>CSC</td>
<td>Int64Type</td>
<td>Float</td>
<td align="right">-3.360</td>
<td align="right">4847.753893</td>
<td align="right">4684.849545</td>
</tr>
<tr>
<td>CSC</td>
<td>Int16Type</td>
<td>Float</td>
<td align="right">-7.408</td>
<td align="right">4956.795348</td>
<td align="right">4589.607722</td>
</tr>
<tr>
<td>CSC</td>
<td>Int32Type</td>
<td>Double</td>
<td align="right">0.491</td>
<td align="right">4917.172760</td>
<td align="right">4941.321519</td>
</tr>
<tr>
<td>CSC</td>
<td>Int64Type</td>
<td>Double</td>
<td align="right">-0.754</td>
<td align="right">5069.589290</td>
<td align="right">5031.376333</td>
</tr>
<tr>
<td>CSC</td>
<td>Int16Type</td>
<td>Double</td>
<td align="right">-2.023</td>
<td align="right">4762.425910</td>
<td align="right">4666.071367</td>
</tr>
<tr>
<td>CSC</td>
<td>Int8Type</td>
<td>Double</td>
<td align="right">-5.987</td>
<td align="right">4934.016268</td>
<td align="right">4638.636384</td>
</tr>
<tr>
<td>CSF</td>
<td>Int16Type</td>
<td>Int8</td>
<td align="right">25.394</td>
<td align="right">7885.986312</td>
<td align="right">9888.571309</td>
</tr>
<tr>
<td>CSF</td>
<td>Int64Type</td>
<td>Int8</td>
<td align="right">22.605</td>
<td align="right">9083.444559</td>
<td align="right">11136.716151</td>
</tr>
<tr>
<td>CSF</td>
<td>Int32Type</td>
<td>Int8</td>
<td align="right">13.486</td>
<td align="right">8228.377064</td>
<td align="right">9338.031561</td>
</tr>
<tr>
<td>CSF</td>
<td>Int8Type</td>
<td>Int8</td>
<td align="right">11.900</td>
<td align="right">7687.347967</td>
<td align="right">8602.168534</td>
</tr>
<tr>
<td>CSF</td>
<td>Int64Type</td>
<td>Int16</td>
<td align="right">28.094</td>
<td align="right">8247.255772</td>
<td align="right">10564.280045</td>
</tr>
<tr>
<td>CSF</td>
<td>Int8Type</td>
<td>Int16</td>
<td align="right">20.276</td>
<td align="right">8172.411486</td>
<td align="right">9829.454135</td>
</tr>
<tr>
<td>CSF</td>
<td>Int32Type</td>
<td>Int16</td>
<td align="right">15.240</td>
<td align="right">8776.213727</td>
<td align="right">10113.751856</td>
</tr>
<tr>
<td>CSF</td>
<td>Int16Type</td>
<td>Int16</td>
<td align="right">13.707</td>
<td align="right">8396.322326</td>
<td align="right">9547.223214</td>
</tr>
<tr>
<td>CSF</td>
<td>Int16Type</td>
<td>Float</td>
<td align="right">25.989</td>
<td align="right">8340.996459</td>
<td align="right">10508.758442</td>
</tr>
<tr>
<td>CSF</td>
<td>Int32Type</td>
<td>Float</td>
<td align="right">20.562</td>
<td align="right">8882.075299</td>
<td align="right">10708.400993</td>
</tr>
<tr>
<td>CSF</td>
<td>Int8Type</td>
<td>Float</td>
<td align="right">19.021</td>
<td align="right">8507.160025</td>
<td align="right">10125.299339</td>
</tr>
<tr>
<td>CSF</td>
<td>Int64Type</td>
<td>Float</td>
<td align="right">1.769</td>
<td align="right">9657.000323</td>
<td align="right">9827.819671</td>
</tr>
<tr>
<td>CSF</td>
<td>Int8Type</td>
<td>Double</td>
<td align="right">20.382</td>
<td align="right">8380.183782</td>
<td align="right">10088.211026</td>
</tr>
<tr>
<td>CSF</td>
<td>Int32Type</td>
<td>Double</td>
<td align="right">14.353</td>
<td align="right">9512.403696</td>
<td align="right">10877.711426</td>
</tr>
<tr>
<td>CSF</td>
<td>Int16Type</td>
<td>Double</td>
<td align="right">13.737</td>
<td align="right">8536.269931</td>
<td align="right">9708.870939</td>
</tr>
<tr>
<td>CSF</td>
<td>Int64Type</td>
<td>Double</td>
<td align="right">12.413</td>
<td align="right">9737.555217</td>
<td align="right">10946.296736</td>
</tr>
</tbody>
</table>
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm edited a comment on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm edited a comment on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-651457062
@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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-651457062
@mrkn it's up to you, it's fine with me if you work on performance tuning in another PR or this one
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-653559925
@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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] mrkn commented on a change in pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
mrkn commented on a change in pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#discussion_r447267538
##########
File path: cpp/src/arrow/tensor/csf_converter.cc
##########
@@ -57,73 +57,86 @@ inline void IncrementIndex(std::vector<int64_t>& coord, const std::vector<int64_
// ----------------------------------------------------------------------
// SparseTensorConverter for SparseCSFIndex
-template <typename TYPE>
-class SparseCSFTensorConverter {
- public:
- using NumericTensorType = NumericTensor<TYPE>;
- using value_type = typename NumericTensorType::value_type;
+class SparseCSFTensorConverter : private SparseTensorConverterMixin {
+ using SparseTensorConverterMixin::AssignIndex;
+ using SparseTensorConverterMixin::IsNonZero;
- SparseCSFTensorConverter(const NumericTensorType& tensor,
+ public:
+ SparseCSFTensorConverter(const Tensor& tensor,
const std::shared_ptr<DataType>& index_value_type,
MemoryPool* pool)
: tensor_(tensor), index_value_type_(index_value_type), pool_(pool) {}
- template <typename IndexValueType>
Status Convert() {
- using c_index_value_type = typename IndexValueType::c_type;
- RETURN_NOT_OK(CheckMaximumValue(std::numeric_limits<c_index_value_type>::max()));
+ RETURN_NOT_OK(CheckSparseIndexMaximumValue(index_value_type_, tensor_.shape()));
+
+ const int index_elsize =
+ checked_cast<const IntegerType&>(*index_value_type_).bit_width() / CHAR_BIT;
+ const int value_elsize =
+ checked_cast<const FixedWidthType&>(*tensor_.type()).bit_width() / CHAR_BIT;
Review comment:
OK. I'll make `inernal::GetByteWidth`, and let it return `-1` for non-`FixedWidthType` values.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #7539: ARROW-9156: [C++] Reducing the code size of the tensor module
Posted by GitBox <gi...@apache.org>.
wesm commented on pull request #7539:
URL: https://github.com/apache/arrow/pull/7539#issuecomment-653023037
@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
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org