Add time profiler for various constexpr evaluation events
so that slow event could be visible on the visualized flame chart.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
We can use this file as an example: ConstProfiler.cpp https://reviews.llvm.org/P8296
The code does a really big constant evaluation.
This is the generated json before the patch: before.json https://reviews.llvm.org/P8294
Giving this json to https://www.speedscope.app/, we get a big empty gap where the constant evaluation is being done.
link to screenshot
This is the generated json after the patch: after.json https://reviews.llvm.org/P8295
The empty gap is filled with four events.
link to screenshot
You can use this command for generating the json:
clang++ ConstProfiler.cpp -std=c++20 -c -ftime-trace -ftime-trace-granularity=50
P.S. I'm not sure how to write tests to cover the patch, but I'll find this out =)
I like the idea of this — +1 for the direction. I'd like to see some tests through before I approve.
Thanks for working on this!
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
15289 | what about Expr::EvaluateASRvalue and Expr::isPotentialConstantExpression? |
Thank you for working on this, having more detailed timing information for where we're spending time in the frontend will really help us to evaluate where we need to work on frontend performance.
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
15289 | Agreed; I think we should cover all the Expr::Evaluate* functions as well as related potentially expensive constant expression functions like Expr::isPotentialConstantExpression and Expr::isIntegerConstantExpr. | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
1740–1746 ↗ | (On Diff #468028) | Huh, I'm a bit surprised that the checking in this function isn't dominated by the time spent in Expr::isPotentialConstantExpr() -- if you add the time tracing to all of the constant expression evaluation functions, do you still need a time trace here in Sema? |
clang/lib/Sema/SemaExpr.cpp | ||
17544 ↗ | (On Diff #468028) | Why not push this down into EvaluateAsConstantExpr()? (Then the name you give the scope will also be more accurate.) |
Thanks for the greenlight!
I added a test that compiles a chunk of code and then checks the time trace graph in a human-readable form.
clang/lib/Sema/SemaDeclCXX.cpp | ||
---|---|---|
1740–1746 ↗ | (On Diff #468028) | Thanks! Removed trace here, added to Expr::isPotentialConstantExpr(). This method is called inside of CheckConstexprFunctionDefinition and really takes almost all the time. |
clang/lib/Sema/SemaExpr.cpp | ||
17544 ↗ | (On Diff #468028) | Thanks! I wrote on this level because I didn't manage to get SemaRef.getSourceManager() without SemaRef (we pass down SemaRef.getASTContext()). Anyway I found out that we can use SemaRef.getASTContext().getSourceManager() =) |
The test doesn't cover exactly all new traces though. For example I couldn't write a code that runs into the EvaluateAsInt method 🤔
If you have an idea for some funky constexpr code that can be tested, please write here =)
clang/unittests/Support/TimeProfilerTest.cpp | ||
---|---|---|
198–199 | This bit worries me because I suspect we'll get pretty wide variation between test bots in the build lab. Do you have an idea of how likely it is that this test will have different behavior depending on the machine? |
clang/unittests/Support/TimeProfilerTest.cpp | ||
---|---|---|
198–199 | I'd say, the test's outcome varies if behaviour of CompilerInstance varies. From my (imperfect) understanding of Clang/LLVM's architecture, its interface is pretty hermetic and we won't get inconsistent behaviour depending on the current machine as soon as we ran Compiler.ExecuteAction(Action);. If we look on other tests, there are tests that seemingly take the machine's target triple https://github.com/llvm/llvm-project/blob/3fee9358baab54e4ed646a106297e7fb6f1b4cff/clang/unittests/CodeGen/TestCompiler.h#L40-L48 to fill some info in CompilerInstance. But this is not the case in our test. Seems like the AST parsing is machine-independent 😃 |
LGTM! Please be sure to add a release note to alert folks to the time profiler becoming more awesome. :-)
clang/unittests/Support/TimeProfilerTest.cpp | ||
---|---|---|
198–199 | Excellent, thank you for the explanation! |
clang/unittests/Support/TimeProfilerTest.cpp | ||
---|---|---|
12 | Why is this in clang/unittests/Support (a new binary to boot)? This doesn't use any code form clang/lib/Support as far as I can tell. |
clang/unittests/Support/TimeProfilerTest.cpp | ||
---|---|---|
12 | It uses code from llvm/Support. There are already tests with similar names: clang/unittest/CodeGen and llvm/unittests/CodeGen, both are pretty close in spirit. So I decided to make clang/unittests/Support as a counterpart to llvm/unittests/Support. There is a time profiler test too - https://github.com/llvm/llvm-project/blob/main/llvm/unittests/Support/TimeProfilerTest.cpp |
Hi @Izaron, several of our internal tests that run tests using -ftime-trace started crashing when run which I bisected back to your change. I have filed issue #58551 for it, can you take a look?
Thanks for this!
When I experimented with this for #42754 I also came across functions like
Expr::isConstantInitializer
ExprConstant.cpp HandleFunctionCall/HandleConstructorCall
ByteCodeEmitter::compileFunc
Worth checking out if these are already covered or potentially useful.
what about Expr::EvaluateASRvalue and Expr::isPotentialConstantExpression?