-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
Slowdown from the final
keyword
#90685
Comments
It is not clear to me if this is a bug on quick read through the blog. If you could reduce this and put in a godbolt example that would really help. Then we could more easily look at the AST and IR and see if anything obvious pops up. This is not my area, CC @erichkeane |
Without a good minimal example (and there isn't any actionable code on that blog post), I can't really see any reason why this would be the case. It isn't particularly reproducible unfortunately. As far as I know/can tell, the only difference from the CFE's perspective is that we do a de-virtualization in the case of a 'final' class: https://godbolt.org/z/8q9qM91o5 See how the use of Even under opt(https://godbolt.org/z/xqefEE1cj) that is effectively the difference you see. @nickdesaulniers is the one who commented on the original HN article, but he's away apparently, so unknown if he can comment. IF we can get some sort of reproducer, it would be nice to see some LLVM folks around to analyze. MY only (CFE only-knowledge though) guesses are: 2- This hit some sort of pathological case, where the inlining of some now-not-virtual functions resulted in LLVM optimizing it in a way that is non-optimal. So without some better reproducer, I don't really have an idea, nor even if we did, do I have the expertise here to help. |
According to https://news.ycombinator.com/item?id=40156196 and https://gitlab.com/define-private-public/PSRayTracing/-/issues/85, it's related to uniform_real_distribution / logl not being inlined |
It might be #19916 |
I'm a little confused, is the actually that the use of |
Is it possible that We've seen a variety of performance issues in Chromium due to overly-aggressive inlining (leading to me asking our compiler team to look at tuning inlining heuristics if they can), but I don't know how plausible that is here. |
I think it can be the case once they are turned into direct calls, indeed llvm-project/clang/lib/AST/DeclCXX.cpp Lines 2337 to 2339 in d86cc73
Out of curiosity, do you have some IR at hand (from Chromium) of overly-aggressive inlining that showed up to be detrimental for performance? |
I don't have any offhand, but Chromium installs a custom clang plugin to force authors to move class constructors/destructors out-of-line for any non-tiny class, because of this problem; this is irritating in C++20 because then people can't use aggregates nearly as often. We are also looking at some binary size changes due to different inlining as part of https://issues.chromium.org/issues/40255003; see comments 15 and 16 for some LLVM IR that changed due to using |
Yeah, looks like we're not folding calls to |
Hi. I published this blog post last week Monday about the
final
keyword. I made a note of a slowdown that was found when using clang/LLVM to compile and run the code.On Hacker News discussion an LLVM developer commented:
I plan on writing a follow up post shortly since the discussion surrounding everything seemed to be a bit more than I anticipated; and I feel a lot of people got the intent of the article wrong.
The text was updated successfully, but these errors were encountered: