feat: Optimize UnsafeRow deserialize for scalar type #10797

jinchengchenghh · 2024-08-21T07:45:07Z

Deserialize rows by memoryAddress and offset, and add the tests and benchmark, the performance improves 3 times.
Current deserialization StructBatchIterator deserializes rows one by one, get the data of the columns of one row, only set one value each time, but current implementation can get the data of one column by memory address of the rows and offset of each column, so we can set all rows of one column at once.

============================================================================
[...]amicRowVectorDeserializeBenchmark.cpp     relative  time/iter   iters/s
============================================================================
deserialize(batch_10_100k_string_only)                    421.74ns     2.37M
deserializeFast(batch_10_100k_string_only)      353.42%   119.33ns     8.38M
deserialize(batch_100_100k_string_only)                   374.03ns     2.67M
deserializeFast(batch_100_100k_string_only)     257.98%   144.98ns     6.90M
deserialize(batch_10_100k_all_types)                      271.23ns     3.69M
deserialize(batch_100_100k_all_types)                     710.39ns     1.41M
deserialize(batch_10_100k_scalar_types)                   264.71ns     3.78M
deserializeFast(batch_10_100k_scalar_types)     340.84%    77.67ns    12.88M
deserialize(batch_100_100k_scalar_types)                  258.62ns     3.87M
deserializeFast(batch_100_100k_scalar_types)    429.72%    60.18ns    16.62M

netlify · 2024-08-21T07:45:23Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`833023a`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/672aca38cd7c760008cc2aff

jinchengchenghh · 2024-08-22T02:47:04Z

Could you help review? Thanks! @rui-mo

rui-mo

Deserialize rows by memoryAddress and offset

Could you add more information on where the performance improvement mainly comes from? I notice previously the deserialize methods in UnsafeRowPrimitiveBatchDeserializer create intermediate values and set it to the vector, while the new way writes to the position directly.

rui-mo

Thanks.

velox/row/UnsafeRowDeserializers.h

velox/row/UnsafeRowDeserializers.cpp

rui-mo · 2024-08-22T09:34:37Z

velox/row/UnsafeRowDeserializers.h

