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

perf_hooks: expose performance global #28635

Closed
silverwind opened this issue Jul 11, 2019 · 16 comments
Closed

perf_hooks: expose performance global #28635

silverwind opened this issue Jul 11, 2019 · 16 comments
Labels
experimental Issues and PRs related to experimental features. feature request Issues that request new features to be added to Node.js. perf_hooks Issues and PRs related to the implementation of the Performance Timing API.

Comments

@silverwind
Copy link
Contributor

silverwind commented Jul 11, 2019

Browsers export a performance global, and I think we should too if the perf_hooks module is stable enough (and can be lifted from experimental stability) to allow easy usage in isomorphic scripts.

@silverwind silverwind added feature request Issues that request new features to be added to Node.js. experimental Issues and PRs related to experimental features. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Jul 11, 2019
@juanarbol
Copy link
Member

I'm quite confused, it would be something like performance.perfMember, where perfMember is provided by an abstraction layer of perf_hook module?

@Trott
Copy link
Member

Trott commented Jul 22, 2019

I'm quite confused, it would be something like performance.perfMember, where perfMember is provided by an abstraction layer of perf_hook module?

I think the suggestion is that perf_hooks.performance should be exposed as global.performance.

@juanarbol
Copy link
Member

In that case won't be difference between using require like: const { performance } = require('perf_hooks'), and just using global such as global.performance? A package that behaves like a module and is part of global, feels weird to me (IMO)

@Trott
Copy link
Member

Trott commented Jul 22, 2019

In that case won't be difference between using require like: const { performance } = require('perf_hooks'), and just using global such as global.performance? A package that behaves like a module and is part of global, feels weird to me (IMO)

console.log(require('timers').setTimeout === global.setTimeout); // true

@juanarbol
Copy link
Member

can I work on this?

@silverwind
Copy link
Contributor Author

@jasnell any objections here?

@juanarbol
Copy link
Member

Can I?

@Trott
Copy link
Member

Trott commented Aug 31, 2019

Can I?

Yes. Not that you need my permission or authority or anything, but if it makes you feel OK about forging ahead: Go for it. No guarantees that it will be accepted or that someone else isn't already working on it (although they probably aren't), but if this is something you'd like to see in Node.js core, you should totally put together a pull request for it.

@juanarbol
Copy link
Member

juanarbol commented Sep 4, 2019

Done at #29432 using perf_hooks module

@jasnell
Copy link
Member

jasnell commented Sep 4, 2019

Yikes, I just spotted this. There's a reason we don't expose it as a global... Specifically, doing so is semver-major and the current implementation is not fully compatible with the browser version.

@joyeecheung
Copy link
Member

IIRC, the convention is exposing a Web API to global when it is no longer experimental. I think at least we should take a look into the criteria for moving performance out of experimental status first.

@legendecas
Copy link
Member

Is there a tracking list of what should have been done to move perf_hooks module out of experimental?

@jasnell
Copy link
Member

jasnell commented Dec 26, 2019

I think it's time that we could. The module has been stable for a while now. There's really no checklist necessary in this case. It has tests, it's being used, so I think we could just open a PR making it stable. I still do not believe we should make the performance object a global, however -- for the same reason I give above.

@gireeshpunathil
Copy link
Member

fwiw, here is one or more criteria that can be reasonably checked against, for experimental exit:

  • API interfaces (functions / CLI) are evolved
  • user land modules are built on top
  • evidence of usage in production
  • test coverage in CI
  • No / containable flaky tests

@joyeecheung
Copy link
Member

joyeecheung commented May 1, 2020

I think it would be good to at least import https://github.com/web-platform-tests/wpt/tree/master/performance-timeline and figure out the difference between the Web before moving this to global, considering people could very well use the same code as how they would use it on the Web and run into differences (which could either be bugs or just wontfix). It would also be good to figure out the subset of performance timeline we want to implement and what we explicitly won't.

joyeecheung added a commit that referenced this issue May 19, 2020
- Add clarifications for Node.js-only extensions
- Explain the Web Performance APIs implemented in Node.js and
  clarify that perf_hooks also include other non-Web APIs.
- Prefix exposed interfaces with `perf_hooks.` to distinguish
  them from internal classes.

PR-URL: #33199
Refs: #28635
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this issue May 21, 2020
- Add clarifications for Node.js-only extensions
- Explain the Web Performance APIs implemented in Node.js and
  clarify that perf_hooks also include other non-Web APIs.
- Prefix exposed interfaces with `perf_hooks.` to distinguish
  them from internal classes.

PR-URL: #33199
Refs: #28635
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this issue Jun 7, 2020
- Add clarifications for Node.js-only extensions
- Explain the Web Performance APIs implemented in Node.js and
  clarify that perf_hooks also include other non-Web APIs.
- Prefix exposed interfaces with `perf_hooks.` to distinguish
  them from internal classes.

PR-URL: #33199
Refs: #28635
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
codebytere pushed a commit that referenced this issue Jun 9, 2020
- Add clarifications for Node.js-only extensions
- Explain the Web Performance APIs implemented in Node.js and
  clarify that perf_hooks also include other non-Web APIs.
- Prefix exposed interfaces with `perf_hooks.` to distinguish
  them from internal classes.

PR-URL: #33199
Refs: #28635
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@silverwind
Copy link
Contributor Author

Fixed by #37970.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Issues and PRs related to experimental features. feature request Issues that request new features to be added to Node.js. 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.

7 participants