node-api: use container swap in DrainFinalizerQueue to reduce erase overhead

[mertcanaltin]

use container swap in DrainFinalizerQueue to reduce erase overhead

[nodejs-github-bot]

Review requested:

• @nodejs/node-api

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.13%. Comparing base (6102159) to head (4fe09e9).
Report is 51 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57861      +/-   ##
==========================================
- Coverage   90.15%   90.13%   -0.02%     
==========================================
  Files         630      629       -1     
  Lines      186756   186629     -127     
  Branches    36653    36623      -30     
==========================================
- Hits       168369   168227     -142     
- Misses      11189    11199      +10     
- Partials     7198     7203       +5     

Files with missing lines	Coverage Δ
src/node_api.cc	76.19% <100.00%> (+0.02%)	⬆️

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[legendecas]

The assumption in the comment "userland code can delete additional references in one finalizer" will break with the new change.

However, this assumption was not reflected in test test/js-native-api/6_object_wrap/6_object_wrap.cc. Would you mind updating the test to verify deleting another reference in the destructor of MyObject?

MyObject::~MyObject() {
  napi_delete_reference(env_, nested_);
  napi_delete_reference(env_, wrapper_);
}

[mertcanaltin]

The assumption in the comment "userland code can delete additional references in one finalizer" will break with the new change.

However, this assumption was not reflected in test test/js-native-api/6_object_wrap/6_object_wrap.cc. Would you mind updating the test to verify deleting another reference in the destructor of MyObject?

MyObject::~MyObject() {
  napi_delete_reference(env_, nested_);
  napi_delete_reference(env_, wrapper_);
}

I updated, thanks

[mertcanaltin]

tests passed in my local environment for macos but I think there is a problem in linux I will try

[legendecas]

#57861 (review) was not addressed.

The change proposed does not fulfill the behavior as described in the original comment:

node/src/node_api.cc

Lines 116 to 118 in 68cc1c9

	// As userland code can delete additional references in one finalizer,
	// the list of pending finalizers may be mutated as we execute them, so
	// we keep iterating it until it is empty.

This change will crash the test #57981.

[mertcanaltin]

#57861 (review) was not addressed.

The change proposed does not fulfill the behavior as described in the original comment:

node/src/node_api.cc

Lines 116 to 118 in 68cc1c9

	// As userland code can delete additional references in one finalizer,
	// the list of pending finalizers may be mutated as we execute them, so
	// we keep iterating it until it is empty.

This change will crash the test #57981.

thanks I updated it as you indicated here
#57861 (comment)

[mertcanaltin]

Hello @vmoroz, @legendecas, @mhdawson

I’ve updated this PR to implement the per-item finalizer drain as discussed—switching from value-based erase(ref_tracker) to iterator-based erase(it) for true O(1) removal:

- pending_finalizers.erase(ref_tracker);
+ auto it = pending_finalizers.begin();
+ pending_finalizers.erase(it);

[legendecas]

FWIW, the existing code always erase the first item on the list. Though the new code LGTM as well.

[nodejs-github-bot]

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

[vmoroz]

Hello @vmoroz, @legendecas, @mhdawson

I’ve updated this PR to implement the per-item finalizer drain as discussed—switching from value-based erase(ref_tracker) to iterator-based erase(it) for true O(1) removal:

- pending_finalizers.erase(ref_tracker);
+ auto it = pending_finalizers.begin();
+ pending_finalizers.erase(it);

@mertcanaltin , I do not see any meaningful difference from the existing code besides the declaring of the unnecessary auto it variable. It is better to abandon the PR if it does not add any value.

[mertcanaltin]

Hello @vmoroz, @legendecas, @mhdawson
I’ve updated this PR to implement the per-item finalizer drain as discussed—switching from value-based erase(ref_tracker) to iterator-based erase(it) for true O(1) removal:

- pending_finalizers.erase(ref_tracker);
+ auto it = pending_finalizers.begin();
+ pending_finalizers.erase(it);

@mertcanaltin , I do not see any meaningful difference from the existing code besides the declaring of the unnecessary auto it variable. It is better to abandon the PR if it does not add any value.

thanks I am closing this pr as it does not contribute to the current code, thanks a lot for the information