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

PerformanceNodeTiming of perf_hooks #45958

Closed
theanarkh opened this issue Dec 23, 2022 · 4 comments
Closed

PerformanceNodeTiming of perf_hooks #45958

theanarkh opened this issue Dec 23, 2022 · 4 comments
Labels
perf_hooks Issues and PRs related to the implementation of the Performance Timing API.

Comments

@theanarkh
Copy link
Contributor

theanarkh commented Dec 23, 2022

Version

main branch

Platform

No response

Subsystem

perf_hooks

What steps will reproduce the bug?

const { performance } = require('perf_hooks');
console.log(performance.nodeTiming)

How often does it reproduce? Is there a required condition?

always.

What is the expected behavior?

v8Start and nodeStart should be greater than 0 ?

What do you see instead?

nodeStart: -75.49576783180237,
v8Start: -34.36598587036133,
bootstrapComplete: 39.0672550201416,
environment: 0,
loopStart: -1,
loopExit: -1,

Additional information

I am not sure if it is a bug. I think the issue is made by this PR. I just wonder if the user will be a little confused ?
cc @legendecas

@theanarkh theanarkh added the perf_hooks Issues and PRs related to the implementation of the Performance Timing API. label Dec 23, 2022
@legendecas
Copy link
Member

I can confirm the issue is introduced in #43781. The problem is that we are exposing timing entries that are earlier than creating environments. These entries are available in the worker threads too.

As these timing entries are relative to the performance.timeOrigin, we may document that these timing entries that are earlier than the creation of environments can be negative values. However, entries like performanceNodeTiming.loopExit are already documented and if the milestone is not reached yet, the value can be -1. This could be confusing too.

As an alternative, we may revert #43781.

@theanarkh
Copy link
Contributor Author

theanarkh commented Jan 11, 2023

If it is expected currently I think we can explain this in the documentation, or i can submit a PR.

@jasnell
Copy link
Member

jasnell commented Jan 13, 2023

oy, yeah, these should definitely not be negative values. I'd consider this a breaking bug. Either #43781 should be reverted or timeOrigin needs to be set sooner.

@legendecas
Copy link
Member

These entries are available in the worker threads too. To avoid observing negative values in the workers, the timeOrigin value in the workers should also be set to the per-process's one, which basically invalidates #43781. Sounds like we should revert #43781.

targos pushed a commit that referenced this issue Mar 13, 2023
PR-URL: #46588
Fixes: #45958
Refs: #43781
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
targos pushed a commit that referenced this issue Mar 14, 2023
PR-URL: #46588
Fixes: #45958
Refs: #43781
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf_hooks Issues and PRs related to the implementation of the Performance Timing API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants