Skip to content

Conversation

@jasnell
Copy link
Collaborator

@jasnell jasnell commented Dec 27, 2025

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.

@kentonv
Copy link
Member

kentonv commented Dec 27, 2025

Hmm. Playing with godbolt I think there's a simpler version of this optimization:

When T is declared final, the compiler can optimize dynamic_cast<T*> to simply check if the vtable is equal to T's vtable -- very similar to the optimization you've implemented here, but without the need to store an additional pointer.

So if we simply declare the two OpaqueWrappable implementations as final then I think that's all we need. (Note that OpaqueWrappable<T, true> will have to be modified to not inherit OpaqueWrappable<T, false>, but instead copy the contents, but that's fine.)

I think this approach is preferable as it is easier to understand and doesn't require storing extra tags.

@jasnell
Copy link
Collaborator Author

jasnell commented Dec 27, 2025

Oh, nifty. Yeah, that's better. Updated to take that approach!

@jasnell jasnell force-pushed the jasnell/jsg-promise-opaque-wrappable-type-tag branch from d85a30d to 2fb8553 Compare December 27, 2025 17:01
@jasnell jasnell force-pushed the jasnell/jsg-promise-opaque-wrappable-type-tag branch from 0a8896b to b2fc14d Compare December 27, 2025 20:48
@jasnell jasnell force-pushed the jasnell/jsg-promise-opaque-wrappable-type-tag branch 2 times, most recently from 2de389f to 3ef05b5 Compare December 30, 2025 00:46
@codspeed-hq

This comment was marked as outdated.

For better perf optimization of dynamic_cast<T*>
@jasnell jasnell force-pushed the jasnell/jsg-promise-opaque-wrappable-type-tag branch from 3ef05b5 to 1a2b9c2 Compare December 30, 2025 14:49
@jasnell
Copy link
Collaborator Author

jasnell commented Dec 30, 2025

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.

@jasnell jasnell requested review from anonrig and kentonv December 30, 2025 16:46
Sign up for free to 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.

3 participants