Skip to content

Commit 9559b20

Browse files
authored
refactor(core): get_zip_runfiles_path should call startswith less (#3442)
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__.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 #3380 and #3381
1 parent 4d9c2b1 commit 9559b20

File tree

1 file changed

+23
-17
lines changed

1 file changed

+23
-17
lines changed

python/private/py_executable.bzl

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ load(":venv_runfiles.bzl", "create_venv_app_files")
7070
_py_builtins = py_internal
7171
_EXTERNAL_PATH_PREFIX = "external"
7272
_ZIP_RUNFILES_DIRECTORY_NAME = "runfiles"
73+
_INIT_PY = "__init__.py"
7374
_LAUNCHER_MAKER_TOOLCHAIN_TYPE = "@bazel_tools//tools/launcher:launcher_maker_toolchain_type"
7475

7576
# Non-Google-specific attributes for executables
@@ -834,14 +835,14 @@ def _create_zip_file(ctx, *, output, zip_main, runfiles):
834835
manifest.add("__main__.py={}".format(zip_main.path))
835836
manifest.add("__init__.py=")
836837
manifest.add(
837-
"{}=".format(
838-
_get_zip_runfiles_path("__init__.py", workspace_name, legacy_external_runfiles),
839-
),
838+
"{}=".format(_get_zip_runfiles_path(_INIT_PY, workspace_name)),
840839
)
841840

842841
def map_zip_empty_filenames(list_paths_cb):
843842
return [
844-
_get_zip_runfiles_path(path, workspace_name, legacy_external_runfiles) + "="
843+
# FIXME @aignas 2025-12-06: what kind of paths do we expect here? Will they
844+
# ever start with `../` or `external`?
845+
_get_zip_runfiles_path_legacy(path, workspace_name, legacy_external_runfiles) + "="
845846
for path in list_paths_cb().to_list()
846847
]
847848

@@ -856,7 +857,7 @@ def _create_zip_file(ctx, *, output, zip_main, runfiles):
856857
def map_zip_runfiles(file):
857858
return (
858859
# NOTE: Use "+" for performance
859-
_get_zip_runfiles_path(file.short_path, workspace_name, legacy_external_runfiles) +
860+
_get_zip_runfiles_path_legacy(file.short_path, workspace_name, legacy_external_runfiles) +
860861
"=" + file.path
861862
)
862863

@@ -893,23 +894,28 @@ def _create_zip_file(ctx, *, output, zip_main, runfiles):
893894
progress_message = "Building Python zip: %{label}",
894895
)
895896

896-
def _get_zip_runfiles_path(path, workspace_name, legacy_external_runfiles):
897-
maybe_workspace = ""
898-
if legacy_external_runfiles and path.startswith(_EXTERNAL_PATH_PREFIX):
899-
zip_runfiles_path = path.removeprefix(_EXTERNAL_PATH_PREFIX)
897+
def _get_zip_runfiles_path(path, workspace_name = ""):
898+
# NOTE @aignas 2025-12-06: This is to avoid the prefix checking in the very
899+
# trivial case that is always happening once per this function call
900+
901+
# NOTE: Use "+" for performance
902+
if workspace_name:
903+
# NOTE: Use "+" for performance
904+
return _ZIP_RUNFILES_DIRECTORY_NAME + "/" + workspace_name + "/" + path
900905
else:
906+
return _ZIP_RUNFILES_DIRECTORY_NAME + "/" + path
907+
908+
def _get_zip_runfiles_path_legacy(path, workspace_name, legacy_external_runfiles):
909+
if legacy_external_runfiles and path.startswith(_EXTERNAL_PATH_PREFIX):
910+
return _get_zip_runfiles_path(path.removeprefix(_EXTERNAL_PATH_PREFIX))
911+
elif path.startswith("../"):
901912
# NOTE: External runfiles (artifacts in other repos) will have a leading
902913
# path component of "../" so that they refer outside the main workspace
903914
# directory and into the runfiles root. So we simplify it, e.g.
904915
# "workspace/../foo/bar" to simply "foo/bar".
905-
if path.startswith("../"):
906-
zip_runfiles_path = path[3:]
907-
else:
908-
zip_runfiles_path = path
909-
maybe_workspace = workspace_name + "/"
910-
911-
# NOTE: Use "+" for performance
912-
return _ZIP_RUNFILES_DIRECTORY_NAME + "/" + maybe_workspace + zip_runfiles_path
916+
return _get_zip_runfiles_path(path[3:])
917+
else:
918+
return _get_zip_runfiles_path(path, workspace_name)
913919

914920
def _create_executable_zip_file(
915921
ctx,

0 commit comments

Comments
 (0)