napi: reduce gc finalization stress by mhdawson · Pull Request #27085 · nodejs/node (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation13 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
#24494 fixed a crash
but resulted in increased stress on gc finalization. A leak
was reported in #26667 which
we are still investigating. As part of this investigation, I
realized we can optimize to reduce the amount of deferred finalization.
Regardless of the root cause of the leak this should be a
good optimization. It also resolves the leak for the case being
reported in #26667. The OP in #26667 has confirmed that he can
still recreate the original problem that #24494 fixed and that
the fix still works with this optimization
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes- commit message follows commit guidelines
Issues and PRs that require attention from people who are familiar with C++.
label
s/finaliation/finalization/ in commit message
@mscdex thanks for catching that, fixed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in commit log: s/gcc/gc/
nodejs#24494 fixed a crash but resulted in increased stress on gc finalization. A leak was reported in nodejs#26667 which we are still investigating. As part of this investigation I realized we can optimize to reduce amount of deferred finalization. Regardless of the root cause of the leak this should be a good optimization. It also resolves the leak for the case being reported in nodejs#26667. The OP in 26667 has confirmed that he can still recreate the original problem that 24494 fixed and that the fix still works with this optimization
napi: reduce gcc finaliation stress napi: reduce gc finaliation stress
napi: reduce gc finaliation stress napi: reduce gc finalization stress
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
danbev pushed a commit that referenced this pull request
#24494 fixed a crash but resulted in increased stress on gc finalization. A leak was reported in #26667 which we are still investigating. As part of this investigation I realized we can optimize to reduce amount of deferred finalization. Regardless of the root cause of the leak this should be a good optimization. It also resolves the leak for the case being reported in #26667. The OP in 26667 has confirmed that he can still recreate the original problem that 24494 fixed and that the fix still works with this optimization
PR-URL: #27085 Reviewed-By: Ben Noordhuis info@bnoordhuis.nl Reviewed-By: Colin Ihrig cjihrig@gmail.com Reviewed-By: James M Snell jasnell@gmail.com
This was referenced
Apr 23, 2019
This was referenced
Apr 23, 2019
@danbev could this please be backported to node10?
@danbev could this please be backported to node10?
Sorry I don't have time to open a backport PR against 10 this week, but I'll try to do so next week unless someone beats me to it.
Labels
Issues and PRs related to native addons.
PRs that have at least one approval, no pending requests for changes, and a CI started.
Issues and PRs that require attention from people who are familiar with C++.
Issues and PRs related to the Node-API.