Skip to content

Use performance.now() instead of Date.now()... #3483

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bitifet
Copy link
Contributor

@bitifet bitifet commented Jun 5, 2025

Replaced Date.now() for performance.now() (wherever applicable) to improve timing accuracy.

Date.now() is prone to system clock shifts, while performance.now() offers a high-resolution, monotonic clock for consistent, reliable measurements.

Not sure, but it could be involved in timeout issues we experience in VM environment but, anyway, this is the best way to measure time.

Added faliback code to support node prior to 16.0.0 (where perf_hook were not globally exposed by default) and even prior to 8.5.0 (defaulting to Date.now()).

Verified tests passes with node 22.12.0.

Attempted to pass with node prior to 16.0.0 but faced issues due to picked dependencies versions and other code of node_postgres that I guess it haven't been tested with so old node versions.

(I attempted to support at least from 8.0.0 which is declared in engines.node of packages/pg/package.json)

References:

https://nodejs.org/docs/latest-v8.x/api/perf_hooks.html#perf_hooks_performance_now

https://w3c.github.io/hr-time/

bitifet added 3 commits June 6, 2025 00:30
  * Wherever applicable (measuring performance, not time).
  * Failback to support node < 16.0.0 (perf_hook not globally exposed)
  * Failback to Date.now() for node < 8.5.0

  ✔ Tests passed with node > 16.0.0 (22.12.0)
  ✕ Couldn't pass with node prior 16.0.0 but not due to this changes.

https://nodejs.org/docs/latest-v8.x/api/perf_hooks.html#perf_hooks_performance_now
https://w3c.github.io/hr-time/
@charmander
Copy link
Collaborator

Not sure, but it could be involved in timeout issues we experience in VM environment

This pull request currently only changes benchmarking code that isn’t used by library consumers, so it definitely won’t fix those issues.

@bitifet
Copy link
Contributor Author

bitifet commented Jun 6, 2025

Not sure, but it could be involved in timeout issues we experience in VM environment

This pull request currently only changes benchmarking code that isn’t used by library consumers, so it definitely won’t fix those issues.

You're absolutely right.

I shouldn't have even mentioned that. It's just that was the reason why I got checking that.

I only grepped and checked that the changes wouldn't break expected behaviour.

As you said, it doesn't fixes anything, at least from the consumers point of view. But, anyway, I still think that it is better than before: At the end, those benchmarks are there for a reason and the more accurate they could be the better.

Anyway, if you feel it's better to reject the PR, don't worry and go ahead.

Regards.

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.

2 participants