+  /// @param offsets offset of the row deserialized data. It's size should be
+  /// equal to the deserialized rows. First offset is 0.
+  /// @param pool the memory pool to allocate Vectors.
+  static VectorPtr deserialize(


Just curious. Is it necessary to keep two APIs for the deserialization instead of replacing the original one?

Yes. This API only supports scalar types, it is hard to support complex data type, because it is hard to get the offsets for them. Another exists API supports all the data types.

Can the APIs be unified so that only complex types use the row-row deserialization?

yes, ok, I will do the refactor.

The previous API read the rows from several std::string_view, but the new API requires all the row in one buffer started with memoryAddress, for this public override API, it supplies ByteInputStream which consists several buffers, so we cannot get all the rows in one buffer, so I keep the two APIs but the new API supports the complex data type by revoking the old API.
https://github.com/facebookincubator/velox/blob/main/velox/serializers/UnsafeRowSerializer.cpp#L135

Users could use any of them as they need.

And here requires the old API, because the serialize format is
| row size | serialized row | row size | serialized row
https://github.com/facebookincubator/velox/blob/main/velox/serializers/UnsafeRowSerializer.cpp#L145

This optimization requires the serialized data is in contiguous memory.

So we should keep the 2 APIs.

jinchengchenghh · 2024-08-23T01:57:42Z

Deserialize rows by memoryAddress and offset

Could you add more information on where the performance improvement mainly comes from? I notice previously the deserialize methods in UnsafeRowPrimitiveBatchDeserializer create intermediate values and set it to the vector, while the new way writes to the position directly.

Added to the PR description.

jinchengchenghh · 2024-08-28T05:32:30Z

Anymore comments? @rui-mo

rui-mo · 2024-08-29T09:28:52Z

@marin-ma Would you like to take a review? The idea appears to be similar as #10714.

rui-mo · 2024-08-29T09:35:33Z

velox/row/UnsafeRowDeserializers.cpp

+
+int32_t getTotalStringSize(
+    int32_t columnIdx,
+    int32_t numRows,


It seems numRows is 'vector_size_t' type in the 'deserialize' method. Perhaps also fix this in the createFlatVectorFast functions.

rui-mo · 2024-08-29T09:40:24Z

velox/row/UnsafeRowDeserializers.cpp

+  for (auto pos = 0; pos < numRows; pos++) {
+    if (!isNullAt(memoryAddress + offsets[pos], columnIdx)) {
+      const uint8_t* srcptr = (memoryAddress + offsets[pos] + fieldOffset);
+      uint8_t* destptr = rawValues + (pos << shift);


nit: use camel case for variable names. srcptr-> srcPtr, wordoffset -> wordOffset, destptr -> destPtr, ...

rui-mo · 2024-08-29T09:41:35Z

velox/row/UnsafeRowDeserializers.cpp

+      int32_t wordoffset = static_cast<int32_t>(offsetAndSize >> 32);
+      uint8_t bytesValue[length];
+      memcpy(bytesValue, memoryAddress + offsets[pos] + wordoffset, length);
+      uint8_t bytesValue2[16]{};


nit: any better naming for 'bytesValue2'?

rui-mo · 2024-08-29T09:49:55Z

velox/row/UnsafeRowDeserializers.h

+  /// @param offsets offset of the row deserialized data. It's size should be
+  /// equal to the deserialized rows. First offset is 0.
+  /// @param pool the memory pool to allocate Vectors.
+  static VectorPtr deserialize(


Can the APIs be unified so that only complex types use the row-row deserialization?

jinchengchenghh · 2024-10-28T07:02:31Z

Could you help review again? Thanks! @rui-mo

rui-mo

Thanks. Added some minors.

rui-mo · 2024-11-04T03:07:00Z

velox/row/UnsafeRowDeserializers.h

+  /// Deserialize fast when all the column types are primitive type, otherwise,
+  /// deserialize rows one by one, as previously. Fast deserialization
+  /// computes the start memoryAddress of one data by the column index and row
+  /// number, and set all the data of one column at once.


Comment suggestion:

/// Deserializes rows that are stored in contiguous memory.
/// If all column types are primitive, fast deserialization is used.
/// Otherwise, rows are deserialized one by one as before.
/// Fast deserialization calculates the starting memory address of data
/// based on the column index and row number, setting all data for one column at once.

rui-mo · 2024-11-04T03:08:30Z

velox/row/UnsafeRowDeserializers.h

+  /// deserialize rows one by one, as previously. Fast deserialization
+  /// computes the start memoryAddress of one data by the column index and row
+  /// number, and set all the data of one column at once.
+  /// @param memoryAddress the start memory address of the serialized rows .


nit: remove extra white-space after 'rows'.

rui-mo · 2024-11-04T03:10:09Z

velox/row/UnsafeRowDeserializers.h

+  /// @param type the element type.
+  /// @param offsets offset of each row serialized data. It's size should be
+  /// equal to the deserialized rows. First offset is 0.
+  /// @param pool the memory pool to allocate Vector.


The memory pool used to allocate vector.

rui-mo · 2024-11-04T03:17:25Z

velox/row/UnsafeRowDeserializers.cpp

+    memory::MemoryPool* pool) {
+  if (fastSupported(type)) {
+    return deserializeFast(memoryAddress, type, offsets, pool);
+  } else {


nit: no need for 'else' after return.

rui-mo · 2024-11-04T03:22:05Z

velox/row/UnsafeRowDeserializers.cpp

+      columnIdx, numRows, fieldOffset, offsets, memoryAddress);
+  char* rawBuffer = column->getRawStringBufferWithSpace(totalSize, true);
+  for (auto row = 0; row < numRows; row++) {
+    if (!isNullAt(memoryAddress + offsets[row], columnIdx)) {


nit: 'memoryAddress + offsets[row]' is used several times, any chance to make it a variable?

I think not. It uses 3 variables and used at hot spot branch, make it to a function does not make code clear.

rui-mo · 2024-11-04T03:22:57Z

velox/row/UnsafeRowDeserializers.cpp

+          *(int64_t*)(memoryAddress + offsets[row] + fieldOffset);
+      const int32_t length = static_cast<int32_t>(offsetAndSize);
+      const int32_t wordOffset = static_cast<int32_t>(offsetAndSize >> 32);
+      auto valueSrcPtr = memoryAddress + offsets[row] + wordOffset;


use auto* to indicate a pointer.

rui-mo · 2024-11-04T03:49:16Z

velox/row/UnsafeRowDeserializers.h

+   * @param pool the memory pool to allocate Vectors from
+   *data to a array.
+   * @return a VectorPtr
+   */


Not your change, but it would be better to not use the old style /* */ for comments.

rui-mo · 2024-11-04T03:51:14Z

velox/row/UnsafeRowDeserializers.cpp

+    const vector_size_t numRows = offsets.size();
+    for (auto i = 0; i < numRows; i++) {
+      const auto length =
+          (i == numRows - 1 ? offsets[i] : offsets[i + 1] - offsets[i]);


if (i == numRows - 1), the length is 'offsets[i]'. Is that correct?

Thanks for catching it, the offsets size should be numRows + 1, then we can get the correct last row length.

rui-mo · 2024-11-04T03:55:10Z

velox/row/UnsafeRowDeserializers.cpp

+namespace facebook::velox::row {
+namespace {
+
+inline int64_t getFieldOffset(int64_t nullBitsetWidthInBytes, int32_t index) {


Would you add some comments for this method?

rui-mo · 2024-11-04T03:56:03Z

velox/row/UnsafeRowDeserializers.cpp

+}
+
+inline bool isNullAt(const uint8_t* memoryAddress, int32_t index) {
+  return bits::isBitSet(memoryAddress, index);


This function just calls another function with the original parameters. Wondering if we need it.

Yes, we need it. It is more clear to indicate it is the null condition.

jinchengchenghh · 2024-12-20T05:58:00Z

velox/row/benchmarks/CMakeLists.txt

+    velox_row_fast
+    velox_vector_fuzzer
+    Folly::folly
+    ${FOLLY_BENCHMARK})


Folly::follybenchmark

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 21, 2024

rui-mo reviewed Aug 22, 2024

View reviewed changes

rui-mo reviewed Aug 29, 2024

View reviewed changes

This was referenced Sep 9, 2024

Optimize OrderBy operator by new layout #10949

Open

[GLUTEN-7205] [VL] Optimize row to column for scalar type apache/incubator-gluten#7206

Merged

FelixYBW mentioned this pull request Sep 16, 2024

Optimize Row to column for scalar type apache/incubator-gluten#7205

Closed

jinchengchenghh requested review from assignUser and majetideepak as code owners October 17, 2024 05:34

jinchengchenghh force-pushed the r2c branch from 080aba5 to 2d65f82 Compare October 28, 2024 07:00

jinchengchenghh force-pushed the r2c branch from 324e27c to 18884c0 Compare November 1, 2024 06:20

rui-mo reviewed Nov 4, 2024

View reviewed changes

jinchengchenghh force-pushed the r2c branch from 797ab17 to 833023a Compare November 6, 2024 01:45

jinchengchenghh added 2 commits November 6, 2024 09:10

Optimize UnsafeRow to Vector

d821fb5

address comments

833023a

jinchengchenghh changed the title ~~Optimize UnsafeRow deserialize for scalar type~~ feat: Optimize UnsafeRow deserialize for scalar type Nov 15, 2024

jinchengchenghh commented Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Optimize UnsafeRow deserialize for scalar type #10797

feat: Optimize UnsafeRow deserialize for scalar type #10797

jinchengchenghh commented Aug 21, 2024 •

edited

Loading

netlify bot commented Aug 21, 2024 •

edited

Loading

jinchengchenghh commented Aug 22, 2024

rui-mo left a comment •

edited

Loading

rui-mo left a comment

rui-mo Aug 22, 2024 •

edited

Loading

jinchengchenghh Aug 23, 2024

rui-mo Aug 29, 2024

jinchengchenghh Aug 30, 2024

jinchengchenghh Sep 3, 2024

jinchengchenghh Sep 3, 2024

jinchengchenghh Oct 28, 2024

jinchengchenghh commented Aug 23, 2024

jinchengchenghh commented Aug 28, 2024

rui-mo commented Aug 29, 2024

rui-mo Aug 29, 2024

rui-mo Aug 29, 2024

rui-mo Aug 29, 2024

rui-mo Aug 29, 2024

jinchengchenghh commented Oct 28, 2024

rui-mo left a comment

rui-mo Nov 4, 2024

rui-mo Nov 4, 2024

rui-mo Nov 4, 2024

rui-mo Nov 4, 2024

rui-mo Nov 4, 2024

jinchengchenghh Nov 4, 2024

rui-mo Nov 4, 2024

rui-mo Nov 4, 2024

rui-mo Nov 4, 2024 •

edited

Loading

jinchengchenghh Nov 4, 2024

rui-mo Nov 4, 2024

rui-mo Nov 4, 2024

jinchengchenghh Nov 4, 2024

jinchengchenghh Dec 20, 2024

feat: Optimize UnsafeRow deserialize for scalar type #10797

Are you sure you want to change the base?

feat: Optimize UnsafeRow deserialize for scalar type #10797

Conversation

jinchengchenghh commented Aug 21, 2024 • edited Loading

netlify bot commented Aug 21, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

jinchengchenghh commented Aug 22, 2024

rui-mo left a comment • edited Loading

Choose a reason for hiding this comment

rui-mo left a comment

Choose a reason for hiding this comment

rui-mo Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Aug 23, 2024

jinchengchenghh commented Aug 28, 2024

rui-mo commented Aug 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Oct 28, 2024

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Aug 21, 2024 •

edited

Loading

netlify bot commented Aug 21, 2024 •

edited

Loading

rui-mo left a comment •

edited

Loading

rui-mo Aug 22, 2024 •

edited

Loading

rui-mo Nov 4, 2024 •

edited

Loading