-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
benchmark: update iterations of assert/deepequal-typedarrays.js #51419
benchmark: update iterations of assert/deepequal-typedarrays.js #51419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The low number was intentional. First of: a micro benchmark should not take too long and increasing the iterations by a factor of 50 also increases the runtime accordingly.
That aside: it is also beneficial to know if a difference can be identified without the full optimizations of V8 are in place as it's more likely for real code to behave like that.
After applying this PR, the testing duration remains almost the same so there was little overhead for the testing duration and runtime. The real reason for this PR is that the old iteration value does not trigger "equal" logic code enought. I pasted the time consumption of top funtions captured by perf-tools as below. It is clear that before this PR, the top 10 functions are around JS file reading. After increasing the iterations to a proper value (the specific value was carefully tested out) the "equal" logic was triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed a very valid point. Thank you for pointing that out! Do we have to increase it by a factor of 50x to reach that or would that also be hit by a lower number?
Yes, the factor was tested out case by case. I also modifed other cases and the factor varies across different cases. The method was to find out the least value to let case making sense and not have notable overhead for running duration. |
Hi @BridgeAR, this PR is not merged yet, and I submitted many other PRs like this but all of them are hanging up with no reviewers for more than 6 weeks. I'm not sure if I need to assign a reviewer or invite you as the reviewer? Thanks! |
Landed in e8de0f5 |
Fixes: #50571 PR-URL: #51419 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Fixes: #50571 PR-URL: #51419 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Fixes: #50571 PR-URL: #51419 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Fixes: #50571 PR-URL: #51419 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Fixes: nodejs#50571 PR-URL: nodejs#51419 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Fixes: #50571
Before applying this PR, the top functions are reading test JS file, instead of real logic code of "equal".
<style> </style>After increasing the iteration value, the test case behaved as expected to trigger "equal" code.