perf_hooks: return different functions in timerify by himself65 · Pull Request #42854 · nodejs/node

himself65 · 2022-04-24T22:39:46Z

Fixes: #42742

nodejs-github-bot · 2022-04-24T22:39:50Z

Review requested:

@nodejs/performance

lib/internal/histogram.js

deps/v8/src/logging/counters.h

lib/internal/histogram.js

himself65 · 2022-04-27T14:26:11Z

I'm thinking to remove the cache feature.
There're two main reasons:

Adding a private symbol field into the user's data is not a good idea. Because we don't know what kind of data will pass in

For example:

const perf_hooks = require('perf_hooks')

function f () {
  console.log('hello, world!')
}
Object.freeze(f)
perf_hooks.performance.timerify(f)

/Users/himself65/Code/node/out/Debug/node /Users/himself65/Code/test/index.js
node:internal/perf/timerify:123
    ObjectDefineProperties(fn, {
    ^

TypeError: Cannot define property Symbol(kTimerified), object is not extensible
    at defineProperties (<anonymous>)
    at Performance.timerify (node:internal/perf/timerify:123:5)

caching by default will increase unnecessary complexity.

The original PR(#37136) only has 1 parameter which is fn. At this time, cache a function is very simple. However, the next PR adds one more parameter histogram(#37475), and this causes unexpected code behavior inconsistent today.
So if we finished a perfect cache system for now and merge it. But what if we add one more parameter in the future?

Moreover, I think users could cache a timerify function if they want. So I will remove cache stuff in this PR

/cc @nodejs/performance

mcollina

lgtm

nodejs-github-bot · 2022-04-28T01:52:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43732/

test/parallel/test-performance-function.js

himself65 · 2022-04-28T18:51:41Z

quashed

nodejs-github-bot · 2022-04-30T03:17:00Z

CI: https://ci.nodejs.org/job/node-test-pull-request/43785/

nodejs-github-bot · 2022-05-01T19:21:48Z

Commit Queue failed

- Loading data for nodejs/node/pull/42854
✔  Done loading data for nodejs/node/pull/42854
----------------------------------- PR info ------------------------------------
Title      perf_hooks: return different timerified function in timerify (#42854)
Author     Himself65  (@Himself65)
Branch     Himself65:20220423-fix-perfomance-timerify -> nodejs:master
Labels     author ready, perf_hooks, needs-ci
Commits    1
 - perf_hooks: return different timerified function in timerify
Committers 1
 - Himself65 
PR-URL: https://github.com/nodejs/node/pull/42854
Fixes: https://github.com/nodejs/node/issues/42742
Reviewed-By: Matteo Collina 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42854
Fixes: https://github.com/nodejs/node/issues/42742
Reviewed-By: Matteo Collina 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - perf_hooks: return different timerified function in timerify
   ℹ  This PR was created on Sun, 24 Apr 2022 22:39:46 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/42854#pullrequestreview-955517509
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/42854#pullrequestreview-955519178
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-04-30T22:10:34Z: https://ci.nodejs.org/job/node-test-pull-request/43800/
- Querying data for job/node-test-pull-request/43800/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2254578403

himself65 · 2022-05-01T19:32:06Z

Let me think... How to use commit-queie🤔

nodejs-github-bot · 2022-05-01T19:37:05Z

Commit Queue failed

- Loading data for nodejs/node/pull/42854
✔  Done loading data for nodejs/node/pull/42854
----------------------------------- PR info ------------------------------------
Title      perf_hooks: return different timerified function in timerify (#42854)
Author     Himself65  (@Himself65)
Branch     Himself65:20220423-fix-perfomance-timerify -> nodejs:master
Labels     author ready, perf_hooks, needs-ci
Commits    1
 - perf_hooks: return different timerified function in timerify
Committers 1
 - Himself65 
PR-URL: https://github.com/nodejs/node/pull/42854
Fixes: https://github.com/nodejs/node/issues/42742
Reviewed-By: Matteo Collina 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42854
Fixes: https://github.com/nodejs/node/issues/42742
Reviewed-By: Matteo Collina 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - perf_hooks: return different timerified function in timerify
   ℹ  This PR was created on Sun, 24 Apr 2022 22:39:46 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/42854#pullrequestreview-955517509
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/42854#pullrequestreview-955519178
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-05-01T19:21:48Z: https://ci.nodejs.org/job/node-test-pull-request/43800/
- Querying data for job/node-test-pull-request/43800/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2254620326

Fixes: #42742 PR-URL: #42854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Co-authored-by: HE Shi-Jun <hax@heshijun.net>

himself65 · 2022-05-01T19:50:23Z

Landed in 3d0fc13

himself65 · 2022-05-01T20:00:18Z

What's more, I found that the commit queue always failed because the commit message tool didn't consider the Co-author-by field to the tailer.
So the final check failed

Fixes: #42742 PR-URL: #42854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Co-authored-by: HE Shi-Jun <hax@heshijun.net>

Fixes: nodejs/node#42742 PR-URL: nodejs/node#42854 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Co-authored-by: HE Shi-Jun <hax@heshijun.net>

nodejs-github-bot added needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Apr 24, 2022

himself65 marked this pull request as ready for review April 24, 2022 23:52

himself65 force-pushed the 20220423-fix-perfomance-timerify branch from 4762978 to 123fa0c Compare April 24, 2022 23:53

himself65 commented Apr 25, 2022

View reviewed changes

lib/internal/histogram.js Outdated Show resolved Hide resolved

targos reviewed Apr 25, 2022

View reviewed changes

deps/v8/src/logging/counters.h Outdated Show resolved Hide resolved

himself65 added perf_hooks Issues and PRs related to the implementation of the Performance Timing API. and removed v8 engine Issues and PRs related to the V8 dependency. labels Apr 25, 2022

himself65 requested a review from jasnell April 25, 2022 14:55

mcollina requested changes Apr 25, 2022

View reviewed changes

lib/internal/histogram.js Outdated Show resolved Hide resolved

mcollina approved these changes Apr 27, 2022

View reviewed changes

jasnell approved these changes Apr 27, 2022

View reviewed changes

himself65 changed the title ~~perf_hooks: return different timerified function when different histogram~~ perf_hooks: return different timerified function in timerify Apr 28, 2022

himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022

hax reviewed Apr 28, 2022

View reviewed changes

test/parallel/test-performance-function.js Outdated Show resolved Hide resolved

himself65 force-pushed the 20220423-fix-perfomance-timerify branch from 46444e2 to f87aed8 Compare April 28, 2022 18:51

himself65 force-pushed the 20220423-fix-perfomance-timerify branch from f87aed8 to f9bc9ee Compare April 28, 2022 18:59

github-actions bot mentioned this pull request Apr 29, 2022

CI Reliability 2022-04-29 nodejs/reliability#266

Open

21 tasks

himself65 force-pushed the 20220423-fix-perfomance-timerify branch from f9bc9ee to 81bbb09 Compare April 29, 2022 22:33

github-actions bot mentioned this pull request Apr 30, 2022

CI Reliability 2022-04-30 nodejs/reliability#267

Open

27 tasks

himself65 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Apr 30, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2022

himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2022

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 1, 2022

himself65 changed the title ~~perf_hooks: return different timerified function in timerify~~ perf_hooks: return different functions in timerify May 1, 2022

himself65 closed this May 1, 2022

himself65 removed needs-ci PRs that need a full CI run. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 1, 2022

himself65 deleted the 20220423-fix-perfomance-timerify branch May 1, 2022 19:51

github-actions bot mentioned this pull request May 2, 2022

CI Reliability 2022-05-02 nodejs/reliability#269

Open

30 tasks

targos mentioned this pull request May 2, 2022

v18.1.0 release proposal #42943

Merged

This was referenced May 3, 2022

CI Reliability 2022-05-03 nodejs/reliability#270

Open

CI Reliability 2022-05-04 nodejs/reliability#271

Open

CI Reliability 2022-05-05 nodejs/reliability#272

Open

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Uh oh!

Conversation

himself65 commented Apr 24, 2022

Uh oh!

nodejs-github-bot commented Apr 24, 2022 • edited by himself65 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

himself65 commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 28, 2022

Uh oh!

Uh oh!

himself65 commented Apr 28, 2022

Uh oh!

nodejs-github-bot commented Apr 30, 2022

Uh oh!

nodejs-github-bot commented May 1, 2022

Uh oh!

himself65 commented May 1, 2022

Uh oh!

nodejs-github-bot commented May 1, 2022

Uh oh!

himself65 commented May 1, 2022

Uh oh!

himself65 commented May 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nodejs-github-bot commented Apr 24, 2022 •

edited by himself65

Loading

himself65 commented Apr 27, 2022 •

edited

Loading