Skip to content

[swift-parse-test] Tweak for review #69550

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 4, 2023
Merged

[swift-parse-test] Tweak for review #69550

merged 1 commit into from
Nov 4, 2023

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented Oct 31, 2023 •
edited
Loading

Tweak fore the review on #69470

  • 'iteration' -> 'iterations'
  • Comment to clarify cpu time vs wall clock time
  • Return 'Error' from the executors

rintaro
rintaro commented Oct 31, 2023
View reviewed changes
lib/DriverTool/swift_parse_test_main.cpp Outdated
@@ -178,13 +194,16 @@ perform(const SmallVectorImpl<std::unique_ptr<llvm::MemoryBuffer>> &buffers,
auto cDisplay =
std::chrono::duration_cast<std::chrono::milliseconds>(cDuration).count();

// Throughputs are based on CPU time.
auto byteTPS = totalBytes * duration_t::period::den / cDuration.count();
auto lineTPS = totalLines * duration_t::period::den / cDuration.count();

llvm::outs() << llvm::format("wall clock time (ms): %8d\n", tDisplay)
<< llvm::format("cpu time (ms): %8d\n", cDisplay)
<< llvm::format("throughput (byte/s): %8d\n", byteTPS)
<< llvm::format("throughput (line/s): %8d\n", lineTPS);
Copy link
Member Author

@rintaro rintaro Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't change this (clarifying CPU time based) because I didn't want to make these headers long.

@rintaro
Copy link
Member Author

rintaro commented Oct 31, 2023

#69470 (comment)

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.

I am not certain at this point. Maybe we can use this as a ASTGen test utility to verify the ASTGen result, or testing SwiftIfConfig combined with CompilerInvocation, etc.

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

I don't really have strong opinion on this 😅

@rintaro rintaro requested a review from ahoppen October 31, 2023 21:08
ahoppen
ahoppen approved these changes Oct 31, 2023
View reviewed changes
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.

Thanks

@rintaro
Copy link
Member Author

rintaro commented Oct 31, 2023

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Nov 3, 2023

@swift-ci Please smoke test

@rintaro
[swift-parse-test] Tweak for review
cb5fd7d 
* 'iteration' -> 'iterations'
* Comment to clarify cpu time vs wall clock time
* Return 'Error' from the executors
@rintaro
Copy link
Member Author

rintaro commented Nov 3, 2023

@swift-ci Please smoke test

@rintaro
Copy link
Member Author

rintaro commented Nov 4, 2023

@swift-ci Please smoke test Windows

@rintaro rintaro merged commit c3af0ff into swiftlang:main Nov 4, 2023
finagolfin
finagolfin reviewed Nov 5, 2023
View reviewed changes
lib/DriverTool/swift_parse_test_main.cpp
return llvm::Error::success();
#else
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"SwiftParser is not supported")
Copy link
Member

Choose a reason for hiding this comment

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

@rintaro, this C++ syntax error broke builds that don't use SwiftSyntax:

/home/ubuntu/jenkins/workspace/oss-swift-RA-linux-ubuntu-16.04-android-arm64/swift/lib/DriverTool/swift_parse_test_main.cpp:111:67: error: expected ';' after return statement
                                   "SwiftParser is not supported")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@finagolfin finagolfin finagolfin left review comments

@ahoppen ahoppen ahoppen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rintaro @ahoppen @finagolfin