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

Utility/Debug: use thread_local for global state #66

Closed
wants to merge 3 commits into from

Conversation

Projects
3 participants
@xqms
Copy link
Contributor

commented Jun 14, 2019

This allows using Debug/Warning/Error redirection in multi-threaded situations.

I roughly measured the performance impact on my system: 45ns (new) vs 37ns (old) for printing a single character (Debug{} << "c"), redirected into std::stringstream.
Benchmark code using https://github.com/david-grs/geiger is here: https://gist.github.com/xqms/d29764bcdeb0d0b49d5a4fd39497a4ec.

In my opinion the additional cost is worth the gain - especially since Utility::Debug is probably not used in perfomance-critical paths.

xqms added some commits Jun 14, 2019

Utility/Debug: use thread_local for global state
This allows using Debug/Warning/Error redirection in background threads.
Performance impact on my system: 45ns (new) vs 37ns (old) for printing
a single character (Debug{} << "c"), redirected into std::stringstream.
Utility/Debug: move global state into implementation to make MSVC happy
Apparently we cannot have thread_local and DLL visibility specifiers
together.
@codecov-io

This comment has been minimized.

Copy link

commented Jun 17, 2019

Codecov Report

Merging #66 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #66   +/-   ##
=======================================
  Coverage   97.61%   97.61%           
=======================================
  Files          80       80           
  Lines        5455     5455           
=======================================
  Hits         5325     5325           
  Misses        130      130
Impacted Files Coverage Δ
src/Corrade/Utility/Debug.h 100% <ø> (ø) ⬆️
src/Corrade/Utility/Debug.cpp 96.75% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ae9031...c763d6a. Read the comment docs.

@xqms

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Urgh, it seems Xcode 7.3 does not support thread_local. I guess you want to keep supporting it?
I'll look for a workaround (probably some ugly map+mutex+this_thread::get_id() thing)...

@xqms xqms force-pushed the xqms:thread_local_debug branch 3 times, most recently from 9754f95 to 01a3177 Jun 17, 2019

@mosra

This comment has been minimized.

Copy link
Owner

commented Jun 17, 2019

Hi, thanks, this is definitely something to have in, yes :)

There's __thread_local for Apple's Clang, it's used in GL::Context as well. Anything but std::map :D Thinking more about it, it probably makes sense to move the MAGNUM_BUILD_MULTITHREADED option into Corrade so people who really don't want the additional runtime or code size overhead (Emscripten apps might come to mind) can turn it off. I can do that during merge.

@mosra mosra added this to TODO in Utility via automation Jun 17, 2019

@mosra mosra added this to the 2019.0b milestone Jun 17, 2019

@xqms xqms force-pushed the xqms:thread_local_debug branch from 01a3177 to c763d6a Jun 17, 2019

@xqms

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

Let's try this version with __thread. I don't have an Apple toolchain here, so I'm (ab)using the CI to check if things compile :-)

I kept the #if __has_feature(cxx_thread_local) since Apple supports thread_local since Xcode 8.0.

@mosra

This comment has been minimized.

Copy link
Owner

commented Jun 20, 2019

Merged as cb0fe13 and 2244c61, thank you! On top I added the ability to opt-out (using BUILD_MULTITHREADED) for people that don't want the overhead (9749f55).

I didn't have a chance to measure file size differences on Emscripten yet, in case the additional thread_local turns out to make a difference, I'll make it off by default there. Next step is also adapting Magnum to use this setting instead of its own independent one.

@mosra mosra closed this Jun 20, 2019

Utility automation moved this from TODO to Done Jun 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.