perf: use direct array access instead of endsWIth

[GalacticHypernova]

🔗 Linked issue

❓ Type of change

• 📖 Documentation (updates to the documentation, readme, or JSdoc annotations)
• 🐞 Bug fix (a non-breaking change that fixes an issue)
• 👌 Enhancement (improving an existing functionality like performance)
• ✨ New feature (a non-breaking change that adds functionality)
• 🧹 Chore (updates to the build process or auxiliary tools and libraries)
• ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

endsWith has a big performance problem when it comes to single or double character checking. We can convert it to index access for better performance.

📝 Checklist

• I have linked an issue or discussion.
• I have updated the documentation accordingly.

[pi0]

Thanks for the PR but I would appreciate sharing a benchmark diff.

[GalacticHypernova]

@pi0 sure thing!
Here's the benchmark diff (taken from the same PR that I made to Nuxt):

And in case you want to copy the benchmark to test for yourself, here it is: nuxt/nuxt#24746 (comment)
I'm not entirely sure why this was closed but it can be safely reopened =)

[pi0]

Script above is not related to unstoage.

I understand that using index access vs endsWith might lead to faster benchmarks but either we need to ban using a javascript feature entirely or only avoid it in critical paths that proofs meaningful performance improvement and for this reason and explicitly mark those lines, for that we need benchmark script first 🙏🏼

[GalacticHypernova]

either we need to ban using a javascript feature entirely or only avoid it in critical paths that proofs meaningful performance improvement

I mean, I get your point, but I'd have to politely disagree with you on this one. It's not about banning a javascript feature.

There are many builtin methods that are there because they have legitimate use cases, but it doesn't necessarily mean that every possible use case is an intended use case.

I'm sure you've heard about Array.prototype.reduce, and the entire section MDN has on it where they show examples of when not to use it. It doesn't mean reduce should be banned altogether, it just means it should be used wherever its actually necessary. And in my opinion this is true for a lot of methods, even endsWith.

As with any abstraction, it has its performance considerations, and there are simply cases when using it is not optimal. You might argue that abstractions make the code more readable, but in this scenario I doubt anyone would say that having a simple if (a[b] === c) check is unreadable enough to warrant endsWith, and if it doesn't have performance benefits nor readability benefits, I don't see a reason to use it.

It's up to you whether to include this or not, but in such a critical component in the unjs ecosystem, in a part that is required to be as performant as possible, especially with certain drivers, I don't really see a reason not to include this, but I'll leave the decision up to you.

[pi0]

we need benchmark script first

[GalacticHypernova]

we need benchmark script first

Do you mean like an actual benchmark test with some benchmarking library? If we're talking about simply showing that it's indeed changing something I have provided a benchmark sample above. EndsWith doesn't discriminate between whether the code is for nuxt or unstorage, the slowdown happens all the time and can easily be observed based on the results that I have also attached, as that's how it was designed. However, if you'd like a full fledged benchmark, then we should already add benchmarks to everything, as nothing is currently benchmarked

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.57%. Comparing base (4d61c78) to head (561b6b3).
Report is 47 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
+ Coverage   65.05%   66.57%   +1.52%     
==========================================
  Files          39       39              
  Lines        4055     4105      +50     
  Branches      487      511      +24     
==========================================
+ Hits         2638     2733      +95     
+ Misses       1408     1363      -45     
  Partials        9        9              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pi0]

Added bench scripts partially specific to this change via d84bcc6

You can try with pnpm bench and see impl in test/server.bench.ts file.

---

Main:

storage server     310 µs/iter     (204 µs … 2'470 µs)    337 µs  1'002 µs  1'758 µs

This branch:

storage server     309 µs/iter     (192 µs … 1'769 µs)    334 µs  1'005 µs  1'666 µs

As you can see, the end-to-end difference is almost nothing. Not that it does not matter but hope you understand why micro-optimizations like this (even though, at micro-level are visible) do not essentially improve final performances that much but instead make code less consistent and less readable. (#386 perf improvement was not micro optimization to compare)

---

I again appreciate your contribution but ask you kindly next times respect maintainer's decision and comments and try to collaborate. Working on the bench took over an hour of my early morning that desperately needed for other stuff.

[GalacticHypernova]

I appreciate the merge!
I had no problem making the benchmark tests myself, I didn't mean you need to make it, I was simply asking whether it would be good idea to add benchmarks to the project so that I could proceed with it without you having to spend your time on it too much.

I know these are micro optimizations, I never said it would be anything ground breaking, but micro optimizations will eventually add up, especially in performance-critical environments and in I/O bound work.

I don't recall ever refusing to collaborate, I did leave the final decision to you, and I believe I have stated that, and I quote, "It's up to you whether to include this or not" and "I'll leave the decision up to you". I believe there was some miscommunication on the matter, which I would be more than willing to discuss in private if you're willing to.