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

refactor: simplify crawlEndFinder #12868

Merged
merged 12 commits into from
Apr 17, 2023
Merged

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Apr 14, 2023

Description

Simplify crawlEndFinder, it seems we can avoid creating promises by using timeouts.
See #12851 for reference.

The idea is from a discussion with @dominikg. He actually proposed splitting delayDepsOptimizerUntil(id, done) into delayDepsOptimizerUntil(id) + markIdAsDone(id) and call markIdAsDone(id) without a promise during build (using the moduleParsed hook). I tried this but I couldn't make it work (I think it may work in case someone wants to take a look).

But this already is way simpler, we could measure perf too but I don't expect a difference.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Apr 14, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev marked this pull request as draft April 15, 2023 05:19
@patak-dev
Copy link
Member Author

We have a missing test to check if when running pnpm run build on playground/optimize-deps there aren't missing dependencies (an Internal error message will appear in this case). Weirdly, the tests would still pass if this happen, we need to review this.

I pushed 969ff9f (#12868) and de0323f (#12868) anyways so you can see how it would look. There are missing dependencies with these two commits though. I think it may be possible to work this out, but I don't know yet how.

In af366a2 (#12868), I added back the ensureFirstRun guard to make this PR easier to merge now as I think removing it requires more testing (we may be missing another test case too here).

So the PR after af366a2 (#12868) is only a simplication of crawlEndFinder that avoids the Promise.allSettled scheme. I think we can move forward with this one, and we can check in a future PR if we can rework where we do delayDepsOptimizerUntil during build. cc @dominikg you can check the commits above if you wanted to play, the tests will pass but make sure to pnpm run build on playground/optimize-deps

@patak-dev patak-dev marked this pull request as ready for review April 15, 2023 07:12
if (!workersSources.has(id)) {
registeredIds.add(id)
done()
.catch(() => {})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of this catch, which has been around for a long time in different forms, we may be swallowing some load errors during dev. If you remove it, you'll see Vitest complaining about unhandled errors. We need to review this in another PR (this PR doesn't change things but it makes more clear that we are doing it).

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Apr 15, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ❌ failure
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ❌ failure
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ❌ failure
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@patak-dev patak-dev requested a review from bluwy April 15, 2023 07:32
@patak-dev
Copy link
Member Author

patak-dev commented Apr 15, 2023

50ms (0.5%) improvement on fcp 1k components: https://github.com/vitejs/vite-benchmark/actions/runs/4706700049

Edit: ok, it is for the k-means, when checking the details, there is only a small improvement in mean and median. @fi3ework, maybe we should get back to using the mean as the main metric?

@patak-dev patak-dev changed the title refactor: simplify crawlEndFinder perf: simplify crawlEndFinder Apr 15, 2023
@patak-dev patak-dev added performance Performance related enhancement p4-important Violate documented behavior or significantly improves performance (priority) feat: deps optimizer Esbuild Dependencies Optimization labels Apr 15, 2023
@patak-dev patak-dev changed the title perf: simplify crawlEndFinder refactor: simplify crawlEndFinder Apr 15, 2023
@patak-dev patak-dev added p3-significant High priority enhancement (priority) and removed p4-important Violate documented behavior or significantly improves performance (priority) performance Performance related enhancement labels Apr 15, 2023
@fi3ework
Copy link
Member

@patak-dev I would prefer median more with a simple swipe off the extremely outlier which is easy to be recognized 👀. The downside of mean is it's still easy to be effected to some slight outliner with a vague definition of "extremely outlier" define. After several PR's results, do you also think median is good enough 🤔?

@patak-dev
Copy link
Member Author

I'm good with using median, and in that case, I don't think we need to remove the outliers, the median would already do itself by choosing the value in the middle. The effect or removing N outliers in both extremes should be a non op

@patak-dev
Copy link
Member Author

test: check no missing deps in build time adds a check that will fail with the reverted commits 969ff9f (#12868) and de0323f (#12868), and would also have failed (if the log would have been there) with 4.2.1.

@patak-dev
Copy link
Member Author

Merging so we have a few days to test this one while we are still in beta.

@patak-dev patak-dev merged commit 31f8b51 into main Apr 17, 2023
@patak-dev patak-dev deleted the refactor/simplify-crawl-end-finder branch April 17, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: deps optimizer Esbuild Dependencies Optimization p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants