X Tutup
The Wayback Machine - https://web.archive.org/web/20221004071333/https://github.com/nodejs/node/pull/44499
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

Jump to bottom

lib: fix reference leak #44499

Merged
merged 1 commit into from Sep 7, 2022
Merged

lib: fix reference leak #44499

merged 1 commit into from Sep 7, 2022
+68 −0

Conversation

falsandtru
Copy link
Contributor

@falsandtru falsandtru commented Sep 2, 2022 

                                                  confidence improvement accuracy (*)   (**)   (***)
process/next-tick-breadth-args.js n=10000000                    -0.88 %       ±2.88% ±3.84%  ±5.01%
process/next-tick-breadth.js n=10000000                          1.89 %       ±6.65% ±8.84% ±11.51%
process/next-tick-depth-args.js n=7000000                       -0.92 %       ±2.44% ±3.25%  ±4.23%
process/next-tick-depth.js n=7000000                     **     -3.93 %       ±2.92% ±3.88%  ±5.06%
process/next-tick-exec-args.js n=4000000                         0.97 %       ±2.80% ±3.73%  ±4.86%
process/next-tick-exec.js n=4000000                             -0.05 %       ±3.25% ±4.33%  ±5.64%
process/next-tick-loop-args.js loop=1000 n=10000        ***     20.59 %       ±2.88% ±3.83%  ±4.98%
process/next-tick-loop-args.js loop=1000 n=20000        ***     12.65 %       ±2.57% ±3.41%  ±4.45%
process/next-tick-loop-args.js loop=1000 n=40000        ***     19.94 %       ±2.52% ±3.36%  ±4.39%
process/next-tick-loop.js loop=1000 n=10000             ***      8.85 %       ±1.78% ±2.36%  ±3.08%
process/next-tick-loop.js loop=1000 n=20000             ***     14.25 %       ±1.53% ±2.04%  ±2.67%
process/next-tick-loop.js loop=1000 n=40000             ***     21.86 %       ±1.41% ±1.88%  ±2.44%

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Sep 2, 2022
@falsandtru falsandtru mentioned this pull request Sep 3, 2022
Closed
@tniessen
Copy link
Member

tniessen commented Sep 3, 2022

Benchmark CI might take a few hours.

@falsandtru
Copy link
Contributor Author

falsandtru commented Sep 3, 2022

Looks good.

22:10:47                                                  confidence improvement accuracy (*)    (**)   (***)
22:10:48 process/next-tick-breadth-args.js n=10000000                     0.90 %       ±1.72%  ±2.27%  ±2.92%
22:10:48 process/next-tick-breadth.js n=10000000                         -3.21 %       ±4.62%  ±6.09%  ±7.82%
22:10:48 process/next-tick-depth-args.js n=7000000                       -0.13 %       ±1.35%  ±1.79%  ±2.30%
22:10:48 process/next-tick-depth.js n=7000000                             0.84 %       ±1.89%  ±2.49%  ±3.19%
22:10:48 process/next-tick-exec-args.js n=4000000                        -7.79 %       ±7.98% ±10.53% ±13.52%
22:10:48 process/next-tick-exec.js n=4000000                              7.32 %      ±10.58% ±13.96% ±17.93%
22:10:48 process/next-tick-loop-args.js loop=1000 n=10000        ***     14.96 %       ±2.06%  ±2.72%  ±3.49%
22:10:48 process/next-tick-loop-args.js loop=1000 n=20000        ***     18.53 %       ±2.69%  ±3.55%  ±4.56%
22:10:48 process/next-tick-loop-args.js loop=1000 n=40000        ***     31.59 %       ±2.47%  ±3.26%  ±4.19%
22:10:48 process/next-tick-loop.js loop=1000 n=10000             ***      8.72 %       ±2.00%  ±2.64%  ±3.39%
22:10:48 process/next-tick-loop.js loop=1000 n=20000             ***     14.93 %       ±1.80%  ±2.38%  ±3.05%
22:10:48 process/next-tick-loop.js loop=1000 n=40000             ***     36.57 %       ±2.27%  ±3.00%  ±3.86%

@tniessen
Copy link
Member

tniessen commented Sep 3, 2022

Indeed :)

visualization of the results posted in the previous comment

@falsandtru
Copy link
Contributor Author

falsandtru commented Sep 4, 2022 •
edited

This fix also improves breadth performance over 10% if the confidence is ***. Probably loop performance is confident(stabilized) breadth and exec performance.

@falsandtru
Copy link
Contributor Author

falsandtru commented Sep 5, 2022

Can you approve?

@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2022
@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Sep 5, 2022

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

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2022
@tniessen tniessen added commit-queue Add this label to land a pull request using GitHub Actions. benchmark Issues and PRs related to the benchmark subsystem. labels Sep 7, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 7, 2022
@nodejs-github-bot nodejs-github-bot merged commit bae03c4 into nodejs:main Sep 7, 2022
54 checks passed
@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Sep 7, 2022

Landed in bae03c4

@falsandtru falsandtru deleted the queue branch Sep 7, 2022
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@falsandtru @Fyko
lib: fix reference leak
7739d9c 
PR-URL: nodejs#44499
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@falsandtru @RafaelGSS
lib: fix reference leak
1e137c2 
PR-URL: #44499
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 26, 2022
Merged
RafaelGSS pushed a commit that referenced this pull request Sep 26, 2022
@falsandtru @RafaelGSS
lib: fix reference leak
2baf532 
PR-URL: #44499
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
juanarbol pushed a commit that referenced this pull request Oct 3, 2022
@falsandtru @juanarbol
lib: fix reference leak
0855d6a 
PR-URL: #44499
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
juanarbol pushed a commit that referenced this pull request Oct 4, 2022
@falsandtru @juanarbol
lib: fix reference leak
141eefc 
PR-URL: #44499
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@juanarbol juanarbol mentioned this pull request Oct 4, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen

@JungMinu JungMinu

@BridgeAR BridgeAR

@Himself65 Himself65

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@falsandtru @tniessen @nodejs-github-bot @JungMinu @BridgeAR @Himself65 @daeyeon
X Tutup