-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
fs: move rmSync implementation to c++
#53617
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
fs: move rmSync implementation to c++
#53617
Conversation
c541155 to
3d2975d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
3d2975d to
d820ac1
Compare
d820ac1 to
8d02638
Compare
8d02638 to
2538925
Compare
|
Benchmark results (since CI is locked down): |
Failed to start CI⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/9784060111 |
5c1f6ed to
8c8e277
Compare
|
cc @nodejs/cpp-reviewers can you review? |
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.
LGTM with green tests
|
Landed in 7168295 |
|
Marking it as dont-land because of #53962 (comment) |
|
@richardlau @aduh95 any idea why the tests are flaky? |
|
@anonrig the flakiness of the test (and its root cause) is beside the point, the fact that it break something is our CI means it likely going to break our users and therefore should not land on release lines. In an ideal world, when the CI reports the same test failing 6 times in the same PR but is passing in others, that PR would not land until the flakiness is addressed. |
Yes, I agree. I'm trying to understand how this change caused this regression. |
|
Is this safe to land on v22.x with cf2bce6 ? |
I need to write some benchmarks before making this pull-request ready.
Benchmark on all
fscommands: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1575/