fs: accept all valid utf8 values in fast paths#64341
Conversation
Signed-off-by: Hamid Reza Ghavami <hamidr.ghavami@gmail.com>
bfaade2 to
1d3f916
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64341 +/- ##
==========================================
- Coverage 92.01% 90.24% -1.77%
==========================================
Files 379 741 +362
Lines 166972 241706 +74734
Branches 25554 45562 +20008
==========================================
+ Hits 153639 218128 +64489
- Misses 13041 15107 +2066
- Partials 292 8471 +8179
🚀 New features to boost your workflow:
|
|
I don't think we should land this. The more checks we have, the more performance impact it would create. Do you have any examples out there that is impacted by this? Does people really pass UTF8? If not, let's close this. |
|
Hi @anonrig thanks for the review! I completely understand the concern regarding the performance impact on the fast path. I picked this up to resolve issue #49888, as there seemed to be some inconsistency in how variations like That said, if the consensus is that the branch/check overhead for the 99% of users passing standard |
Signed-off-by: Hamid Reza Ghavami <hamidr.ghavami@gmail.com>
1d3f916 to
fccaa91
Compare
|
The commit message linting and sign-off trailer are fixed, and all CI checks are passing! Could someone from the @nodejs/fs team please take a look when you have a moment? Thanks! |
Fixes: #49888
This PR introduces a lightweight
isUtf8Encodinghelper to ensurefs.readFileSync,fs.writeFileSync, andencodeRealpathResultuse the fast C++ path for all valid variations of the utf8 encoding string (utf8,utf-8,UTF8,UTF-8).