Skip to content

fix: upgrade tinyglobby#121

Merged
robertsLando merged 1 commit into
yao-pkg:mainfrom
SuperchupuDev:fix/less-globbing
Feb 18, 2025
Merged

fix: upgrade tinyglobby#121
robertsLando merged 1 commit into
yao-pkg:mainfrom
SuperchupuDev:fix/less-globbing

Conversation

@SuperchupuDev
Copy link
Copy Markdown

@SuperchupuDev SuperchupuDev commented Nov 5, 2024

fixes #119

the problem with glob usage in this project is that globSync is called multiple times which isn't optimal, as it forces tinyglobby to traverse the filesystem multiple times. in theory, globSync could be called just once during the walking process, but that would be a bigger refactor, although a possible one

EDIT: this pr upgrades tinyglobby, ignore the text below this

it looks like under a default config most patterns passed to globSync aren't even globs, so this PR avoids unnecessary globSync calls when none of the patterns are globs

screenshot of patterns passed to tinyglobby in 6.1.0, each log is a different glob call

image

locally (windows) this change makes it faster than 5.15.0, with the reproduction from #119 taking 25s in 5.15.0 and 13s in latest with the change (latest without the change took too long to measure)

@SuperchupuDev
Copy link
Copy Markdown
Author

ci is failing, will debug tomorrow

@robertsLando
Copy link
Copy Markdown
Member

robertsLando commented Nov 5, 2024

Could you also test if there are improvements with the test-80-compression-node-opcua test? It actually takes 5 min on ubuntu to complete.

Seems now it went down to 2min: https://github.com/yao-pkg/pkg/actions/runs/11675697212/job/32510712730?pr=121#step:7:20 :) I think there is still range of improvement here. Could you try enabling that test also for windows and mac?

Just remove this lines:

// FIXME: this test takes a long time to run (from 5min on linux up to 10 minuntes on windows)
// run only on linux to save time on CI
if (process.platform !== 'linux') {
return;
}

@robertsLando robertsLando changed the title avoid globbing non-dynamic paths fix: avoid globbing non-dynamic paths Nov 6, 2024
@robertsLando
Copy link
Copy Markdown
Member

I also created #122, I would like to compare performances of tests in this two PR

@SuperchupuDev
Copy link
Copy Markdown
Author

from my local tests in the zip repro of #119 this should be faster than #122, since the unnecesary glob calls were still present back then

@robertsLando
Copy link
Copy Markdown
Member

In fact seems this is faster 👍🏼

Comment thread lib/walker.ts Outdated
@SuperchupuDev
Copy link
Copy Markdown
Author

okay i see why at least some tests fail, it's due to this project using directory expansion, which means that globbing dir is the same as globbing dir/**. dir itself is technically not dynamic so it gets skipped which shouldn't happen

@robertsLando
Copy link
Copy Markdown
Member

@SuperchupuDev Yep understood, could you fix that?

@SuperchupuDev
Copy link
Copy Markdown
Author

trying

@SuperchupuDev
Copy link
Copy Markdown
Author

SuperchupuDev commented Nov 6, 2024

okay, fixing it would just make all patterns dynamic, making performance not change whatsoever. we need a better solution. there needs to be a refactor in the walker logic so that all patterns are pushed into an array and then when all of them are collected call globSync once

@robertsLando
Copy link
Copy Markdown
Member

@SuperchupuDev Makes sense, agreee

@robertsLando
Copy link
Copy Markdown
Member

@SuperchupuDev News on this?

@SuperchupuDev
Copy link
Copy Markdown
Author

SuperchupuDev commented Nov 20, 2024

been busy with university, i have to debug and figure out why vite's tests fail with tinyglobby's upcoming version (which should fix the perf issue), could try today

@SuperchupuDev SuperchupuDev changed the title fix: avoid globbing non-dynamic paths fix: upgrade tinyglobby Nov 28, 2024
@SuperchupuDev SuperchupuDev marked this pull request as draft November 28, 2024 13:41
@SuperchupuDev SuperchupuDev force-pushed the fix/less-globbing branch 2 times, most recently from 20d13d8 to 8d47f55 Compare November 28, 2024 14:20
@SuperchupuDev
Copy link
Copy Markdown
Author

@robertsLando changed this pr to the upcoming tinyglobby version instead, can you compare performance again?

@SuperchupuDev
Copy link
Copy Markdown
Author

SuperchupuDev commented Nov 28, 2024

okay, thanks. fyi it's 4 minutes on fast-glob as well: https://github.com/yao-pkg/pkg/actions/runs/11703987586/job/32595478258?pr=122#step:7:20

@SuperchupuDev
Copy link
Copy Markdown
Author

SuperchupuDev commented Nov 28, 2024

although this pr should solve the perf issue reported in #119, for the record, if you want to avoid unnecesary globbing one solution can be setting expandDirectories to false in tinyglobby's options, then doing what this pr did before i force pushed. it'd be a breaking change though as users would have to replace some of their patterns i.e. src with src/**

@robertsLando
Copy link
Copy Markdown
Member

Ok thanks for that! I would not go for that option as I think there will be too much edge cases to handle and I'm quite sure it could cause some unexpected issues I don't want to deal with right now. We can merge this if it's ready 👍🏼

@robertsLando robertsLando marked this pull request as ready for review November 28, 2024 16:55
@robertsLando
Copy link
Copy Markdown
Member

I correct myself: we can merge this once you have a release for tinyglobby :)

@robertsLando
Copy link
Copy Markdown
Member

@SuperchupuDev News on this? Got a new release for tinyglobby?

@SuperchupuDev
Copy link
Copy Markdown
Author

SuperchupuDev commented Jan 15, 2025

i'm waiting for fdir's author to release a new version with a fix that got merged a few weeks ago. tinyglobby's upcoming version has a few breaks without the fix. should be out fairly soon after that happens

@robertsLando
Copy link
Copy Markdown
Member

@SuperchupuDev ok will wait so, thanks! :)

@SuperchupuDev
Copy link
Copy Markdown
Author

good news - the new fdir version was released today. i'll work on tinyglobby starting next week

@robertsLando
Copy link
Copy Markdown
Member

@SuperchupuDev Thanks for the update! :)

@SuperchupuDev
Copy link
Copy Markdown
Author

the new optimizer was completely rewritten for the second time and merged on tinyglobby's main branch. all that's left for a release is to fix a completely unrelated bug that many have been asking me to fix 👍 should be out really soon!!

@SuperchupuDev SuperchupuDev force-pushed the fix/less-globbing branch 2 times, most recently from 934cb59 to bf42a50 Compare February 14, 2025 23:31
@robertsLando
Copy link
Copy Markdown
Member

@SuperchupuDev Good news! Thanks for the update, let me know when the last fix is ready and I can merge this :)

@SuperchupuDev
Copy link
Copy Markdown
Author

@robertsLando the new release is out!! i've updated my pr. sorry it took so long :P

@robertsLando
Copy link
Copy Markdown
Member

@SuperchupuDev no need to be sorry man, it's OSS and I appreciate every help even if it take some time 🙏🏼 😄

Thanks!

@robertsLando robertsLando merged commit d2d38f9 into yao-pkg:main Feb 18, 2025
@SuperchupuDev SuperchupuDev deleted the fix/less-globbing branch February 18, 2025 07:42
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.

Slow packaging of pnpm workspaces project

2 participants