Skip to content
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

Overhaul options creation #88

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

Torathion
Copy link
Contributor

@Torathion Torathion commented Feb 16, 2025

Hi, me again.

After updating the performance of this project, I decided to reduce the package size even further and clean it further up by overhauling how fdir options are built with the given options and properties.

What did I do?

  • Tolerate redundant checks to reduce bundle size
  • Outsource types and fdir-related functions to their respective files for better project management
  • Potentially improved performance by using less object property accesses
  • Add forgotten performance improvement in getPartialMatcher
  • Added more type definitions for better readability

This reduced the build size from

CJS dist\index.js     11.94 KB
CJS dist\index.js.map 21.24 KB
CJS ⚡️ Build success in 17ms
ESM dist\index.mjs     9.96 KB
ESM dist\index.mjs.map 21.10 KB

to

ESM dist\index.mjs     9.58 KB
ESM dist\index.mjs.map 20.59 KB
ESM ⚡️ Build success in 16ms
CJS dist\index.js     11.56 KB
CJS dist\index.js.map 20.73 KB

which is around 400b.

It seems like biome doesn't support 32 bit windows (or there is any other bug), so I can't lint and format :(

Copy link

pkg-pr-new bot commented Feb 16, 2025

Open in Stackblitz

npm i https://pkg.pr.new/tinyglobby@88

commit: 8cf0612

@Torathion
Copy link
Contributor Author

Torathion commented Feb 16, 2025

Also I have a question: Are found paths both processed in filter and exclude? If yes, there could be the possibility to cache processed paths, which would significantly improve the performance.

@SuperchupuDev
Copy link
Owner

i haven't had the time to look at this pr, but answering your question: wouldn't work in most cases. fdir exclude paths are always absolute and filter paths aren't, plus the fact that you wouldn't even find directories in filter unless you set up the options to also receive dirs

@Torathion
Copy link
Contributor Author

Torathion commented Feb 16, 2025

Yeah, I tried caching anywhere and the tests started to fail. Damn...

@Torathion Torathion changed the title Overhaul fdir options creation Overhaul options creation Feb 19, 2025
@Torathion
Copy link
Contributor Author

Torathion commented Feb 19, 2025

I've realized the way tinyglobby manages it's own options was copy pasted between glob and globSync, so I tried to centralize the logic.

This also included some tiny restructurings and an additional type called Input to describe the first parameter of those two functions to increase readability. I had to make the GlobOptions type stricter and introduce a Partial<GlobOptions> for the options processing.

This change reduced the bundle size by another ~350 bytes.

ESM dist\index.mjs     9.23 KB
ESM dist\index.mjs.map 20.09 KB
ESM ⚡️ Build success in 18ms
CJS dist\index.js     11.19 KB
CJS dist\index.js.map 20.22 KB
CJS ⚡️ Build success in 19ms
DTS Build start
DTS ⚡️ Build success in 908ms
DTS dist\index.d.mts 1.01 KB
DTS dist\index.d.ts  1.01 KB

@Torathion
Copy link
Contributor Author

Torathion commented Feb 19, 2025

I found some more options handling logic in processPatterns that can be initially handled in getOptions. I also extended the InternalProps with cwd and expandDirs so we need to pass less arguments over to normalizePattern. Even though the build command says I've added 30 bytes in size again, it should still be smaller when minified.

Edit: After reverting the InternalProps extension and using GlobOptions instead, I managed to save 20 bytes more.

@Torathion
Copy link
Contributor Author

Torathion commented Feb 19, 2025

We are almost at 1KB reduced file size, basically almost 10% reduction. The only question is the behavior of options.followSymbolicLinks, options.onlyFiles and options.caseSensitiveMatch in the fdir options creation. They have an explicit comparison with false, implying a different behavior when it's undefined. This could be mitigated and save the bytes by omitting those explicit comparisons by defining a base option value for each of them, ensuring no undefined is passed through.

So this question is: Do false and undefined have a different behavior for these options?

@SuperchupuDev
Copy link
Owner

yes they do, because those options are meant to default to true

@Torathion
Copy link
Contributor Author

Okay, so the undefined can be prevented by initializing all of them as true when processing tinyglobby's options, got it. It's easier to read that way as they don't have a third hidden state causing bugs.

@SuperchupuDev
Copy link
Owner

okay! i thought initializing them to true was less performant, although that was just a guess and i have no actual evidence behind it

@Torathion
Copy link
Contributor Author

Torathion commented Feb 19, 2025

Well, there should always be a balance of package size and performance. If you care too much about package size, it's going to be slow, if you care too much about performance, it's going to be unreadable (because everything theoretically makes the program slower). I know one or two big projects that accidentally introduced this third buggy state of undefined and people kept building around it. For example, I wanted to debloat npm-check-updates and had to start over multiple times, because of it.

Defining all necessary default options makes the code look bigger, but is the most clean and easy to maintain way. You can define your options type as strict as possible, are able to process it centrally and use the input value with Partial before processing it. This helps you understand your options more and don't need to make sure over and over again that everything is correctly processed and bug-free.

Using this approach made this project almost 1KB smaller with this PR.

Speaking of performance: Since objects are dynamically sized and shaped, JS engines introduced the concept of a "shape" to optimize and organize the heap by implicitly defining two objects with the same properties as one fixed "shape" like two instances of a class. This improves performance as the engine becomes aware of what kind of object you want to use. TypeScript helps you utilize this concept better by defining interfaces for your objects or force you to define all properties of a class outside of the constructor to make the shape of an object static. So changing the values of properties is extremely fast, but adding and removing properties with undefined or delete is slow.

You can read more about it here, here and here

@Torathion
Copy link
Contributor Author

Torathion commented Feb 20, 2025

@SuperchupuDev Okay, I'm done and everything should be ready. If there is anything wrong with it, Let me know.

I'd be happy if this could be merged 🙏

Edit: Also I optimized a tiny thing from #91. Instead of splitting every time, we can just check input.startsWith('..'), because we are only looking at the first part.

@SuperchupuDev
Copy link
Owner

great! will check it out at some point. no ETA since i got a cold and there's no rush for a new release right now anyways

@Torathion
Copy link
Contributor Author

Oh no!! Get well soon and don't stress yourself! ♥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants