Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

maint: Replace use of folly getCurrentThreadId with STL #1417

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

ianthomas23
Copy link
Collaborator

Reference Issues/PRs

What does this implement or fix?

This is work to assist @jjerphan in removing use of folly in the ArcticDB code base.

The change here is to remove use of folly::getCurrentThreadID() in get_thread_id(), replacing it with use of std::this_thread::get_id().

This replaces #1408 and is submitted from a branch of the main repo rather than my fork so that CI runs automatically.

Any other comments?

The folly implementation is much more complicated as there are different implementation for the 3 major OSes:

https://github.com/facebook/folly/blob/69a1b93df285457cd0799a6898e2e5390a09cd73/folly/system/ThreadId.cpp#L29-L37

The different implementations are not required as the STL C++11 solution should work on all OSes. Following
#1408 (comment) the solution here returns a std::thread::id rather than a uint64_t and this is formatted correctly in log messages (the only use of thread ID) by including <fmt/std.h>.

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@ianthomas23 ianthomas23 mentioned this pull request Mar 13, 2024
17 tasks
@ianthomas23
Copy link
Collaborator Author

Some CI builds are passing and some failing. The failures are

error: invalid use of incomplete type ‘struct std::variant_size<long unsigned int>’
     struct variant_size<volatile _Variant> : variant_size<_Variant> {};
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I can reproduce this locally using a docker build whereas a conda build works fine (on Ubuntu 13.10).

I've tracked this down to fmtlib/fmt#3068 which was fixed in fmtlib/fmt#3072, dated 2022-09-03. The first release of fmt to include this fix is 10.0.0. I note that

# ArcticDB is currently incompatible with fmt 10
- fmt < 10

indicates we need to stick with fmt < 10.

Apparently gcc >= 11 doesn't need the fmt fix which is presumably why building using conda works OK (I have gcc 12.3.0 this way) whereas in the docker image I have gcc 10.2.1.

I suppose the easiest way to avoid these problems is to remove the #include <fmt/std.h> and instead locally implement a formatter for std::thread::id.

@jjerphan
Copy link
Collaborator

We might be able to use fmt 10.

@ianthomas23
Copy link
Collaborator Author

We might be able to use fmt 10.

I will take a look.

@jjerphan jjerphan mentioned this pull request Mar 15, 2024
5 tasks
@ianthomas23 ianthomas23 force-pushed the ianthomas23/remove_folly_thread_id branch from f64936f to 7059777 Compare March 15, 2024 12:43
Copy link
Collaborator

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM given 🟢 CI checks.

Thank you for helping with #1412, @ianthomas23.

@jjerphan jjerphan merged commit 62c322f into master Mar 15, 2024
111 checks passed
@jjerphan jjerphan deleted the ianthomas23/remove_folly_thread_id branch March 15, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants