-
-
Notifications
You must be signed in to change notification settings - Fork 576
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
perf: lazily load gazelle manifest files #2746
base: main
Are you sure you want to change the base?
perf: lazily load gazelle manifest files #2746
Conversation
The CI failure here seems unrelated, it's failed to clone the repo on the agent. |
3a57101
to
225f9cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any issues with this. I'd like to test it on our monorepo before approving, but I predict it'll be just fine.
For your case, what sort of speedups are you seeing? And how did you profile it (so that I can try to reproduce it)?
we currently have ~250 Python manifests,
Do you mean ~250 gazelle_python.yaml
files? Or 250 packages listed in a single gazelle_python.yaml
?
Sure, this has been running internally for us for ~2 months now, but we are still on the (now very diverged) original copy of the extension. Let me know how it goes.
It's slightly tricky to say, as it depends on the directory that Gazelle is running over, but we saw a reduction of about 30 seconds when running over only web targets, where no Python manifests are required. If you always run Gazelle over Python directories, then this isn't going to change anything. Gazelle can be profiled by via the
250 individual |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
250 individual
gazelle_python.yaml
files
Holy crap! I'd be interested to learn why you have all those. But anyway, I finally got around to testing things. It all looks good on my end.
Approved, ideally with the nit resolved. 🎉
@@ -76,6 +76,7 @@ Unreleased changes template. | |||
* (pypi) The PyPI extension will no longer write the lock file entries as the | |||
extension has been marked reproducible. | |||
Fixes [#2434](https://github.com/bazel-contrib/rules_python/issues/2434). | |||
* (gazelle) Lazily load and parse manifest files when running Gazelle. This ensures no manifest files are loaded when Gazelle is run over a set of non-python directories. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please add a link to the issue (if any) or to this PR for easy future reference.
In large repositories where Python may not be the only language, the gazelle manifest loading is done unnecessarily, and is done during the configuration walk.
This means that even for non-python gazelle invocations (eg
bazel run gazelle -- web/
), Python manifest files are being parsed and loaded into memory.This issue compounds if the repository uses multiple dependency closures, ie multiple
gazelle_python.yaml
files.In our repo, we currently have ~250 Python manifests, so loading them when Gazelle is only running over other languages is time consuming.