You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2023/01/04 13:28:03 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #3452: Remove multiversion dependency
tustvold opened a new pull request, #3452:
URL: https://github.com/apache/arrow-rs/pull/3452
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
We only use this in a single location, and in general recommend people compile with the appropriate `target-cpu`, as such I think we can remove this without any major issue.
Eliminating a dependency is always a good thing, especially as this results in additional codegen, and reduces our vulnerability to upstream shenanigans.
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1376374773
> It's OS specific (dlopen etc) and precludes users from making statically linked executables
I guess what I had in mind was literally create builds of the influxdb_iox binary for each architecture (well really probably "AVX512" and "SSE2", and maybe a third one in the middle).
As you note Rust doesn't have a stable ABI (and therefore will recompile almost everything from source each time anyways)
--
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
[GitHub] [arrow-rs] tustvold commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1371519508
> I don't think it's reasonable to expect users to use target-cpu beyond local testing or targeting a particular embedded system
I don't disagree that it is definitely not ideal, but it also is not really feasible for us to generate multiple versions of all our kernels, as we already have severe issues with the amount of codegen and the corresponding build times.
Correct me if I am mistaken, but if a user enabled AVX using `target-cpu` the produced binary will support 95% of the CPUs in the latest Steam Hardware Survey. A more aggressive `target-cpu=haswell` will still support almost all CPUs from this decade. I'm inclined to think most people can safely make this trade-off?
Ultimately whatever solution we come up with, we need to be able to apply consistently. I personally am eagerly awaiting stable SIMD support as despite a non-trivial amount of arcane hackery the LLVM generated code for many kernels is still sub-optimal, in some cases by orders of magnitude.
--
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
[GitHub] [arrow-rs] calebzulawski commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
calebzulawski commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1371486920
That's precisely the use-case for `multiversion`. I understand if `multiversion` isn't appropriate for this project, but I don't think it's reasonable to expect users to use `target-cpu` beyond local testing or targeting a particular embedded system.
> https://github.com/calebzulawski/target-features appears to be a one hit wonder crate (basically one version, no obvious plan for maintenance)
>
> https://github.com/calebzulawski/target-features/commits/master
>
> I am all for removing this dependency 👍
`target-features` was created mostly to address issues [discovered while creating the `std::simd` module](https://calebzulawski.github.io/rust-simd-book/4.2-native-vector-width.html) that didn't quite make it to inclusion in the standard library. I decided to keep it separate from `multiversion` because some users may want to access only the `target-features` hardware database without all of the multiversioning apparatus.
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1371580612
> I'm inclined to think most people can safely make this trade-off?
I personally think it is more beneficial to choose the appropriate target architecture higher up at the application level rather than in a lower level library.
For example, I would rather compile several versions of our top level `influxdb_iox` binary for various different architectures / instruction sets and pick between those binaries in a launcher rather than have the choice done inside of the low level arrow library
I would prefer the top level approach because more of my application could take advantage of the target cpu's instructions (via llvm's support for auto vectorization, for example), rather than just arrow.
--
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
[GitHub] [arrow-rs] ursabot commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1371101553
Benchmark runs are scheduled for baseline = 61a77a5afc2c141eb530f29ee6e99eafe6756170 and contender = ec2fd47046d341b41b9d2416f0c23b8d2e25008d. ec2fd47046d341b41b9d2416f0c23b8d2e25008d is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/dd10fd008aab4d80ae2b1e724d58c354...026f853a87cd4e10846b54caf27e1464/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/a22d40e776a943c69b1fe863cd411797...2f02e9d36d974f28a346bac8d8fa6838/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/f9d094c994e147489d7e7ca6215f1302...fad95f8b4f6b42ea95ec9e31e1308c20/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/72247c0dc15c4b31ae8e12b58d8da473...6a30d4fcccef4bea88ccd2e1727416e6/)
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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
[GitHub] [arrow-rs] Dandandan commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
Dandandan commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1371427597
> It would be nice to ping whoever initially added the multiversion dependency as a courtesy -- I didn't do enough code archeology to really figure it out
I believe I did. The goal was to be able to use more SIMD for some kernels.
The benefit I see of using `multiversion` would be compiling for a somewhat older CPU, while benefiting from runtime feature detection for newer CPUs (and not having to build an unportable binary. Although the use now is very limited...
--
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
[GitHub] [arrow-rs] alamb commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1371302823
Related upstream ticket: https://github.com/calebzulawski/target-features/issues/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
[GitHub] [arrow-rs] alamb commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
alamb commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1371504338
Thank you for the response @calebzulawski
--
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
[GitHub] [arrow-rs] tustvold merged pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452
--
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
[GitHub] [arrow-rs] calebzulawski commented on pull request #3452: Remove multiversion dependency
Posted by GitBox <gi...@apache.org>.
calebzulawski commented on PR #3452:
URL: https://github.com/apache/arrow-rs/pull/3452#issuecomment-1374949998
> > I don't think it's reasonable to expect users to use target-cpu beyond local testing or targeting a particular embedded system
>
> I don't disagree that it is definitely not ideal, but it also is not really feasible for us to generate multiple versions of all our kernels, as we already have severe issues with the amount of codegen and the corresponding build times.
>
> Correct me if I am mistaken, but if a user enabled AVX using `target-cpu` the produced binary will support 95% of the CPUs in the latest Steam Hardware Survey. A more aggressive `target-cpu=haswell` will still support almost all CPUs from this decade. I'm inclined to think most people can safely make this trade-off?
In this case, yes, AVX is commonly available. When AVX512 stabilizes in Rust, however, you would likely not want to build an entire application that requires it. If the amount of codegen is a concern then removing multiversioning can definitely help--you could also consider using a cargo feature to control whether or not multiversioning is present, and leave it up to the users. Another thing to consider is that particular software distributions (e.g. Linux) probably have particular CPUs they target, and won't be able to take advantage of `target-cpu`.
> > I'm inclined to think most people can safely make this trade-off?
>
> I personally think it is more beneficial to choose the appropriate target architecture higher up at the application level rather than in a lower level library.
>
> For example, I would rather compile several versions of our top level `influxdb_iox` binary for various different architectures / instruction sets and pick between those binaries in a launcher rather than have the choice done inside of the low level arrow library
>
> I would prefer the top level approach because more of my application could take advantage of the target cpu's instructions (via llvm's support for auto vectorization, for example), rather than just arrow.
That is another possibility, one that's used by Intel's MKL for example. I'm not sure it's the best approach, however:
* If the amount of codegen (or binary size) is a concern, this is the worst option, since all code is duplicated
* Rust still doesn't (and perhaps never will) have a stable ABI
* It's OS specific (dlopen etc) and precludes users from making statically linked executables
* The vast majority of code, from what I've seen, does not benefit from vectorization. MKL is a special case, where nearly every function it provides is vectorized.
With all of this in mind, I don't have any opinion on what is the best option for `arrow`, but it might be helpful if I summarized all of this in `multiversion`'s documentation.
--
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