Skip to content

ARROW-18350: [C++] Use std::to_chars instead of std::to_string - #14666

Merged
pitrou merged 5 commits into
apache:masterfrom
pitrou:ARROW-18350-to-chars
Nov 23, 2022
Merged

ARROW-18350: [C++] Use std::to_chars instead of std::to_string#14666
pitrou merged 5 commits into
apache:masterfrom
pitrou:ARROW-18350-to-chars

Conversation

@pitrou

@pitroupitrou commented Nov 17, 2022

Copy link
Copy Markdown
Member

std::to_chars is locale-independent unlike std::to_string; it may also be faster.

This PR does neither bother with test and benchmark files, nor with debug output strings.

Unfortunately, some standard libraries don't provide a full implementation of std::to_chars, in which case std::to_string is still used for the unimplemented input types.

@pitrou
pitrou requested a review from bkietzNovember 17, 2022 17:39
@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@pitrou

Copy link
Copy Markdown
MemberAuthor

Darn, std::to_chars is not implemented for floating-point on all C++ standard libraries :-(
https://stackoverflow.com/questions/63963961/what-is-the-correct-way-to-call-stdto-chars

@westonpacewestonpace left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks useful. Thanks!

Are there any cases where std::to_string should be preferred you think? Or, are most formatting functions that output to the user already going through some other kind of string formatting function?

Comment threadcpp/src/arrow/util/string.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this is redundant: https://godbolt.org/z/6vcW1jEfh

The call to resize should set out[length] to 0 so that std::string can fulfill its requirements for c_str.

That being said, it should be harmless and I imagine would optimize out.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hmm, you're right, we shouldn't need to do this on our own.

@pitrou

Copy link
Copy Markdown
MemberAuthor

Are there any cases where std::to_string should be preferred you think?

I don't think so (unless locale-dependence is desired).

Or, are most formatting functions that output to the user already going through some other kind of string formatting function?

Places where performance is important generally use our home-grown formatting routines, yes.

@pitrou

Copy link
Copy Markdown
MemberAuthor

@bkietz Can you think of a simple way to solve the incompleteness issue?

Darn, std::to_chars is not implemented for floating-point on all C++ standard libraries :-(

@westonpace

Copy link
Copy Markdown
Member

@pitrou maybe this (gist, godbolt)? It's a mix of sfinae (the int version is preferred over the long version) and constexpr.

`std::to_chars` is locale-independent unlike `std::to_string`; it may also be faster in some cases.
@pitrou

pitrou commented Nov 23, 2022

Copy link
Copy Markdown
MemberAuthor

@westonpace Thanks a lot. I'm trying to come up with a more condensed version.

@pitrou
pitrouforce-pushed the ARROW-18350-to-chars branch from 28e23d6 to 067bf1cCompareNovember 23, 2022 10:42
@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit -g cpp

@github-actions

This comment was marked as outdated.

@pitrou

Copy link
Copy Markdown
MemberAuthor

@github-actions crossbow submit -g cpp

@github-actions

This comment was marked as outdated.

@pitrou

Copy link
Copy Markdown
MemberAuthor

Wow, turns out <charconv> isn't even provided in some cases.

@pitrou

Copy link
Copy Markdown
MemberAuthor

Revision: 32f378c

Submitted crossbow builds: ursacomputing/crossbow @ ARROW-18350-to-chars-0

TaskStatus
test-alpine-linux-cppGithub Actions
test-build-cpp-fuzzGithub Actions
test-conda-cppGithub Actions
test-conda-cpp-valgrindAzure
test-cuda-cppGithub Actions
test-debian-10-cpp-amd64Github Actions
test-debian-10-cpp-i386Github Actions
test-debian-11-cpp-amd64Github Actions
test-debian-11-cpp-i386Github Actions
test-fedora-35-cppGithub Actions
test-ubuntu-18.04-cppGithub Actions
test-ubuntu-18.04-cpp-releaseGithub Actions
test-ubuntu-18.04-cpp-staticGithub Actions
test-ubuntu-20.04-cppGithub Actions
test-ubuntu-20.04-cpp-20Github Actions
test-ubuntu-20.04-cpp-bundledGithub Actions
test-ubuntu-20.04-cpp-thread-sanitizerGithub Actions
test-ubuntu-22.04-cppGithub Actions

@pitrou

Copy link
Copy Markdown
MemberAuthor

I'm gonna merge now that CI is green.

@pitrou
pitrou merged commit ad54d6c into apache:masterNov 23, 2022
@pitrou
pitrou deleted the ARROW-18350-to-chars branch November 23, 2022 14:49
@wgtmac

wgtmac commented Nov 24, 2022

Copy link
Copy Markdown
Member

On my Mac M1 laptop, the compilation looks good but linking complains about undefined symbols:

[ 63%] Built target arrow-io-file-test
[ 63%] Built target arrow-io-memory-test
[ 63%] Linking CXX executable ../../../debug/arrow-utility-test
Undefined symbols for architecture arm64:
"std::__1::to_chars(char*, char*, double)", referenced from:
std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > arrow::internal::ToChars<double>(double) in string_test.cc.o
"std::__1::to_chars(char*, char*, float)", referenced from:
std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > arrow::internal::ToChars<float>(float) in string_test.cc.o
ld: symbol(s) not found for architecture arm64
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [debug/arrow-utility-test] Error 1
make[1]: *** [src/arrow/util/CMakeFiles/arrow-utility-test.dir/all] Error 2
make: *** [all] Error 2

It looks like I am hitting an old issue: https://stackoverflow.com/questions/58923623/stdto-chars-compile-but-not-linking-on-macos-clang/59672017#59672017

BTW, my clang version is as below:

➜ clang -v
Homebrew clang version 14.0.6
Target: arm64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /opt/homebrew/opt/llvm@14/bin

@pitrou

Copy link
Copy Markdown
MemberAuthor

@wgtmac Sigh :-( As I understand it, you should be able to upgrade your Mac to a newer libc++ version (not sure how that's done, though)?

@ursabot

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 62829c5 and contender = ad54d6c. ad54d6c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.34% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.45% ⬆️0.07%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] ad54d6ca ec2-t3-xlarge-us-east-2
[Finished] ad54d6ca test-mac-arm
[Finished] ad54d6ca ursa-i9-9960x
[Finished] ad54d6ca ursa-thinkcentre-m75q
[Finished] 62829c5c ec2-t3-xlarge-us-east-2
[Finished] 62829c5c test-mac-arm
[Finished] 62829c5c ursa-i9-9960x
[Finished] 62829c5c ursa-thinkcentre-m75q
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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@pitrou@westonpace@wgtmac@ursabot