Skip to content

Measure config parse / file scan time, actual total time #1069

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 1 commit into from
Jun 5, 2025

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jun 5, 2025

In Strada, I can't tell if this time was omitted or just a part of the parse time (probably the latter). But we were totally omitting it in this repo, which hides the fact that config loading and file scanning takes a significant amount of time. Collect this number, and then ensure the total time actually includes everything.

See: #1062 (comment)

Config time:      0.120s
Parse time:       0.513s
Bind time:        0.100s
Check time:       5.719s
Emit time:        0.721s
Total time:       7.176s

This is going to make our total time stat longer, but it's more realistic, and we should totally go and optimize file scanning/matching to eliminate the use of regexp2.

As part of this, I've refactored System such that time fully goes through System, which I mistakenly did not do in my original PR moving this functionality into the execute pacakge.

Verified

This commit was signed with the committer’s verified signature. The key has expired.
andrewbranch Andrew Branch
…lly total
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the timing instrumentation by including the configuration file parse and file scan time in the overall compilation time measurement. Key changes include:

  • Measuring and propagating config parse time in execute/tsc.go.
  • Refactoring performCompilation to accept and assign the new configTime.
  • Updating the system interfaces and test scaffolds (testsys_test.go and cmd/tsgo/sys.go) to support accurate total time measurement.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/execute/tsc.go Added configTime measurement and adjusted totalTime computation
internal/execute/testsys_test.go Added start time initialization and SinceStart() in the test system
internal/execute/system.go Updated the System interface to include the SinceStart() method
internal/execute/outputs.go Added reporting of config time if non-zero
cmd/tsgo/sys.go Integrated a start time field and SinceStart() method for accurate timing
Comments suppressed due to low confidence (1)

internal/execute/tsc.go:197

  • [nitpick] Consider adding a comment clarifying that sys.SinceStart() reports the elapsed time from system initialization, ensuring that maintainers understand the starting point of the total time measurement.
result.totalTime = sys.SinceStart()

@jakebailey jakebailey enabled auto-merge June 5, 2025 18:54
@jakebailey jakebailey added this pull request to the merge queue Jun 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 5, 2025
@jakebailey jakebailey added this pull request to the merge queue Jun 5, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 5, 2025
@jakebailey jakebailey added this pull request to the merge queue Jun 5, 2025
Merged via the queue into main with commit 9ca67e4 Jun 5, 2025
23 of 63 checks passed
@jakebailey jakebailey deleted the jabaile/stat-time branch June 5, 2025 23:34
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.

None yet

3 participants