-
Notifications
You must be signed in to change notification settings - Fork 500
Use type tagging for OpaqueWrappable type checks #5781
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
base: main
Are you sure you want to change the base?
Conversation
|
Hmm. Playing with godbolt I think there's a simpler version of this optimization: When So if we simply declare the two I think this approach is preferable as it is easier to understand and doesn't require storing extra tags. |
|
Oh, nifty. Yeah, that's better. Updated to take that approach! |
d85a30d to
2fb8553
Compare
0a8896b to
b2fc14d
Compare
2de389f to
3ef05b5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
For better perf optimization of dynamic_cast<T*>
3ef05b5 to
1a2b9c2
Compare
|
Switched the PR back to using the type tag approach. Since we build with thin-LTO and visibility=hidden flags, and since benchmarks and some tests are compiled as shared libraries rather than static, we end up hitting a known llvm bug (llvm/llvm-project#71196) when attempting to use the approach @kentonv suggests in #5781 (comment). The original type tag approach works as an alternative. |
Avoids using dynamic_cast/RTTI by using unique type tags for each type T. Perf gains here are expected to be negligible in typical benchmarks as there are other bottlenecks drowning it out... however, it's still a good micro-optimization / low-hanging-fruit update that carries very little risk.