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

performance: cache realpathSync access in package-cache.get #1608

Merged
merged 1 commit into from Sep 26, 2023

Conversation

raycohen
Copy link
Contributor

With a large, interconnected app (~40 engines, ~100 addons), this gets hit a lot with the same relatively small set of roots. Caching it saves as much as 6 minutes for us.

@ef4
Copy link
Contributor

ef4 commented Sep 22, 2023

Thanks.

This is probably good and we can land it. But I suspect that this spot being so hot may be the symptom of a different performance problem, and fixing that would yield an even bigger win.

I've had reports that suggest that the PackageCache is getting cache misses when we wouldn't expect it to. If you'd like to check for that in your app, see how many times Package is constructed for the same root. We'd expect a small number (maybe two per OS-level process). The constructor is here.

@raycohen
Copy link
Contributor Author

raycohen commented Sep 22, 2023

That Package constructor is getting hit 4201 times for us, but I don't see any cases where there are duplicates - each root looks like it gets one Package.

Oh, however as the build continues it gets hit with more roots, these ones are rewritten-packages/ with fingerprinted names. But I think that is after the speed savings this caching introduced for us

@ef4
Copy link
Contributor

ef4 commented Sep 22, 2023

I'm not quite sure what the CI failures are here, a quick attempt didn't reproduce them locally.

@raycohen
Copy link
Contributor Author

raycohen commented Sep 22, 2023

To provide more color on this particular slowness, when we hit these realpathSync calls we're inside the recursive findActiveAddons logic inside partitionEngines:
Screenshot 2023-09-22 at 1 26 17 PM

Is it possible that it is safe to cache some of this work further up? We have lots of engines that have lots of addon dependencies in common, so I think effectively there's a lot of redundant work happening. And it just happens that the realpath use is the main expensive thing in that work.

@ef4
Copy link
Contributor

ef4 commented Sep 24, 2023

Can you please test #1609 in your app as well to see what the build performance impact is?

I am hoping that PR makes a significant improvement, and also it might mean we don't need the new cache that's in this PR.

@raycohen
Copy link
Contributor Author

@ef4 #1609 seems to make a massive improvement to how much time our build spends in webpack, but I'm not sure if it cuts the number of hits I'm seeing to realpath, it still looks to be in the millions.

@ef4 ef4 marked this pull request as ready for review September 26, 2023 15:37
@ef4 ef4 merged commit eba2166 into embroider-build:main Sep 26, 2023
196 checks passed
@mansona mansona changed the title cache realpathSync access in package-cache.get performance: cache realpathSync access in package-cache.get Sep 29, 2023
@mansona mansona added the enhancement New feature or request label Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants