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

benchmark: fix timer display in progress output #11235

Closed

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 8, 2017

This commit fixes an issue where the minutes would not display properly on the benchmark timer once an hour had elapsed.

/cc @AndreasMadsen @joyeecheung

Lint: https://ci.nodejs.org/job/node-test-linter/6921/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • benchmark

This commit fixes an issue where the minutes would not display
properly on the benchmark timer once an hour had elapsed.
@mscdex mscdex added the benchmark Issues and PRs related to the benchmark subsystem. label Feb 8, 2017
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Feb 8, 2017
@AndreasMadsen
Copy link
Member

What is the reason for the other changes, only minutes was calculated wrong?

@mscdex
Copy link
Contributor Author

mscdex commented Feb 8, 2017

@AndreasMadsen True, only minutes were calculated wrong, but because of that, seconds need to be re-calculated as well. Additionally, I could have used a temporary variable to make the adjustment once and base the minutes and seconds off of that, but I opted to just duplicate the calculations inline.

@AndreasMadsen
Copy link
Member

I think it is fine, but I'm pretty sure that x mod m^2 mod m = x mod m, so the seconds shouldn't have changed unless something javascript-mysterious is going on.

@joyeecheung
Copy link
Member

joyeecheung commented Feb 8, 2017

Hmm, sorry about not testing this earlier (come to think of it I could've just change the clock and observe), thanks for fixing this.

I think (time % 3600) % 60 is the same as time % 60 like @AndreasMadsen suggests above, but I think the former works fine too.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

I thought that had looked off but hadn't taken the time yet to investigate. Thanks for this

jasnell pushed a commit that referenced this pull request Feb 17, 2017
This commit fixes an issue where the minutes would not display
properly on the benchmark timer once an hour had elapsed.

PR-URL: #11235
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 17, 2017

landed in a08fcc0

@jasnell jasnell closed this Feb 17, 2017
@mscdex mscdex deleted the benchmark-fix-progress-timer-display branch February 17, 2017 00:56
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
This commit fixes an issue where the minutes would not display
properly on the benchmark timer once an hour had elapsed.

PR-URL: nodejs#11235
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
This commit fixes an issue where the minutes would not display
properly on the benchmark timer once an hour had elapsed.

PR-URL: #11235
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Andreas Madsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

Depends on previous landed PR that has not been backported to v6 or v4. Marking as don't land on those branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants