Skip to content

lib: expose performance interface to global - #32790

Closed
juanarbol wants to merge 1 commit into
nodejs:masterfrom
juanarbol:juanarbol/expose-performance-global
Closed

lib: expose performance interface to global#32790
juanarbol wants to merge 1 commit into
nodejs:masterfrom
juanarbol:juanarbol/expose-performance-global

Conversation

@juanarbol

@juanarboljuanarbol commented Apr 12, 2020

Copy link
Copy Markdown
Member

May fix: #28635

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@addaleaxaddaleax added semver-major PRs that contain breaking changes and should be released in the next major version. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Apr 12, 2020
devsnek
devsnek approved these changes Apr 27, 2020
View reviewed changes
Comment threadlib/internal/bootstrap/node.js
exposeInterface(global, 'URLSearchParams', URLSearchParams);

const { performance } = require('perf_hooks');
// https://www.w3.org/TR/performance-timeline/

@devsnekdevsnekApr 27, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// https://www.w3.org/TR/performance-timeline/
// https://www.w3.org/TR/performance-timeline/
// https://www.w3.org/TR/hr-time-2/

@joyeecheungjoyeecheungMay 1, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We also implement https://www.w3.org/TR/user-timing/

@joyeecheung

joyeecheung commented May 1, 2020•
edited
Loading

Copy link
Copy Markdown
Member

quoting my comments in #28635 (comment)

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.

@jasnell

jasnell commented May 1, 2020

Copy link
Copy Markdown
Member

+1 to what @joyeecheung said.

targos
targos requested changes May 7, 2020
View reviewed changes

@targostargos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should also expose PerformanceObserver

targos added a commit to targos/node that referenced this pull request May 7, 2020
@targos
test: add hr-time Web platform tests
880aab0
Refs: nodejs#32790
@targostargos mentioned this pull request May 7, 2020
Closed
@targos

targos commented May 7, 2020

Copy link
Copy Markdown
Member

I'm looking at the spec for PerformanceObserver.observe and our implementation is missing a few things to be compliant.
I could work on improving it but I'd like to ask @jasnell if that's something we want (maybe we are not spec-compliant for a reason that I don't know about)?

@jasnell

jasnell commented May 7, 2020

Copy link
Copy Markdown
Member

That depends on the specific pieces you're seeing but the short version is: we're intentionally not fully compliant because the specs written includes a built-in memory leak that we are avoiding.

@targos

targos commented May 7, 2020

Copy link
Copy Markdown
Member

I don't know what you are referring to, but the first thing I noted is that we support the entryTypes option but not the type one.

@jasnell

jasnell commented May 7, 2020

Copy link
Copy Markdown
Member

I could be mistaken but type I believe, was added after the implementation was done. Either way it would be good to add that one

targos added a commit to targos/node that referenced this pull request May 7, 2020
@targos
test: add performance-timeline Web platform tests
c2ebe75
Refs: nodejs#32790
targos added a commit that referenced this pull request May 10, 2020
@targos
test: add hr-time Web platform tests
c252f6c
Refs: #32790
PR-URL: #33287
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
codebytere pushed a commit that referenced this pull request May 11, 2020
@targos@codebytere
test: add hr-time Web platform tests
1d2c81f
Refs: #32790
PR-URL: #33287
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@joyeecheungjoyeecheung mentioned this pull request May 12, 2020
Closed
4 tasks
@juanarbol

juanarbol commented May 13, 2020

Copy link
Copy Markdown
MemberAuthor

Performance API seems to need more love (hopefully I could help to stabilize this) and semver major cut was few weeks ago. I better close this PR.

@juanarbol

juanarbol commented May 13, 2020

Copy link
Copy Markdown
MemberAuthor

Oh, sorry, I see activity related to this.

@juanarboljuanarbol reopened this May 13, 2020
@BridgeAR
BridgeARforce-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72CompareMay 31, 2020 12:18
codebytere pushed a commit that referenced this pull request Jun 7, 2020
@targos@codebytere
test: add hr-time Web platform tests
cd92052
Refs: #32790
PR-URL: #33287
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@juanarbol

juanarbol commented Jul 21, 2020

Copy link
Copy Markdown
MemberAuthor

Closing due inactivity

@juanarbol
juanarbol deleted the juanarbol/expose-performance-global branch January 19, 2021 16:25
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheungjoyeecheungjoyeecheung left review comments
@targostargostargos requested changes
@addaleaxaddaleaxaddaleax approved these changes
+1 more reviewer
@devsnekdevsnekdevsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

perf_hooksIssues and PRs related to the implementation of the Performance Timing API.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

perf_hooks: expose performance global

6 participants

@juanarbol@joyeecheung@jasnell@targos@addaleax@devsnek