Skip to content
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

7553 get elapsed time refactoring #7558

Merged

Conversation

ssonthal
Copy link
Contributor

@ssonthal ssonthal commented Oct 5, 2024

Resolves #7553

Changes

  • Replacing the logic of stopwatch.start() and stopwatch.stop() with dotnet suggested optimized approach of using startTime and GetElapsedTime method combination.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

  • Yes
  • No

@ssonthal ssonthal marked this pull request as ready for review October 5, 2024 15:49
Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Stopwatch.ElapsedMilliseconds was long but Stopwatch.GetElapsedTime(startTime).TotalMillisecond is double so I expect the formatting in strings/logs will be suboptimal, can you check?

@ssonthal
Copy link
Contributor Author

ssonthal commented Oct 6, 2024

Yes, you are correct. I missed that. Have updated the changes accordingly. Pls check now.

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Last thing: could you check if just adding N0 formatting like {X:N0} wouldn't be better?

@ssonthal
Copy link
Contributor Author

ssonthal commented Oct 7, 2024

Hey, I have modified the long formatting as suggested. Also, implemented the suggested changes in the code. Pls check now.

@ssonthal ssonthal requested a review from LukaszRozmej October 7, 2024 14:31
@LukaszRozmej LukaszRozmej merged commit dac7890 into NethermindEth:master Oct 7, 2024
67 checks passed
rjnrohit pushed a commit that referenced this pull request Oct 10, 2024
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.

Remove Stopwatch.StartNew() and new Stopwatch where makes sense
2 participants