Skip to content

[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

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Oct 27, 2023

As a ground work, move ASTGen exported C function declaration to include/swift/Bridiging.

Usage:

% 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

@rintaro rintaro force-pushed the swift-parse-test branch 5 times, most recently from 77ec52b to dc29c21 Compare October 27, 2023 23:49
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"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed a typo

Suggested change
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
```
@rintaro
Copy link
Member Author

rintaro commented Oct 30, 2023

@swift-ci Please smoke test

@rintaro rintaro marked this pull request as ready for review October 30, 2023 15:21
@rintaro rintaro requested review from bnbarham, ahoppen and hamishknight and removed request for xedin, slavapestov, artemcm and hborla October 30, 2023 15:21
Copy link
Contributor

@bnbarham bnbarham left a 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?

@bnbarham
Copy link
Contributor

include/swift/Bridiging

Such an awful word to type. @hamishknight had a bunch of briding and briging in his PR too 😆.

This is no-op for SwiftParser. Also add a sanity check test file.
@rintaro
Copy link
Member Author

rintaro commented Oct 30, 2023

@swift-ci Please smoke test

Copy link
Member

@ahoppen ahoppen left a 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.

Comment on lines +49 to +50
llvm::cl::opt<unsigned int> Iteration = llvm::cl::opt<unsigned int>(
"n", llvm::cl::desc("iteration"), llvm::cl::init(1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

Suggested change
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
Copy link
Member

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?

Copy link
Contributor

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) {
Copy link
Member

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?

Comment on lines +186 to +187
<< llvm::format("throughput (byte/s): %8d\n", byteTPS)
<< llvm::format("throughput (line/s): %8d\n", lineTPS);
Copy link
Member

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)
Copy link
Member

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).

@rintaro rintaro merged commit 22592d2 into swiftlang:main Oct 30, 2023
@rintaro
Copy link
Member Author

rintaro commented Oct 30, 2023

Ooh, sorry @ahoppen I didn't see your comment before merging. I will address your comments in follow-ups

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants