Skip to content

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-botnodejs-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
@himself65himself65 marked this pull request as ready for review April 24, 2022 23:52
@himself65himself65force-pushed the 20220423-fix-perfomance-timerify branch from 4762978 to 123fa0cCompareApril 24, 2022 23:53
@himself65himself65 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
@himself65himself65 requested a review from jasnellApril 25, 2022 14:55
@himself65
Copy link
MemberAuthor

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:

constperf_hooks=require('perf_hooks')functionf(){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

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@himself65himself65 changed the title perf_hooks: return different timerified function when different histogram perf_hooks: return different timerified function in timerifyApr 28, 2022
@himself65himself65 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022
@github-actionsgithub-actionsbot 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

@himself65himself65force-pushed the 20220423-fix-perfomance-timerify branch from 46444e2 to f87aed8CompareApril 28, 2022 18:51
@himself65
Copy link
MemberAuthor

quashed

@himself65himself65force-pushed the 20220423-fix-perfomance-timerify branch from f87aed8 to f9bc9eeCompareApril 28, 2022 18:59
@himself65himself65force-pushed the 20220423-fix-perfomance-timerify branch from f9bc9ee to 81bbb09CompareApril 29, 2022 22:33
@himself65himself65 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-actionsgithub-actionsbot 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

@himself65himself65 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

@himself65himself65 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
MemberAuthor

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

@nodejs-github-botnodejs-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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Co-authored-by: HE Shi-Jun <[email protected]>
@himself65himself65 changed the title perf_hooks: return different timerified function in timerifyperf_hooks: return different functions in timerifyMay 1, 2022
@himself65
Copy link
MemberAuthor

Landed in 3d0fc13

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

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Co-authored-by: HE Shi-Jun <[email protected]>
@targostargos 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Co-authored-by: HE Shi-Jun <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Fixes: #42742 PR-URL: #42854 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Co-authored-by: HE Shi-Jun <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
Fixes: #42742 PR-URL: #42854 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Co-authored-by: HE Shi-Jun <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Fixes: #42742 PR-URL: #42854 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Co-authored-by: HE Shi-Jun <[email protected]>
@targostargos mentioned this pull request Aug 3, 2022
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Co-authored-by: HE Shi-Jun <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.perf_hooksIssues 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

@himself65@nodejs-github-bot@mcollina@hax@jasnell@targos