path: change posix.join to use array
#54331
Conversation
Change posix.join to use array.join instead of additional assignment.
nodejs-github-bot
added
needs-ci
| joined = arg; | ||
| else | ||
| joined += `/${arg}`; | ||
| path.push(arg); |
There was a problem hiding this comment.
Does this need a primordial?
There was a problem hiding this comment.
If I use ArrayPrototypePush, the benchmark result is as follows.
confidence improvement accuracy (*) (**) (***)
path/join-posix.js n=100000 paths='/foo|bar||baz/asdf|quux|..' 0.20 % ±0.63% ±0.84% ±1.09%
There was a problem hiding this comment.
I think the primordial is needed here, regardless of benchmark results.
CC @nodejs/primordials
There was a problem hiding this comment.
Using primordials are not required. If we see a critical performance impact, just like this, we should avoid using it.
There was a problem hiding this comment.
How are they not required? This isn't the error path.
What about an alternative:
| path.push(arg); | |
| path[path.length - 1] = arg; |
There was a problem hiding this comment.
primordials are used elsewhere in this function, shouldn't they be used here, or removed from the other spots?
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1601/ Results: |
anonrig
added
author ready
github-actions
bot
removed
the
request-ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54331 +/- ##
==========================================
- Coverage 87.09% 87.09% -0.01%
==========================================
Files 647 647
Lines 181849 181849
Branches 34891 34885 -6
==========================================
- Hits 158380 158375 -5
- Misses 16766 16779 +13
+ Partials 6703 6695 -8
|
anonrig
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 6051826 |
Change posix.join to use array.join instead of additional assignment. PR-URL: nodejs#54331 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Change path.join to use array dynamic allocation instead of push. Refs: nodejs#54331
Change posix.join to use array.join instead of additional assignment. PR-URL: #54331 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Change posix.join to use array.join instead of additional assignment. PR-URL: #54331 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Change
posix.jointo usearray.joininstead of additional assignment.