-
-
Notifications
You must be signed in to change notification settings - Fork 640
refactor(core): get_zip_runfiles_path should call startswith less #3442
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(core): get_zip_runfiles_path should call startswith less #3442
Conversation
Looking at the investigation in bazel-contrib#3381, it seems that we are calling the startswith many times and I wanted to see if it would be possible to optimize how it is done. I also realized that no matter what target we have, we will be calling the function once with a `__init__.py` path and we can inline this case as a separate if statement checking for equality instead, which Starlark optimizer should understand better. Before this PR for every executable target we would go through the `legacy_external_runfiles and "__init__.py".startswith("external")` and this PR eliminates this. Related to bazel-contrib#3380 and bazel-contrib#3381
Summary of ChangesHello @aignas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a performance optimization by refactoring the path resolution logic within Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request refactors _get_zip_runfiles_path to optimize for the __init__.py case by avoiding unnecessary startswith checks. The logic is soundly split into a core path construction function and a legacy function that handles prefix stripping, which improves clarity. The changes are consistent with the description and appear correct. I have one minor suggestion to further simplify the new _get_zip_runfiles_path function to make it more DRY.
| # NOTE: Use "+" for performance | ||
| if workspace_name: | ||
| # NOTE: Use "+" for performance | ||
| return _ZIP_RUNFILES_DIRECTORY_NAME + "/" + workspace_name + "/" + path | ||
| else: | ||
| return _ZIP_RUNFILES_DIRECTORY_NAME + "/" + path |
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.
This function can be slightly simplified to reduce code duplication and improve readability by building the path prefix incrementally. This change maintains the use of + for string concatenation for performance.
# NOTE: Use "+" for performance
prefix = _ZIP_RUNFILES_DIRECTORY_NAME + "/"
if workspace_name:
prefix += workspace_name + "/"
return prefix + path
|
@tobyh-canva, could you please check the profile for this PR? #3442 I suspect that there should be an improvement, but not sure how large. At least for setups that do not use zip builds and do not have the legacy "external" code path active it should eliminate all appearances of the |
| def map_zip_empty_filenames(list_paths_cb): | ||
| return [ | ||
| _get_zip_runfiles_path(path, workspace_name, legacy_external_runfiles) + "=" | ||
| # FIXME @aignas 2025-12-06: what kind of paths do we expect here? Will they |
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 would expect the paths to match what you'd see for a file in runfiles.files.
e.g., if a file in the runfiles comes from an external repo (and would have a short path of external/repo/foo.py or ../repo/foo.py), then there would be an empty file with a similar short path.
Looking at the investigation in #3380, it seems that we are calling
the startswith many times and I wanted to see if it would be possible
to optimize how it is done.
I also realized that no matter what target we have, we will be calling
the function once with a
__init__.pypath and we can inline this caseas a separate if statement checking for equality instead, which Starlark
optimizer should understand better.
Before this PR for every executable target we would go through the
legacy_external_runfiles and "__init__.py".startswith("external")andthis PR eliminates this.
Related to #3380 and #3381