You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "AntoinePrv (via GitHub)" <gi...@apache.org> on 2025/09/16 12:22:49 UTC
[PR] GH-47572: [C++][Parquet] Generalize bpacking and possible speedup [arrow]
AntoinePrv opened a new pull request, #47573:
URL: https://github.com/apache/arrow/pull/47573
### Rationale for this change
### What changes are included in this PR?
### Are these changes tested?
### Are there any user-facing changes?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2428886890
##########
cpp/src/arrow/util/bpacking_scalar_internal.h:
##########
@@ -23,7 +23,20 @@
namespace arrow::internal {
-ARROW_EXPORT int unpack32_neon(const uint8_t* in, uint32_t* out, int batch_size,
+template <typename Uint>
+ARROW_EXPORT int unpack_scalar(const uint8_t* in, Uint* out, int batch_size,
int num_bits);
+extern template ARROW_TEMPLATE_EXPORT int unpack_scalar<uint8_t>(const uint8_t*, uint8_t*,
Review Comment:
I don't think `ARROW_TEMPLATE_EXPORT` is useful, because this probably wouldn't be called across DLL boundaries (or would it?).
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Generalize bpacking and possible speedup [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3347131011
The results for this are not good
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3402161887
I've revert to a `switch` statement and properly handled `bool`.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3409928377
@AntoinePrv There are a couple remaining comments on the codegen scripts, do you want to address them?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3402471156
> I've revert to a `switch` statement and properly handled `bool`.
Thank you. Here are updated benchmark results on my machine for Parquet read/decode: https://gist.github.com/pitrou/d9c22d4030e87b0c2cad66dd99687d63
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2428884588
##########
cpp/src/arrow/util/bpacking_internal.h:
##########
@@ -23,17 +23,19 @@
namespace arrow::internal {
-/// The scalar 32 bit unpacking.
-ARROW_EXPORT int unpack32_scalar(const uint8_t* in, uint32_t* out, int batch_size,
- int num_bits);
-
-/// The scalar 64 bit unpacking.
-ARROW_EXPORT int unpack64_scalar(const uint8_t* in, uint64_t* out, int batch_size,
- int num_bits);
-
-ARROW_EXPORT
-int unpack32(const uint8_t* in, uint32_t* out, int batch_size, int num_bits);
-ARROW_EXPORT
-int unpack64(const uint8_t* in, uint64_t* out, int batch_size, int num_bits);
+template <typename Uint>
+ARROW_EXPORT int unpack(const uint8_t* in, Uint* out, int batch_size, int num_bits);
+
+extern template ARROW_TEMPLATE_EXPORT int unpack<uint8_t>(const uint8_t*, uint8_t*, int,
Review Comment:
I don't think `ARROW_TEMPLATE_EXPORT` is useful, this shouldn't be called across DLL boundaries.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2431562433
##########
.gitattributes:
##########
@@ -1,4 +1,4 @@
-cpp/src/arrow/util/bpacking_*_generated.h linguist-generated=true
+cpp/src/arrow/util/bpacking_*_generated_internal.h linguist-generated=true
Review Comment:
Yes, we'll probably find out in further PRs :)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3406552924
One hypothesis I'm wondering about is whether mixing scalar code with SIMD introduce additional latency.
For comparison, the Lemire implementation I'm curious about is loading data once with a `load_unaligned`, then using a swizzle (byte reorder), then a rhsift, then a mask.
For small sizes, we could even make multiple shifts per swizzle and multiple swizzle per read (the extreme case being bit_width=1 where we can read once ~256 bits and write unpacked 256 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3402494444
And here are benchmark results on my machine for the bpacking benchmarks: https://gist.github.com/pitrou/f08136eb7ca9c3710493132f74da5b1f
Everything is at least as good as it was, except for a couple scalar UInt32 benchmarks, and I don't think we should care much about them.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3402512604
Thanks for your patience. Conbench analyzed the 0 benchmarking runs that have been run so far on PR commit dd1d942649567320d7b56f39ef2f521160a4b060.
None of the specified runs were found on the Conbench server.
The [full Conbench report](https://github.com/apache/arrow/runs/52722267440) has more 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3401550480
@ursabot please benchmark lang=C++
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432487023
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
Review Comment:
Also, `out_bit_width` is not involved in the actual generated code: https://github.com/apache/arrow/blob/c8404a16805ed3ccde2f3687dced45859002ad87/cpp/src/arrow/util/bpacking_scalar_generated_internal.h#L54-L61
So perhaps `out_bit_width` should raise `NotImplementedError` for `bool`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3401579875
> @pitrou what about the Read/Decoder benchmarks?
Indeed the picture is different when looking at the Read/Decode benchmarks, where there isn't much of a difference between the array of function pointers and the switch/case statement.
Here is the diff between this PR and git main here: https://gist.github.com/pitrou/39f2c7ea2faec482c4d74a26e1c98f06
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2431582660
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
Review Comment:
That's not necessarily the case, right?
It sounds like, instead of generating dedicated functions for `bool`, you should perhaps add logic on the C++ side to redirect `bool` to the appropriately sized `uint`?
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
+ elif self.out_type.startswith("uint"):
+ return int(self.out_type.removeprefix("uint").removesuffix("_t"))
+
+ @property
+ def out_byte_width(self) -> int:
+ return self.out_bit_width // 8
+
+ @property
+ def out_type_half(self) -> str:
+ return f"uint{self.out_bit_width // 2}_t"
+
+ @property
+ def struct_name(self) -> str:
+ return "ScalarUnpackerForWidth"
+
+ def struct_specialization(self, bit: int) -> str:
+ return f"{self.struct_name}<{self.out_type}, {bit}>"
+
+ def print_struct_declaration(self):
+ print("template<typename Uint, int kBitWidth>")
+ print(f"struct {self.struct_name};")
+
+ @property
+ def howmany(self) -> int:
Review Comment:
It would be worth giving this a more explicit name, e.g. `num_unpacked_values`?
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
+ elif self.out_type.startswith("uint"):
+ return int(self.out_type.removeprefix("uint").removesuffix("_t"))
+
+ @property
+ def out_byte_width(self) -> int:
+ return self.out_bit_width // 8
+
+ @property
+ def out_type_half(self) -> str:
+ return f"uint{self.out_bit_width // 2}_t"
+
+ @property
+ def struct_name(self) -> str:
+ return "ScalarUnpackerForWidth"
+
+ def struct_specialization(self, bit: int) -> str:
+ return f"{self.struct_name}<{self.out_type}, {bit}>"
+
+ def print_struct_declaration(self):
+ print("template<typename Uint, int kBitWidth>")
+ print(f"struct {self.struct_name};")
+
+ @property
+ def howmany(self) -> int:
+ """How many values are we going to pack?"""
+ if self.smart_halve:
+ return self.out_bit_width // 2
+ return self.out_bit_width
+
+ def howmanywords(self, bit: int) -> int:
Review Comment:
Similar remark wrt. giving this a more descriptive name.
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
+ elif self.out_type.startswith("uint"):
+ return int(self.out_type.removeprefix("uint").removesuffix("_t"))
Review Comment:
It seems like it lacks a fallback `else`, perhaps raising an exception?
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
+ elif self.out_type.startswith("uint"):
+ return int(self.out_type.removeprefix("uint").removesuffix("_t"))
+
+ @property
+ def out_byte_width(self) -> int:
+ return self.out_bit_width // 8
+
+ @property
+ def out_type_half(self) -> str:
+ return f"uint{self.out_bit_width // 2}_t"
+
+ @property
+ def struct_name(self) -> str:
+ return "ScalarUnpackerForWidth"
+
+ def struct_specialization(self, bit: int) -> str:
+ return f"{self.struct_name}<{self.out_type}, {bit}>"
+
+ def print_struct_declaration(self):
+ print("template<typename Uint, int kBitWidth>")
+ print(f"struct {self.struct_name};")
+
+ @property
+ def howmany(self) -> int:
+ """How many values are we going to pack?"""
Review Comment:
```suggestion
"""How many values are we going to unpack?"""
```
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
Review Comment:
I'm afraid I don't understand what `smart_halve` means here. If it's meant to compute the number of unpacked values, perhaps we should simply pass it directly here?
```suggestion
num_values: int
```
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
Review Comment:
I don't this needs to be in the generated code, it's sufficient to put this at the top of this script.
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
Review Comment:
Seems like this line needs updating?
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
Review Comment:
Let's give the name of the script for clarity?
```suggestion
// File automatically generated by `bpacking_scalar_codegen.py`; DO NOT EDIT.
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2428884588
##########
cpp/src/arrow/util/bpacking_internal.h:
##########
@@ -23,17 +23,19 @@
namespace arrow::internal {
-/// The scalar 32 bit unpacking.
-ARROW_EXPORT int unpack32_scalar(const uint8_t* in, uint32_t* out, int batch_size,
- int num_bits);
-
-/// The scalar 64 bit unpacking.
-ARROW_EXPORT int unpack64_scalar(const uint8_t* in, uint64_t* out, int batch_size,
- int num_bits);
-
-ARROW_EXPORT
-int unpack32(const uint8_t* in, uint32_t* out, int batch_size, int num_bits);
-ARROW_EXPORT
-int unpack64(const uint8_t* in, uint64_t* out, int batch_size, int num_bits);
+template <typename Uint>
+ARROW_EXPORT int unpack(const uint8_t* in, Uint* out, int batch_size, int num_bits);
+
+extern template ARROW_TEMPLATE_EXPORT int unpack<uint8_t>(const uint8_t*, uint8_t*, int,
Review Comment:
I don't think `ARROW_TEMPLATE_EXPORT` is useful, this shouldn't be called across DLL boundaries.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2429012083
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -490,6 +490,8 @@ set(ARROW_UTIL_SRCS
util/bitmap_builders.cc
util/bitmap_ops.cc
util/bpacking.cc
+ util/bpacking_scalar.cc
+ util/bpacking_simd_min.cc
Review Comment:
It was either neon or sse4.2 (the latter was removed). I could rename it `_neon` but it does not highlight that it is not compiled any different than regular files.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2429716240
##########
.gitattributes:
##########
@@ -1,4 +1,4 @@
-cpp/src/arrow/util/bpacking_*_generated.h linguist-generated=true
+cpp/src/arrow/util/bpacking_*_generated_internal.h linguist-generated=true
Review Comment:
For some reason this doesn't seem to match the generated files below. Though this might just be a GitHub 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2428956737
##########
cpp/src/arrow/util/bpacking_dispatch_internal.h:
##########
@@ -0,0 +1,104 @@
+// 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.
+
+#pragma once
+
+#include <cstring>
+#include <utility>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+
+/// Unpack a zero bit packed array.
+template <typename Uint>
+int unpack_null(const uint8_t* in, Uint* out, int batch_size) {
+ std::memset(out, 0, batch_size * sizeof(Uint));
+ return batch_size;
+}
+
+/// Unpack a packed array where packed and unpacked values have exactly the same number of
+/// bits.
+template <typename Uint>
+int unpack_full(const uint8_t* in, Uint* out, int batch_size) {
+ if constexpr (ARROW_LITTLE_ENDIAN == 1) {
+ std::memcpy(out, in, batch_size * sizeof(Uint));
+ } else {
+ using bit_util::FromLittleEndian;
+ using util::SafeLoadAs;
+
+ for (int k = 0; k < batch_size; k += 1) {
+ out[k] = FromLittleEndian(SafeLoadAs<Uint>(in + (k * sizeof(Uint))));
+ }
+ }
+ return batch_size;
+}
+
+/// Unpack a packed array, delegating to a Unpacker struct.
+///
+/// @tparam kPackedBitWidth The width in bits of the values in the packed array.
+/// @tparam Unpacker The struct providing information and an ``unpack`` method to unpack a
+/// fixed amount of values (usually constrained by SIMD batch sizes and
+/// byte alignment).
+/// @tparam UnpackedUInt The type in which we unpack the values.
+template <int kPackedBitWidth, template <typename, int> typename Unpacker,
+ typename UnpackedUInt>
+int unpack_width(const uint8_t* in, UnpackedUInt* out, int batch_size) {
+ using UnpackerForWidth = Unpacker<UnpackedUInt, kPackedBitWidth>;
+
+ constexpr auto kValuesUnpacked = UnpackerForWidth::kValuesUnpacked;
Review Comment:
I think the currently implicit contract of these functions is unpack up to `batch_size` but possibly less.
At least that's how they are called. Best would be to have the epilog in these functions so that they extract exactly `batch_size` 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2431921237
##########
cpp/src/arrow/util/bpacking_test.cc:
##########
@@ -201,52 +198,116 @@ class TestUnpack : public ::testing::TestWithParam<TestUnpackSize> {
template <typename Int>
void TestAll(UnpackFunc<Int> unpack) {
- // Known values
- TestUnpackZeros(unpack);
- TestUnpackOnes(unpack);
- TestUnpackAlternating(unpack);
-
- // Roundtrips
- TestRoundtripAlignment(unpack, /* alignment_offset= */ 0);
- TestRoundtripAlignment(unpack, /* alignment_offset= */ 1);
+ constexpr int kMaxBitWidth = std::is_same_v<Int, bool> ? 1 : 8 * sizeof(Int);
+ // Given how many edge cases there are in unpacking integers, it is best to test all
+ // sizes
+ for (int bit_width = 0; bit_width <= kMaxBitWidth; ++bit_width) {
+ SCOPED_TRACE(::testing::Message() << "Testing bit_width=" << bit_width);
+
+ // Known values
+ TestUnpackZeros(unpack, bit_width);
+ TestUnpackOnes(unpack, bit_width);
+ TestUnpackAlternating(unpack, bit_width);
+
+ // Roundtrips
+ TestRoundtripAlignment(unpack, bit_width, /* alignment_offset= */ 0);
+ TestRoundtripAlignment(unpack, bit_width, /* alignment_offset= */ 1);
+ }
}
};
-INSTANTIATE_TEST_SUITE_P(
- UnpackMultiplesOf64Values, TestUnpack,
- ::testing::Values(TestUnpackSize{64, 1}, TestUnpackSize{128, 1},
- TestUnpackSize{2048, 1}, TestUnpackSize{64, 31},
- TestUnpackSize{128, 31}, TestUnpackSize{2048, 1},
- TestUnpackSize{2048, 8}, TestUnpackSize{2048, 13},
- TestUnpackSize{2048, 16}, TestUnpackSize{2048, 31},
- TestUnpackSize{2048, 32}));
+// There are actually many differences across the different sizes.
+// It is best to test them all.
+INSTANTIATE_TEST_SUITE_P(UnpackMultiplesOf64Values, TestUnpack,
+ ::testing::Values(64, 128, 2048),
+ [](const ::testing::TestParamInfo<TestUnpack::ParamType>& info) {
+ return "Count" + std::to_string(info.param);
Review Comment:
Well, sorry, it seems this was a bad suggestion since it broke CI :-(
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "voltrondatabot (via GitHub)" <gi...@apache.org>.
voltrondatabot commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3401550734
Benchmark runs are scheduled for commit dd1d942649567320d7b56f39ef2f521160a4b060. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2428993190
##########
cpp/src/arrow/util/bpacking_scalar_internal.h:
##########
@@ -23,7 +23,20 @@
namespace arrow::internal {
-ARROW_EXPORT int unpack32_neon(const uint8_t* in, uint32_t* out, int batch_size,
+template <typename Uint>
+ARROW_EXPORT int unpack_scalar(const uint8_t* in, Uint* out, int batch_size,
int num_bits);
+extern template ARROW_TEMPLATE_EXPORT int unpack_scalar<uint8_t>(const uint8_t*, uint8_t*,
Review Comment:
Yes, only the benchmarks and tests need it. Any solution how we might change that?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3402895252
Thanks for your patience. Conbench analyzed the 2 benchmarking runs that have been run so far on PR commit cad05efb3946f864bdc931a08d4e49f279f741ac.
There were 19 benchmark results indicating a performance regression:
- Pull Request Run on `arm64-t4g-2xlarge-linux` at [2025-10-14 16:47:37Z](https://conbench.ursa.dev/compare/runs/f2f406dc492a4647adea478f9c5a872e...8f3f65d9005947df87cb2c0cefdd12c3/)
- [`BM_UnpackUint64` (C++) with params=DynamicAligned/20/64, source=cpp-micro, suite=arrow-bpacking-benchmark](https://conbench.ursa.dev/compare/benchmarks/068ed0c742c174a78000ef20a9c0e693...068ee7f5c5f1725f800083cc29247e97)
- [`BM_UnpackUint64` (C++) with params=DynamicUnaligned/1/64, source=cpp-micro, suite=arrow-bpacking-benchmark](https://conbench.ursa.dev/compare/benchmarks/068ed0c7483b7ede800009044d253e58...068ee7f5ca78747a8000981b2bac8375)
- and 17 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/52730340522) has more 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3410978099
Revision: 38aa0522220e6aba634cdb287850c44b6b256c43
Submitted crossbow builds: [ursacomputing/crossbow @ actions-beeb73de91](https://github.com/ursacomputing/crossbow/branches/all?query=actions-beeb73de91)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456163/job/52918050463)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/18563457122/job/52918053314)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/18563457029/job/52918052750)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456872/job/52918052858)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456650/job/52918051428)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/18563455784/job/52918050379)|
|test-cuda-cpp-ubuntu-22.04-cuda-11.7.1|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456557/job/52918051153)|
|test-debian-12-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456250/job/52918050266)|
|test-debian-12-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456776/job/52918052058)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456015/job/52918050073)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456017/job/52918049709)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456794/job/52918052017)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456744/job/52918054294)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456140/job/52918049533)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456858/job/52918052205)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/18563455752/job/52918048621)|
|test-ubuntu-24.04-cpp-bundled-offline|[](https://github.com/ursacomputing/crossbow/actions/runs/18563455927/job/52918048844)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/18563455753/job/52918048535)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456451/job/52918050953)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/18563456916/job/52918052403)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/18563455861/job/52918048839)|
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Generalize bpacking and possible speedup [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3298433063
:warning: GitHub issue #47572 **has been automatically assigned in GitHub** to PR creator.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432479467
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
Review Comment:
But as we discussed, `sizeof(bool)` is not necessarily 1.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432485283
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
Review Comment:
Hmm, ok, so let's perhaps remove the snippet I was commenting on?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3406771727
> One hypothesis I'm wondering about is whether mixing scalar code with SIMD introduce additional latency.
Indeed, when looking at the disassembly above, I noticed that many/most instructions actually just do scalar reads + scalar-to-vector register moves to load the data in the SIMD registers.
> For comparison, the Lemire implementation I'm curious about is loading data once with a `load_unaligned`, then using a swizzle (byte reorder), then a rshift, then a mask.
That's probably the ideal implementation indeed. However, one must be careful to not read data out of bounds, which introduces some complication. Perhaps the various `unpack` specializations would need to take an additional argument indicating available padding.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2431370659
##########
.gitattributes:
##########
@@ -1,4 +1,4 @@
-cpp/src/arrow/util/bpacking_*_generated.h linguist-generated=true
+cpp/src/arrow/util/bpacking_*_generated_internal.h linguist-generated=true
Review Comment:
I copy pasted + `ls` in my terminal and it shows the relevant files (no typos).
Perhaps it will only be used by Github in the next PR.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2431698433
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
Review Comment:
(or perhaps this part is never used because `bool` actually unpacks to `uint32_t` before converting?)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3401219805
I'm testing performance of this PR locally on a AMD Zen 2 CPU on Ubuntu, and I'm seeing large regressions on unpack32 performance.
1) scalar unpack32 regresses on all input sizes:
* before:
```
BM_UnpackUint32/ScalarUnaligned/1/64 23.3 ns 23.3 ns 30099581 items_per_second=2.74685G/s
BM_UnpackUint32/ScalarUnaligned/2/64 15.1 ns 15.1 ns 45528078 items_per_second=4.22707G/s
BM_UnpackUint32/ScalarUnaligned/8/64 16.2 ns 16.1 ns 43418304 items_per_second=3.96297G/s
BM_UnpackUint32/ScalarUnaligned/20/64 22.4 ns 22.4 ns 31217249 items_per_second=2.85244G/s
BM_UnpackUint32/ScalarUnaligned/1/1024 310 ns 310 ns 2251073 items_per_second=3.29834G/s
BM_UnpackUint32/ScalarUnaligned/2/1024 213 ns 213 ns 3288295 items_per_second=4.80898G/s
BM_UnpackUint32/ScalarUnaligned/8/1024 357 ns 357 ns 1957044 items_per_second=2.86556G/s
BM_UnpackUint32/ScalarUnaligned/20/1024 339 ns 339 ns 2066262 items_per_second=3.01942G/s
BM_UnpackUint32/ScalarUnaligned/1/32768 9829 ns 9828 ns 71098 items_per_second=3.33403G/s
BM_UnpackUint32/ScalarUnaligned/2/32768 6543 ns 6543 ns 107368 items_per_second=5.00848G/s
BM_UnpackUint32/ScalarUnaligned/8/32768 11410 ns 11410 ns 60956 items_per_second=2.87199G/s
BM_UnpackUint32/ScalarUnaligned/20/32768 10737 ns 10736 ns 65168 items_per_second=3.05219G/s
```
* after:
```
BM_UnpackUint32/ScalarUnaligned/1/64 26.3 ns 26.3 ns 26597461 items_per_second=2.43124G/s
BM_UnpackUint32/ScalarUnaligned/2/64 25.9 ns 25.9 ns 27099769 items_per_second=2.47115G/s
BM_UnpackUint32/ScalarUnaligned/8/64 23.0 ns 23.0 ns 30377366 items_per_second=2.78236G/s
BM_UnpackUint32/ScalarUnaligned/20/64 41.1 ns 41.1 ns 17029900 items_per_second=1.55726G/s
BM_UnpackUint32/ScalarUnaligned/1/1024 368 ns 368 ns 1903394 items_per_second=2.78384G/s
BM_UnpackUint32/ScalarUnaligned/2/1024 342 ns 342 ns 2047462 items_per_second=2.99373G/s
BM_UnpackUint32/ScalarUnaligned/8/1024 358 ns 358 ns 1956688 items_per_second=2.86192G/s
BM_UnpackUint32/ScalarUnaligned/20/1024 538 ns 538 ns 1306908 items_per_second=1.90488G/s
BM_UnpackUint32/ScalarUnaligned/1/32768 11647 ns 11646 ns 59931 items_per_second=2.81379G/s
BM_UnpackUint32/ScalarUnaligned/2/32768 10844 ns 10843 ns 64537 items_per_second=3.02197G/s
BM_UnpackUint32/ScalarUnaligned/8/32768 11303 ns 11301 ns 61943 items_per_second=2.89948G/s
BM_UnpackUint32/ScalarUnaligned/20/32768 16915 ns 16915 ns 41375 items_per_second=1.93724G/s
```
2) SIMD unpack32 regresses, but only a small input sizes:
* before:
```
BM_UnpackUint32/Avx2Unaligned/1/64 6.10 ns 6.10 ns 115307711 items_per_second=10.4919G/s
BM_UnpackUint32/Avx2Unaligned/2/64 5.80 ns 5.80 ns 119616924 items_per_second=11.0319G/s
BM_UnpackUint32/Avx2Unaligned/8/64 7.84 ns 7.83 ns 89310891 items_per_second=8.16917G/s
BM_UnpackUint32/Avx2Unaligned/20/64 30.1 ns 30.1 ns 23168069 items_per_second=2.12844G/s
BM_UnpackUint32/Avx2Unaligned/1/1024 61.3 ns 61.3 ns 11549890 items_per_second=16.7084G/s
BM_UnpackUint32/Avx2Unaligned/2/1024 60.4 ns 60.4 ns 11462262 items_per_second=16.9551G/s
BM_UnpackUint32/Avx2Unaligned/8/1024 118 ns 118 ns 5936723 items_per_second=8.68313G/s
BM_UnpackUint32/Avx2Unaligned/20/1024 426 ns 426 ns 1628900 items_per_second=2.40438G/s
BM_UnpackUint32/Avx2Unaligned/1/32768 1916 ns 1916 ns 364744 items_per_second=17.1002G/s
BM_UnpackUint32/Avx2Unaligned/2/32768 2071 ns 2071 ns 340164 items_per_second=15.8259G/s
BM_UnpackUint32/Avx2Unaligned/8/32768 3814 ns 3814 ns 183339 items_per_second=8.59169G/s
BM_UnpackUint32/Avx2Unaligned/20/32768 13674 ns 13672 ns 51167 items_per_second=2.39671G/s
```
* after:
```
BM_UnpackUint32/Avx2Unaligned/1/64 11.7 ns 11.7 ns 60043243 items_per_second=5.47447G/s
BM_UnpackUint32/Avx2Unaligned/2/64 11.7 ns 11.7 ns 59912039 items_per_second=5.47921G/s
BM_UnpackUint32/Avx2Unaligned/8/64 13.4 ns 13.3 ns 52435550 items_per_second=4.79412G/s
BM_UnpackUint32/Avx2Unaligned/20/64 37.6 ns 37.6 ns 18551367 items_per_second=1.69999G/s
BM_UnpackUint32/Avx2Unaligned/1/1024 68.2 ns 68.2 ns 10297857 items_per_second=15.0158G/s
BM_UnpackUint32/Avx2Unaligned/2/1024 68.2 ns 68.2 ns 10265410 items_per_second=15.0135G/s
BM_UnpackUint32/Avx2Unaligned/8/1024 122 ns 122 ns 5723819 items_per_second=8.37532G/s
BM_UnpackUint32/Avx2Unaligned/20/1024 460 ns 460 ns 1521877 items_per_second=2.2277G/s
BM_UnpackUint32/Avx2Unaligned/1/32768 1958 ns 1957 ns 357945 items_per_second=16.7407G/s
BM_UnpackUint32/Avx2Unaligned/2/32768 1939 ns 1939 ns 361274 items_per_second=16.9008G/s
BM_UnpackUint32/Avx2Unaligned/8/32768 3831 ns 3830 ns 182683 items_per_second=8.55518G/s
BM_UnpackUint32/Avx2Unaligned/20/32768 14430 ns 14429 ns 48513 items_per_second=2.27106G/s
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3401365958
A quick experiment shows that much of the original performance can be re-gained by replacing the array of function pointers with a switch statement in `unpack_jump`. It's less pretty for sure.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2428947100
##########
cpp/apidoc/Doxyfile:
##########
@@ -1095,6 +1095,7 @@ EXCLUDE_PATTERNS = *-test.cc \
*test* \
*_generated.h \
*-benchmark.cc \
+ *_codegen.py \
Review Comment:
I was too but it was giving me trouble with it
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3402430291
@ursabot please benchmark lang=C++
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432462429
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
Review Comment:
This is seldom used to generate the `bool` wrapper that unpack to `uint32_t` + `static_cast` loop.
On the one hand the code generator is different from the general one, on the other, there are functions to reuse.
But it is consistent with the definition of `out_bit_width` which always means `8*sizeof(UnpackedType)`.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432474259
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
Review Comment:
Actually it gets generated below from `print_note()`. This was the original source comment.
```
// WARNING: this file is generated, DO NOT EDIT.
// Usage:
// python cpp/src/arrow/util/bpacking_scalar_codegen.py
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432474259
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
Review Comment:
Actually it's below, this was the original source comment.
```
// WARNING: this file is generated, DO NOT EDIT.
// Usage:
// python cpp/src/arrow/util/bpacking_scalar_codegen.py
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432787324
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
Review Comment:
Well, I think this is because 32 words of N bits do not always fall on a 64-bit boundary, so we need a 32-bit read for the last word.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2435352792
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
+
+ @property
+ def out_bit_width(self) -> int:
+ if self.out_type == "bool":
+ return 8
Review Comment:
> But as we discussed, sizeof(bool) is not necessarily 1.
Sorry I missed that. Can we safely assume `sizeof(bool) < 8`?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2435609840
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
Review Comment:
Do you want to add a comment explaining 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3410967130
@github-actions crossbow submit -g cpp
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432540073
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+"""
+
+HEADER = """
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+namespace {
+
+template <typename Int>
+Int LoadInt(const uint8_t* in) {
+ return bit_util::FromLittleEndian(util::SafeLoadAs<Int>(in));
+}
+"""
+
+FOOTER = """
+} // namespace
+} // namespace arrow::internal
+"""
+
+
+@dataclasses.dataclass
+class ScalarUnpackGenerator:
+ out_type: str
+ smart_halve: bool
Review Comment:
I don't fully either, it's reverse engineering of this and the Lemire source for extending this script to `uint32_t` (which did not exist).
For some reason, on `uint64_t`, we can load data as `uint32_t` on odd bit widths ([original use](https://github.com/apache/arrow/blob/a53ebde62053a6fc791fc7d8a8280167d5f3fda7/cpp/src/arrow/util/bpacking64_codegen.py#L101)).
This logic does not work out on `uint32_t` and `uint_16_t` (for which the script was extended) so this option was added to exclude it.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3402660087
This looks pretty good now! I still have to take a look at the generation scripts, though.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2428640481
##########
cpp/apidoc/Doxyfile:
##########
@@ -1095,6 +1095,7 @@ EXCLUDE_PATTERNS = *-test.cc \
*test* \
*_generated.h \
*-benchmark.cc \
+ *_codegen.py \
Review Comment:
I'm surprised, does Doxygen actually look at `*.py` files?
##########
cpp/src/arrow/util/bpacking_dispatch_internal.h:
##########
@@ -0,0 +1,104 @@
+// 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.
+
+#pragma once
+
+#include <cstring>
+#include <utility>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/logging.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+
+/// Unpack a zero bit packed array.
+template <typename Uint>
+int unpack_null(const uint8_t* in, Uint* out, int batch_size) {
+ std::memset(out, 0, batch_size * sizeof(Uint));
+ return batch_size;
+}
+
+/// Unpack a packed array where packed and unpacked values have exactly the same number of
+/// bits.
+template <typename Uint>
+int unpack_full(const uint8_t* in, Uint* out, int batch_size) {
+ if constexpr (ARROW_LITTLE_ENDIAN == 1) {
+ std::memcpy(out, in, batch_size * sizeof(Uint));
+ } else {
+ using bit_util::FromLittleEndian;
+ using util::SafeLoadAs;
+
+ for (int k = 0; k < batch_size; k += 1) {
+ out[k] = FromLittleEndian(SafeLoadAs<Uint>(in + (k * sizeof(Uint))));
+ }
+ }
+ return batch_size;
+}
+
+/// Unpack a packed array, delegating to a Unpacker struct.
+///
+/// @tparam kPackedBitWidth The width in bits of the values in the packed array.
+/// @tparam Unpacker The struct providing information and an ``unpack`` method to unpack a
+/// fixed amount of values (usually constrained by SIMD batch sizes and
+/// byte alignment).
+/// @tparam UnpackedUInt The type in which we unpack the values.
+template <int kPackedBitWidth, template <typename, int> typename Unpacker,
+ typename UnpackedUInt>
+int unpack_width(const uint8_t* in, UnpackedUInt* out, int batch_size) {
+ using UnpackerForWidth = Unpacker<UnpackedUInt, kPackedBitWidth>;
+
+ constexpr auto kValuesUnpacked = UnpackerForWidth::kValuesUnpacked;
Review Comment:
Should we `ARROW_DCHECK_EQ(batch_size % kValuesUnpacked, 0)`?
##########
cpp/src/arrow/util/bpacking_scalar_generated_internal.h:
##########
@@ -0,0 +1,5832 @@
+// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
+// Original source:
+// https://github.com/lemire/FrameOfReference/blob/master/scripts/turbopacking64.py
+// The original copyright notice follows.
+
+// This code is released under the
+// Apache License Version 2.0 http://www.apache.org/licenses/.
+// (c) Daniel Lemire 2013
+
+// WARNING: this file is generated, DO NOT EDIT.
+// Usage:
+// python cpp/src/arrow/util/bpacking_scalar_codegen.py
+
+#pragma once
+
+#include <cstdint>
+#include <cstring>
+
+#include "arrow/util/endian.h"
+#include "arrow/util/ubsan.h"
+
+namespace arrow::internal {
+
+template <typename Int>
Review Comment:
Perhaps try to put all this in the anonymous namespace to allow for potentially more optimizations?
##########
cpp/src/arrow/util/bpacking_scalar_internal.h:
##########
@@ -23,7 +23,20 @@
namespace arrow::internal {
-ARROW_EXPORT int unpack32_neon(const uint8_t* in, uint32_t* out, int batch_size,
+template <typename Uint>
+ARROW_EXPORT int unpack_scalar(const uint8_t* in, Uint* out, int batch_size,
int num_bits);
+extern template ARROW_TEMPLATE_EXPORT int unpack_scalar<uint8_t>(const uint8_t*, uint8_t*,
+ int, int);
Review Comment:
Same remark here wrt. parameter names.
##########
cpp/src/arrow/util/bpacking_scalar_generated_internal.h:
##########
Review Comment:
Can you update the `.gitattributes` file so that this file is considered generated?
##########
cpp/src/arrow/util/bpacking_internal.h:
##########
@@ -23,17 +23,19 @@
namespace arrow::internal {
-/// The scalar 32 bit unpacking.
-ARROW_EXPORT int unpack32_scalar(const uint8_t* in, uint32_t* out, int batch_size,
- int num_bits);
-
-/// The scalar 64 bit unpacking.
-ARROW_EXPORT int unpack64_scalar(const uint8_t* in, uint64_t* out, int batch_size,
- int num_bits);
-
-ARROW_EXPORT
-int unpack32(const uint8_t* in, uint32_t* out, int batch_size, int num_bits);
-ARROW_EXPORT
-int unpack64(const uint8_t* in, uint64_t* out, int batch_size, int num_bits);
+template <typename Uint>
+ARROW_EXPORT int unpack(const uint8_t* in, Uint* out, int batch_size, int num_bits);
+
+extern template ARROW_TEMPLATE_EXPORT int unpack<uint8_t>(const uint8_t*, uint8_t*, int,
+ int);
Review Comment:
Could we keep the parameter names explicit in the signature? The last two `int`s especially are not self-describing.
##########
cpp/src/arrow/CMakeLists.txt:
##########
@@ -490,6 +490,8 @@ set(ARROW_UTIL_SRCS
util/bitmap_builders.cc
util/bitmap_ops.cc
util/bpacking.cc
+ util/bpacking_scalar.cc
+ util/bpacking_simd_min.cc
Review Comment:
What is "min" for?
##########
cpp/src/arrow/util/bpacking.cc:
##########
@@ -15,381 +15,53 @@
// specific language governing permissions and limitations
// under the License.
-#include "arrow/util/bpacking_internal.h"
+#include <array>
-#include "arrow/util/bpacking64_default_internal.h"
-#include "arrow/util/bpacking_default_internal.h"
-#include "arrow/util/cpu_info.h"
+#include "arrow/util/bpacking_dispatch_internal.h"
+#include "arrow/util/bpacking_internal.h"
+#include "arrow/util/bpacking_scalar_internal.h"
+#include "arrow/util/bpacking_simd_internal.h"
#include "arrow/util/dispatch_internal.h"
-#include "arrow/util/logging_internal.h"
-
-#if defined(ARROW_HAVE_RUNTIME_AVX2)
-# include "arrow/util/bpacking_avx2_internal.h"
-#endif
-#if defined(ARROW_HAVE_RUNTIME_AVX512)
-# include "arrow/util/bpacking_avx512_internal.h"
-#endif
-#if defined(ARROW_HAVE_NEON)
-# include "arrow/util/bpacking_neon_internal.h"
-#endif
-
-namespace arrow {
-namespace internal {
-
-int unpack32_scalar(const uint8_t* in_, uint32_t* out, int batch_size, int num_bits) {
- const uint32_t* in = reinterpret_cast<const uint32_t*>(in_);
- batch_size = batch_size / 32 * 32;
- int num_loops = batch_size / 32;
-
- switch (num_bits) {
- case 0:
- for (int i = 0; i < num_loops; ++i) in = nullunpacker32(in, out + i * 32);
- break;
- case 1:
- for (int i = 0; i < num_loops; ++i) in = unpack1_32(in, out + i * 32);
- break;
- case 2:
- for (int i = 0; i < num_loops; ++i) in = unpack2_32(in, out + i * 32);
- break;
- case 3:
- for (int i = 0; i < num_loops; ++i) in = unpack3_32(in, out + i * 32);
- break;
- case 4:
- for (int i = 0; i < num_loops; ++i) in = unpack4_32(in, out + i * 32);
- break;
- case 5:
- for (int i = 0; i < num_loops; ++i) in = unpack5_32(in, out + i * 32);
- break;
- case 6:
- for (int i = 0; i < num_loops; ++i) in = unpack6_32(in, out + i * 32);
- break;
- case 7:
- for (int i = 0; i < num_loops; ++i) in = unpack7_32(in, out + i * 32);
- break;
- case 8:
- for (int i = 0; i < num_loops; ++i) in = unpack8_32(in, out + i * 32);
- break;
- case 9:
- for (int i = 0; i < num_loops; ++i) in = unpack9_32(in, out + i * 32);
- break;
- case 10:
- for (int i = 0; i < num_loops; ++i) in = unpack10_32(in, out + i * 32);
- break;
- case 11:
- for (int i = 0; i < num_loops; ++i) in = unpack11_32(in, out + i * 32);
- break;
- case 12:
- for (int i = 0; i < num_loops; ++i) in = unpack12_32(in, out + i * 32);
- break;
- case 13:
- for (int i = 0; i < num_loops; ++i) in = unpack13_32(in, out + i * 32);
- break;
- case 14:
- for (int i = 0; i < num_loops; ++i) in = unpack14_32(in, out + i * 32);
- break;
- case 15:
- for (int i = 0; i < num_loops; ++i) in = unpack15_32(in, out + i * 32);
- break;
- case 16:
- for (int i = 0; i < num_loops; ++i) in = unpack16_32(in, out + i * 32);
- break;
- case 17:
- for (int i = 0; i < num_loops; ++i) in = unpack17_32(in, out + i * 32);
- break;
- case 18:
- for (int i = 0; i < num_loops; ++i) in = unpack18_32(in, out + i * 32);
- break;
- case 19:
- for (int i = 0; i < num_loops; ++i) in = unpack19_32(in, out + i * 32);
- break;
- case 20:
- for (int i = 0; i < num_loops; ++i) in = unpack20_32(in, out + i * 32);
- break;
- case 21:
- for (int i = 0; i < num_loops; ++i) in = unpack21_32(in, out + i * 32);
- break;
- case 22:
- for (int i = 0; i < num_loops; ++i) in = unpack22_32(in, out + i * 32);
- break;
- case 23:
- for (int i = 0; i < num_loops; ++i) in = unpack23_32(in, out + i * 32);
- break;
- case 24:
- for (int i = 0; i < num_loops; ++i) in = unpack24_32(in, out + i * 32);
- break;
- case 25:
- for (int i = 0; i < num_loops; ++i) in = unpack25_32(in, out + i * 32);
- break;
- case 26:
- for (int i = 0; i < num_loops; ++i) in = unpack26_32(in, out + i * 32);
- break;
- case 27:
- for (int i = 0; i < num_loops; ++i) in = unpack27_32(in, out + i * 32);
- break;
- case 28:
- for (int i = 0; i < num_loops; ++i) in = unpack28_32(in, out + i * 32);
- break;
- case 29:
- for (int i = 0; i < num_loops; ++i) in = unpack29_32(in, out + i * 32);
- break;
- case 30:
- for (int i = 0; i < num_loops; ++i) in = unpack30_32(in, out + i * 32);
- break;
- case 31:
- for (int i = 0; i < num_loops; ++i) in = unpack31_32(in, out + i * 32);
- break;
- case 32:
- for (int i = 0; i < num_loops; ++i) in = unpack32_32(in, out + i * 32);
- break;
- default:
- DCHECK(false) << "Unsupported num_bits";
- }
-
- return batch_size;
-}
+namespace arrow::internal {
namespace {
-struct Unpack32DynamicFunction {
- using FunctionType = decltype(&unpack32_scalar);
+template <typename Uint>
+struct UnpackDynamicFunction {
+ using FunctionType = decltype(&unpack_scalar<Uint>);
+ using Implementation = std::pair<DispatchLevel, FunctionType>;
- static std::vector<std::pair<DispatchLevel, FunctionType>> implementations() {
- return {{DispatchLevel::NONE, unpack32_scalar}
+ static auto implementations() {
Review Comment:
Nit: this might be `constexpr`?
##########
cpp/src/arrow/util/bit_stream_utils_internal.h:
##########
@@ -296,6 +296,18 @@ inline bool BitReader::GetValue(int num_bits, T* v) {
return GetBatch(num_bits, v, 1) == 1;
}
+namespace internal_bit_reader {
+template <typename T>
+struct unpack_detect {
+ using type = std::make_unsigned_t<T>;
+};
+
+template <>
+struct unpack_detect<bool> {
+ using type = uint8_t;
+};
Review Comment:
This seems to assume that `bool` always occupies a single byte in memory, but the C++ standard [does not guarantee](https://en.cppreference.com/w/cpp/language/types.html#Boolean_type) anything about it:
> The value of sizeof(bool) is implementation defined and might differ from 1.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3401369526
@pitrou what about the Read/Decoder benchmarks?
The algorithms are mostly similar, except:
- The way pointers are incremented: using `out[0]`, `out[1]`... instead of `out++ =`
- The function names and how they are grouped in files.
- The jump table (that should have a bigger relatitve impact for SIMD since they are faster).
I'm gonna investigate but I suspect these benchmarks are highly unstable.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2429001616
##########
cpp/src/arrow/util/bpacking_scalar_internal.h:
##########
@@ -23,7 +23,20 @@
namespace arrow::internal {
-ARROW_EXPORT int unpack32_neon(const uint8_t* in, uint32_t* out, int batch_size,
+template <typename Uint>
+ARROW_EXPORT int unpack_scalar(const uint8_t* in, Uint* out, int batch_size,
int num_bits);
+extern template ARROW_TEMPLATE_EXPORT int unpack_scalar<uint8_t>(const uint8_t*, uint8_t*,
Review Comment:
Ah. Well, we can keep it like this for now if that doesn't degrade performance...
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #47573:
URL: https://github.com/apache/arrow/pull/47573
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3412978988
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 7a38744e979def14885c9ced423771af0916090d.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/52960497707) has more details. It also includes information about 17 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2428963471
##########
cpp/src/arrow/util/bit_stream_utils_internal.h:
##########
@@ -296,6 +296,18 @@ inline bool BitReader::GetValue(int num_bits, T* v) {
return GetBatch(num_bits, v, 1) == 1;
}
+namespace internal_bit_reader {
+template <typename T>
+struct unpack_detect {
+ using type = std::make_unsigned_t<T>;
+};
+
+template <>
+struct unpack_detect<bool> {
+ using type = uint8_t;
+};
Review Comment:
Good catch! Well since `unpack<uint8_t>` has a `static_cast` loop that's gonna be easy to extend.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3405482969
For the record, when disassembling `libarrow.so` locally, it turns out that all the unpack functions are inlined inside `arrow::internal::unpack_avx2<unsigned int>`.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3405559075
Also some statistics on the generated assembly code:
```console
# Number of scalar shifts and ORs
$ objdump -w --demangle --no-addresses --no-show-raw-insn --disassemble='int arrow::internal::unpack_avx2<unsigned int>(unsigned char const*, unsigned int*, int, int)' /build/build-release/relwithdebinfo/libarrow.so | rg -e '\b(sh[lr]|or)' -c
430
# Number of instructions moving data around
$ objdump -w --demangle --no-addresses --no-show-raw-insn --disassemble='int arrow::internal::unpack_avx2<unsigned int>(unsigned char const*, unsigned int*, int, int)' /build/build-release/relwithdebinfo/libarrow.so | rg -e '\b(mov|vmov|vinsert|vpinsr|vpunpck|vpbroadcast)' -c
2290
```
Based on these numbers, it's not obvious that eliminating the remaining scalar shifts and ORs (when a bit-packed value straddles an input word boundary) would really improve performance, as they would be replaced by additional register moves to populate a second vector register.
<details>
Basically the idea would be to replace (example of unpacking 11-bit values into 32-bit output):
```c++
// extract 11-bit bundles 0 to 7
words = simd_batch{
SafeLoadAs<uint32_t>(in + 4 * 0),
SafeLoadAs<uint32_t>(in + 4 * 0),
SafeLoadAs<uint32_t>(in + 4 * 0) >> 22 | SafeLoadAs<uint32_t>(in + 4 * 1) << 10,
SafeLoadAs<uint32_t>(in + 4 * 1),
SafeLoadAs<uint32_t>(in + 4 * 1),
SafeLoadAs<uint32_t>(in + 4 * 1) >> 23 | SafeLoadAs<uint32_t>(in + 4 * 2) << 9,
SafeLoadAs<uint32_t>(in + 4 * 2),
SafeLoadAs<uint32_t>(in + 4 * 2),
};
shifts = simd_batch{ 0, 11, 0, 1, 12, 0, 2, 13 };
results = (words >> shifts) & masks;
results.store_unaligned(out);
```
with:
```c++
// extract 11-bit bundles 0 to 7
words1 = simd_batch{
SafeLoadAs<uint32_t>(in + 4 * 0),
SafeLoadAs<uint32_t>(in + 4 * 0),
SafeLoadAs<uint32_t>(in + 4 * 0),
SafeLoadAs<uint32_t>(in + 4 * 1),
SafeLoadAs<uint32_t>(in + 4 * 1),
SafeLoadAs<uint32_t>(in + 4 * 1),
SafeLoadAs<uint32_t>(in + 4 * 2),
SafeLoadAs<uint32_t>(in + 4 * 2),
} >> simd_batch{ 0, 0, 22, 0, 0, 23, 0, 0};
words2 = simd_batch{
SafeLoadAs<uint32_t>(in + 4 * 0),
SafeLoadAs<uint32_t>(in + 4 * 0),
SafeLoadAs<uint32_t>(in + 4 * 1),
SafeLoadAs<uint32_t>(in + 4 * 1),
SafeLoadAs<uint32_t>(in + 4 * 1),
SafeLoadAs<uint32_t>(in + 4 * 2),
SafeLoadAs<uint32_t>(in + 4 * 2),
SafeLoadAs<uint32_t>(in + 4 * 2),
} << simd_batch{ 0, 0, 10, 0, 0, 9, 0, 0};;
shifts = simd_batch{ 0, 11, 0, 1, 12, 0, 2, 13 };
results = ((words1 | words2) >> shifts) & masks;
results.store_unaligned(out);
```
</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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2432474259
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
Review Comment:
Actually it gets generated below from `print_note()`. Above was the original source comment.
```
// WARNING: this file is generated, DO NOT EDIT.
// Usage:
// python cpp/src/arrow/util/bpacking_scalar_codegen.py
```
##########
cpp/src/arrow/util/bpacking_scalar_codegen.py:
##########
@@ -0,0 +1,264 @@
+#!/bin/python
+
+# 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.
+
+# This script is modified from its original version in GitHub. Original source:
+# https://github.com/lemire/FrameOfReference/blob/146948b6058a976bc7767262ad3a2ce201486b93/scripts/turbopacking64.py
+
+# Usage:
+# python bpacking64_codegen.py > bpacking64_default_internal.h
+
+
+import dataclasses
+import sys
+
+
+LICENSE = """// 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.
+
+// This file was generated by script which is modified from its original version in GitHub.
Review Comment:
Actually it gets generated below from `print_note()`. Above was the original source comment.
```cpp
// WARNING: this file is generated, DO NOT EDIT.
// Usage:
// python cpp/src/arrow/util/bpacking_scalar_codegen.py
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "voltrondatabot (via GitHub)" <gi...@apache.org>.
voltrondatabot commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3402430685
Benchmark runs are scheduled for commit cad05efb3946f864bdc931a08d4e49f279f741ac. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #47573:
URL: https://github.com/apache/arrow/pull/47573#discussion_r2429738842
##########
cpp/src/arrow/util/bpacking_test.cc:
##########
@@ -201,52 +198,116 @@ class TestUnpack : public ::testing::TestWithParam<TestUnpackSize> {
template <typename Int>
void TestAll(UnpackFunc<Int> unpack) {
- // Known values
- TestUnpackZeros(unpack);
- TestUnpackOnes(unpack);
- TestUnpackAlternating(unpack);
-
- // Roundtrips
- TestRoundtripAlignment(unpack, /* alignment_offset= */ 0);
- TestRoundtripAlignment(unpack, /* alignment_offset= */ 1);
+ constexpr int kMaxBitWidth = std::is_same_v<Int, bool> ? 1 : 8 * sizeof(Int);
+ // Given how many edge cases there are in unpacking integers, it is best to test all
+ // sizes
+ for (int bit_width = 0; bit_width <= kMaxBitWidth; ++bit_width) {
+ SCOPED_TRACE(::testing::Message() << "Testing bit_width=" << bit_width);
+
+ // Known values
+ TestUnpackZeros(unpack, bit_width);
+ TestUnpackOnes(unpack, bit_width);
+ TestUnpackAlternating(unpack, bit_width);
+
+ // Roundtrips
+ TestRoundtripAlignment(unpack, bit_width, /* alignment_offset= */ 0);
+ TestRoundtripAlignment(unpack, bit_width, /* alignment_offset= */ 1);
+ }
}
};
-INSTANTIATE_TEST_SUITE_P(
- UnpackMultiplesOf64Values, TestUnpack,
- ::testing::Values(TestUnpackSize{64, 1}, TestUnpackSize{128, 1},
- TestUnpackSize{2048, 1}, TestUnpackSize{64, 31},
- TestUnpackSize{128, 31}, TestUnpackSize{2048, 1},
- TestUnpackSize{2048, 8}, TestUnpackSize{2048, 13},
- TestUnpackSize{2048, 16}, TestUnpackSize{2048, 31},
- TestUnpackSize{2048, 32}));
+// There are actually many differences across the different sizes.
+// It is best to test them all.
+INSTANTIATE_TEST_SUITE_P(UnpackMultiplesOf64Values, TestUnpack,
+ ::testing::Values(64, 128, 2048),
+ [](const ::testing::TestParamInfo<TestUnpack::ParamType>& info) {
+ return "Count" + std::to_string(info.param);
Review Comment:
Nit: perhaps `Length=` instead of `Count`, for clarity.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Generalize bpacking and possible speedup [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3398204626
- The dispatch once and `uint16_t` implementation were removed as they turned out to be slower.
- Added an unpack implementation for `uint8_t` and `uint16_t` that call the `uint32_t` version on a local buffer combined with a `static_cast` loop (what was done on the call site before).
- The performances should remain on par with previous implementations. The PR as it is now mainly changes the interface of unpack functions for future iterations and cleaner 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-47572: [C++][Parquet] Uniform unpack interface [arrow]
Posted by "AntoinePrv (via GitHub)" <gi...@apache.org>.
AntoinePrv commented on PR #47573:
URL: https://github.com/apache/arrow/pull/47573#issuecomment-3410182404
Sorry, I missed that in the notifications.
I pushed a commit for sizeof bool and the top level comment.
Was there anything else?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org