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: return different functions in timerify #42854

Closed

Conversation

himself65
Copy link
Member

Fixes: #42742

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 24, 2022

Review requested:

  • @nodejs/performance

@nodejs-github-bot 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 himself65 marked this pull request as ready for review April 24, 2022 23:52
@himself65 himself65 force-pushed the 20220423-fix-perfomance-timerify branch from 4762978 to 123fa0c Compare April 24, 2022 23:53
lib/internal/histogram.js Outdated Show resolved Hide resolved
@himself65 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 himself65 requested a review from jasnell April 25, 2022 14:55
lib/internal/histogram.js Outdated Show resolved Hide resolved
@himself65
Copy link
Member Author

himself65 commented Apr 27, 2022

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

  1. 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)
  1. 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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@himself65 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 himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022
@nodejs-github-bot
Copy link
Collaborator

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

quashed

@himself65 himself65 force-pushed the 20220423-fix-perfomance-timerify branch from f87aed8 to f9bc9ee Compare April 28, 2022 18:59
@himself65 himself65 force-pushed the 20220423-fix-perfomance-timerify branch from f9bc9ee to 81bbb09 Compare April 29, 2022 22:33
@himself65 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2022
@nodejs-github-bot
Copy link
Collaborator

@himself65 himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2022
@nodejs-github-bot
Copy link
Collaborator

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 himself65 added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels May 1, 2022
@himself65
Copy link
Member Author

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

@nodejs-github-bot 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
@nodejs-github-bot
Copy link
Collaborator

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

himself65 added a commit that referenced this pull request May 1, 2022
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 himself65 changed the title perf_hooks: return different timerified function in timerify perf_hooks: return different functions in timerify May 1, 2022
@himself65
Copy link
Member Author

Landed in 3d0fc13

@himself65 himself65 closed this May 1, 2022
@himself65 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 himself65 deleted the 20220423-fix-perfomance-timerify branch May 1, 2022 19:51
@himself65
Copy link
Member Author

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

targos pushed a commit that referenced this pull request May 2, 2022
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>
@targos targos mentioned this pull request May 2, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
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>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
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>
targos pushed a commit that referenced this pull request Jul 12, 2022
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>
targos pushed a commit that referenced this pull request Jul 31, 2022
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>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. perf_hooks Issues and PRs related to the implementation of the Performance Timing API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

performance.timerify(fn, options) always return the same timerifed function
6 participants