Skip to content

Replacing all TLS variants with the C++11 thread_local#24247

Merged
veblush merged 2 commits intogrpc:masterfrom
veblush:tls
Oct 8, 2020
Merged

Replacing all TLS variants with the C++11 thread_local#24247
veblush merged 2 commits intogrpc:masterfrom
veblush:tls

Conversation

@veblush
Copy link
Copy Markdown
Contributor

@veblush veblush commented Sep 25, 2020

This PR is trying to replace all TLS variants (gcc, msvc, pthread) with the C++11 thread_local. This helps simplify all complicated TLS implementations. There was a previous attempt (#20413) but rejected because it required the XCode and iOS bump. Let's revisit this along with required platform upgrade (Apple #24282, Android #24283)

Unlike the original attempt, C++ TLS is implemented as one of TLS variants for incremental adoption. This begins with all major platforms such as Linux, Mac, Windows, and Mobile and will cover all other platforms.

Related Issues

@veblush veblush added the release notes: no Indicates if PR should not be in release notes label Sep 25, 2020
@veblush veblush force-pushed the tls branch 2 times, most recently from 9536538 to e74415d Compare September 25, 2020 21:11
@veblush veblush force-pushed the tls branch 2 times, most recently from a78f8c6 to d973774 Compare September 26, 2020 00:46
@veblush
Copy link
Copy Markdown
Contributor Author

veblush commented Sep 26, 2020

grpc_build_artifacts_multiplatform: passed

@jtattermusch
Copy link
Copy Markdown
Contributor

While I'm liking that we're finally working on fixing #13856, before we proceed I'd like to get a clarification on whether this will affect performance when C-core library is loaded dynamically? (e.g. some wrapped language do rely on using dlopen / dlsym to load C core).)
https://stackoverflow.com/questions/13106049/what-is-the-performance-penalty-of-c11-thread-local-variables-in-gcc-4-8

@jtattermusch
Copy link
Copy Markdown
Contributor

CC @jtattermusch

@veblush
Copy link
Copy Markdown
Contributor Author

veblush commented Oct 6, 2020

@jtattermusch That's good point. This article is good to understand why dynamic linking has a different performance for TLS in general. This applies for both __thread and thread_local. And we don't worry about the slow case mentioned in Q&A because we don't use extern and class-type. So I don't expect any performance regression because of this case at least for Linux.

@veblush
Copy link
Copy Markdown
Contributor Author

veblush commented Oct 7, 2020

grpc_build_artifacts_multiplatform: passed

@veblush veblush marked this pull request as ready for review October 7, 2020 23:14
Copy link
Copy Markdown
Member

@yashykt yashykt left a comment

Choose a reason for hiding this comment

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

just minor nits otherwise lgtm

@kalbasit
Copy link
Copy Markdown

kalbasit commented Nov 2, 2020

Was this reverted? Version 1.33.0 works for me but neither 1.33.1 nor 1.33.2 work for me.

@veblush
Copy link
Copy Markdown
Contributor Author

veblush commented Nov 3, 2020

@kalbasit It's still here and I don't think there were relevant changes between 1.33.0 and 1.33.1+.

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

Labels

kind/internal cleanup lang/core release notes: no Indicates if PR should not be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants