-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[swift-parse-test] A parser performance check utility #69470
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
Conversation
77ec52b
to
dc29c21
Compare
llvm::cl::opt<bool>("swift-parser", llvm::cl::desc("Use SwiftParser")); | ||
|
||
llvm::cl::opt<bool> LibParse = | ||
llvm::cl::opt<bool>("lib-parse", llvm::cl::desc("Use iibParse")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a typo
llvm::cl::opt<bool>("lib-parse", llvm::cl::desc("Use iibParse")); | |
llvm::cl::opt<bool>("lib-parse", llvm::cl::desc("Use libParse")); |
``` % swift-parse-test -swift-parser -lib-parse -n 20 ../swift-syntax/Sources files count: 228 total bytes: 5662399 total lines: 158428 iteration: 20 ---- parser: SwiftParser wall clock time (ms): 3848 cpu time (ms): 3843 throughput (byte/sec): 355824 throughput (line/sec): 41225 ---- parser: libParse wall clock time (ms): 296 cpu time (ms): 290 throughput (byte/sec): 4715281 throughput (line/sec): 546303 ```
dc29c21
to
e01df92
Compare
@swift-ci Please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a sanity check test case?
Such an awful word to type. @hamishknight had a bunch of |
This is no-op for SwiftParser. Also add a sanity check test file.
d05b4c6
to
66b8946
Compare
@swift-ci Please smoke test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few nitpicky naming comments.
llvm::cl::opt<unsigned int> Iteration = llvm::cl::opt<unsigned int>( | ||
"n", llvm::cl::desc("iteration"), llvm::cl::init(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
llvm::cl::opt<unsigned int> Iteration = llvm::cl::opt<unsigned int>( | |
"n", llvm::cl::desc("iteration"), llvm::cl::init(1)); | |
llvm::cl::opt<unsigned int> Iterations = llvm::cl::opt<unsigned int>( | |
"n", llvm::cl::desc("iteration"), llvm::cl::init(1)); |
buffer.getBufferStart(), buffer.getBufferSize(), /*moduleName=*/"", | ||
buffer.getBufferIdentifier().data(), /*ASTContext=*/nullptr); | ||
swift_ASTGen_destroySourceFile(sourceFile); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn’t really matter since it’s a test utility but should we error if we don’t have SWIFT_BUILD_SWIFT_SYNTAX
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do in swift_parse_test_main
, I also made this mistake when I first went through it. An alternative design would be to have these methods return an Error
, which would mean the SWIFT_BUILD_SWIFT_SYNTAX check in swift_parse_test_main
could be removed for something more generic.
|
||
/// Measure the duration of \p body execution. | ||
template <typename Duration> | ||
static std::pair<Duration, Duration> measure(llvm::function_ref<void()> body) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a doc comment what the first and what the second Duration
is?
<< llvm::format("throughput (byte/s): %8d\n", byteTPS) | ||
<< llvm::format("throughput (line/s): %8d\n", lineTPS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to specify that throughput is measured in CPU time, not wall time?
@@ -112,6 +112,7 @@ void Driver::parseDriverKind(ArrayRef<const char *> Args) { | |||
.Case("swift-api-extract", DriverKind::APIExtract) | |||
.Case("swift-api-digester", DriverKind::APIDigester) | |||
.Case("swift-cache-tool", DriverKind::CacheTool) | |||
.Case("swift-parse-test", DriverKind::ParseTest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should extend this tool to do more parser tests? If it should just stay a performance measuring tool, I would name it swift-parser-performance-test
.
Also, I just want to note that swift-syntax has swift-parser-test
(with an r
). I don’t know if it’s good that this is spelled without an r
(means we don’t have two utilities with the same name) or bad (inconsistency).
Ooh, sorry @ahoppen I didn't see your comment before merging. I will address your comments in follow-ups |
As a ground work, move
ASTGen
exported C function declaration toinclude/swift/Bridiging
.Usage: