Skip to content

Commit

Permalink
More go_context speedups (#4058)
Browse files Browse the repository at this point in the history
**What type of PR is this?**
Starlark cleanup

**What does this PR do? Why is it needed?**
After the previous cleanups, `mode` and `GoConfigInfo` are pretty
similar, the only differences were `link` vs `linkmode` and goos/goarch
were missing on `GoConfigInfo`. So we add the default ones from the
toolchain, and this lets us use the `GoConfigInfo` in the majority of
cases without having to copy all the properties to `mode`. Another
option is to move the `goos`/`goarch` from `mode` to top-level
properties on `go_context`, though this is potentially a breaking
change. Having them on GoConfigInfo seems like a reasonable direction to
move in anyway if we plan to use Bazel's platforms to control what we
target.

This required aligning the `linkmode` name, which is good for
consistency anyway.

We pass in `goos`/`goarch` to `go_context` since they are only set in
terminal rules and we don't need to look them up for intermediate libs.

I also noticed `getattr` from the proto libs showing up in profiles, so
we clean that up as well.

This speeds up go_context another 2x, saving 100ms in buildbuddy repo

**Which issues(s) does this PR fix?**

Fixes #

**Other notes for review**
  • Loading branch information
dzbarsky authored Aug 21, 2024
1 parent ee8b2b8 commit 5933f87
Show file tree
Hide file tree
Showing 11 changed files with 115 additions and 99 deletions.
4 changes: 2 additions & 2 deletions go/private/actions/archive.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d

testfilter = getattr(source.library, "testfilter", None)
pre_ext = ""
if go.mode.link == LINKMODE_C_ARCHIVE:
if go.mode.linkmode == LINKMODE_C_ARCHIVE:
pre_ext = "_" # avoid collision with go_binary output file with .a extension
elif testfilter == "exclude":
pre_ext = ".internal"
Expand Down Expand Up @@ -96,7 +96,7 @@ def emit_archive(go, source = None, _recompile_suffix = "", recompile_internal_d
cxxopts = cxxopts,
clinkopts = clinkopts,
)
if go.mode.link in (LINKMODE_C_SHARED, LINKMODE_C_ARCHIVE):
if go.mode.linkmode in (LINKMODE_C_SHARED, LINKMODE_C_ARCHIVE):
out_cgo_export_h = go.declare_file(go, path = "_cgo_install.h")
cgo_deps = cgo.deps
runfiles = runfiles.merge(cgo.runfiles)
Expand Down
6 changes: 3 additions & 3 deletions go/private/actions/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,12 @@ def emit_binary(

archive = go.archive(go, source)
if not executable:
if go.mode.link == LINKMODE_C_SHARED:
if go.mode.linkmode == LINKMODE_C_SHARED:
name = "lib" + name # shared libraries need a "lib" prefix in their name
extension = goos_to_shared_extension(go.mode.goos)
elif go.mode.link == LINKMODE_C_ARCHIVE:
elif go.mode.linkmode == LINKMODE_C_ARCHIVE:
extension = ARCHIVE_EXTENSION
elif go.mode.link == LINKMODE_PLUGIN:
elif go.mode.linkmode == LINKMODE_PLUGIN:
extension = goos_to_shared_extension(go.mode.goos)
else:
extension = goos_to_extension(go.mode.goos)
Expand Down
8 changes: 4 additions & 4 deletions go/private/actions/link.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ def emit_link(
tool_args.add_all(extld)
if extld and (go.mode.static or
go.mode.race or
go.mode.link != LINKMODE_NORMAL or
go.mode.linkmode != LINKMODE_NORMAL or
go.mode.goos == "windows" and go.mode.msan):
# Force external linking for the following conditions:
# * Mode is static but not pure: -static must be passed to the C
Expand All @@ -101,9 +101,9 @@ def emit_link(

if go.mode.static:
extldflags.append("-static")
if go.mode.link != LINKMODE_NORMAL:
builder_args.add("-buildmode", go.mode.link)
if go.mode.link == LINKMODE_PLUGIN:
if go.mode.linkmode != LINKMODE_NORMAL:
builder_args.add("-buildmode", go.mode.linkmode)
if go.mode.linkmode == LINKMODE_PLUGIN:
tool_args.add("-pluginpath", archive.data.importpath)

# TODO(zbarsky): Bazel versions older than 7.2.0 do not properly deduplicate this dep
Expand Down
2 changes: 1 addition & 1 deletion go/private/actions/stdlib.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def _should_use_sdk_stdlib(go):
not go.mode.msan and
not go.mode.pure and
not go.mode.gc_goopts and
go.mode.link == LINKMODE_NORMAL)
go.mode.linkmode == LINKMODE_NORMAL)

def _build_stdlib_list_json(go):
sdk = go.sdk
Expand Down
57 changes: 52 additions & 5 deletions go/private/context.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load(
"@bazel_skylib//lib:structs.bzl",
"structs",
)
load(
"@bazel_skylib//rules:common_settings.bzl",
"BuildSettingInfo",
Expand Down Expand Up @@ -53,8 +57,9 @@ load(
)
load(
":mode.bzl",
"get_mode",
"LINKMODE_NORMAL",
"installsuffix",
"validate_mode",
)
load(
":providers.bzl",
Expand Down Expand Up @@ -375,6 +380,24 @@ def get_nogo(go):
else:
return None

default_go_config_info = GoConfigInfo(
static = False,
race = False,
msan = False,
pure = False,
strip = False,
debug = False,
linkmode = LINKMODE_NORMAL,
gc_linkopts = [],
tags = [],
stamp = False,
cover_format = None,
gc_goopts = [],
amd64 = None,
arm = None,
pgoprofile = None,
)

def go_context(
ctx,
attr = None,
Expand All @@ -383,7 +406,9 @@ def go_context(
importmap = None,
embed = None,
importpath_aliases = None,
go_context_data = None):
go_context_data = None,
goos = "auto",
goarch = "auto"):
"""Returns an API used to build Go code.
See /go/toolchains.rst#go-context
Expand Down Expand Up @@ -419,7 +444,21 @@ def go_context(
stdlib = go_context_data[GoStdLib]
go_context_info = go_context_data[GoContextInfo]

mode = get_mode(ctx, toolchain, cgo_context_info, go_config_info)
if goos == "auto" and goarch == "auto" and cgo_context_info:
# Fast-path to reuse the GoConfigInfo as-is
mode = go_config_info
else:
if go_config_info == None:
go_config_info = default_go_config_info
mode_kwargs = structs.to_dict(go_config_info)
mode_kwargs["goos"] = toolchain.default_goos if goos == "auto" else goos
mode_kwargs["goarch"] = toolchain.default_goarch if goarch == "auto" else goarch
if not cgo_context_info:
if getattr(ctx.attr, "pure", None) == "off":
fail("{} has pure explicitly set to off, but no C++ toolchain could be found for its platform".format(ctx.label))
mode_kwargs["pure"] = True
mode = struct(**mode_kwargs)
validate_mode(mode)

if stdlib:
goroot = stdlib.root_file.dirname
Expand Down Expand Up @@ -879,7 +918,11 @@ def _go_config_impl(ctx):
if msan:
tags.append("msan")

return [GoConfigInfo(
toolchain = ctx.toolchains[GO_TOOLCHAIN]

go_config_info = GoConfigInfo(
goos = toolchain.default_goos,
goarch = toolchain.default_goarch,
static = ctx.attr.static[BuildSettingInfo].value,
race = race,
msan = msan,
Expand All @@ -895,7 +938,10 @@ def _go_config_impl(ctx):
amd64 = ctx.attr.amd64,
arm = ctx.attr.arm,
pgoprofile = pgoprofile,
)]
)
validate_mode(go_config_info)

return [go_config_info]

go_config = rule(
implementation = _go_config_impl,
Expand Down Expand Up @@ -953,6 +999,7 @@ go_config = rule(
doc = """Collects information about build settings in the current
configuration. Rules may depend on this instead of depending on all
the build settings directly.""",
toolchains = [GO_TOOLCHAIN],
)

def _expand_opts(go, attribute_name, opts):
Expand Down
95 changes: 21 additions & 74 deletions go/private/mode.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@
# See the License for the specific language governing permissions and
# limitations under the License.

load(":providers.bzl", "GoConfigInfo")

# Modes are documented in go/modes.rst#compilation-modes

LINKMODE_NORMAL = "normal"
Expand Down Expand Up @@ -54,76 +52,22 @@ def mode_string(mode):
result.append("debug")
if mode.strip:
result.append("stripped")
if not result or not mode.link == LINKMODE_NORMAL:
result.append(mode.link)
if not result or not mode.linkmode == LINKMODE_NORMAL:
result.append(mode.linkmode)
if mode.gc_goopts:
result.extend(mode.gc_goopts)
return "_".join(result)

default_go_config_info = GoConfigInfo(
static = False,
race = False,
msan = False,
pure = False,
strip = False,
debug = False,
linkmode = LINKMODE_NORMAL,
gc_linkopts = [],
tags = [],
stamp = False,
cover_format = None,
gc_goopts = [],
amd64 = None,
arm = None,
pgoprofile = None,
)

def get_mode(ctx, go_toolchain, cgo_context_info, go_config_info):
if go_config_info == None:
go_config_info = default_go_config_info

if not cgo_context_info:
if getattr(ctx.attr, "pure", None) == "off":
fail("{} has pure explicitly set to off, but no C++ toolchain could be found for its platform".format(ctx.label))
pure = True
else:
pure = go_config_info.pure

race = go_config_info.race
msan = go_config_info.msan
linkmode = go_config_info.linkmode
goos = go_toolchain.default_goos if getattr(ctx.attr, "goos", "auto") == "auto" else ctx.attr.goos
goarch = go_toolchain.default_goarch if getattr(ctx.attr, "goarch", "auto") == "auto" else ctx.attr.goarch

def validate_mode(mode):
# TODO(jayconrod): check for more invalid and contradictory settings.
if pure:
if race:
if mode.pure:
if mode.race:
fail("race instrumentation can't be enabled when cgo is disabled. Check that pure is not set to \"off\" and a C/C++ toolchain is configured.")
if msan:
if mode.msan:
fail("msan instrumentation can't be enabled when cgo is disabled. Check that pure is not set to \"off\" and a C/C++ toolchain is configured.")
if linkmode in LINKMODES_REQUIRING_EXTERNAL_LINKING:
if mode.linkmode in LINKMODES_REQUIRING_EXTERNAL_LINKING:
fail(("linkmode '{}' can't be used when cgo is disabled. Check that pure is not set to \"off\" and that a C/C++ toolchain is configured for " +
"your current platform. If you defined a custom platform, make sure that it has the @io_bazel_rules_go//go/toolchain:cgo_on constraint value.").format(linkmode))

return struct(
static = go_config_info.static,
race = race,
msan = msan,
pure = pure,
link = linkmode,
gc_linkopts = go_config_info.gc_linkopts,
strip = go_config_info.strip,
stamp = go_config_info.stamp,
debug = go_config_info.debug,
goos = goos,
goarch = goarch,
tags = go_config_info.tags,
cover_format = go_config_info.cover_format,
amd64 = go_config_info.amd64,
arm = go_config_info.arm,
gc_goopts = go_config_info.gc_goopts,
pgoprofile = go_config_info.pgoprofile,
)
"your current platform. If you defined a custom platform, make sure that it has the @io_bazel_rules_go//go/toolchain:cgo_on constraint value.").format(mode.linkmode))

def installsuffix(mode):
s = mode.goos + "_" + mode.goarch
Expand Down Expand Up @@ -185,28 +129,31 @@ _LINK_PIE_PLATFORMS = {
"freebsd/amd64": None,
}

def _platform(mode):
return mode.goos + "/" + mode.goarch

def link_mode_arg(mode):
# based on buildModeInit in cmd/go/internal/work/init.go
platform = mode.goos + "/" + mode.goarch
if mode.link == LINKMODE_C_ARCHIVE:
if mode.linkmode == LINKMODE_C_ARCHIVE:
platform = _platform(mode)
if (platform in _LINK_C_ARCHIVE_PLATFORMS or
mode.goos in _LINK_C_ARCHIVE_GOOS and platform != "linux/ppc64"):
return "-shared"
elif mode.link == LINKMODE_C_SHARED:
elif mode.linkmode == LINKMODE_C_SHARED:
if mode.goos in _LINK_C_SHARED_GOOS:
return "-shared"
elif mode.link == LINKMODE_PLUGIN:
if platform in _LINK_PLUGIN_PLATFORMS:
elif mode.linkmode == LINKMODE_PLUGIN:
if _platform(mode) in _LINK_PLUGIN_PLATFORMS:
return "-dynlink"
elif mode.link == LINKMODE_PIE:
if platform in _LINK_PIE_PLATFORMS:
elif mode.linkmode == LINKMODE_PIE:
if _platform(mode) in _LINK_PIE_PLATFORMS:
return "-shared"
return None

def extldflags_from_cc_toolchain(go):
if not go.cgo_tools:
return []
elif go.mode.link in (LINKMODE_SHARED, LINKMODE_PLUGIN, LINKMODE_C_SHARED):
elif go.mode.linkmode in (LINKMODE_SHARED, LINKMODE_PLUGIN, LINKMODE_C_SHARED):
return go.cgo_tools.ld_dynamic_lib_options
else:
# NOTE: in c-archive mode, -extldflags are ignored by the linker.
Expand All @@ -217,9 +164,9 @@ def extldflags_from_cc_toolchain(go):
def extld_from_cc_toolchain(go):
if not go.cgo_tools:
return []
elif go.mode.link in (LINKMODE_SHARED, LINKMODE_PLUGIN, LINKMODE_C_SHARED, LINKMODE_PIE):
elif go.mode.linkmode in (LINKMODE_SHARED, LINKMODE_PLUGIN, LINKMODE_C_SHARED, LINKMODE_PIE):
return ["-extld", go.cgo_tools.ld_dynamic_lib_path]
elif go.mode.link == LINKMODE_C_ARCHIVE:
elif go.mode.linkmode == LINKMODE_C_ARCHIVE:
if go.mode.goos in ["darwin", "ios"]:
# TODO(jayconrod): on macOS, set -extar. At this time, wrapped_ar is
# a bash script without a shebang line, so we can't execute it. We
Expand Down
12 changes: 7 additions & 5 deletions go/private/rules/binary.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,11 @@ def _go_binary_impl(ctx):
embed = ctx.attr.embed,
# It's a list because it is transitioned.
go_context_data = ctx.attr._go_context_data[0],
goos = ctx.attr.goos,
goarch = ctx.attr.goarch,
)

is_main = go.mode.link not in (LINKMODE_SHARED, LINKMODE_PLUGIN)
is_main = go.mode.linkmode not in (LINKMODE_SHARED, LINKMODE_PLUGIN)
library = go.new_library(go, importable = False, is_main = is_main)
source = go.library_to_source(go, ctx.attr, library, ctx.coverage_instrumented())
name = ctx.attr.basename
Expand Down Expand Up @@ -156,7 +158,7 @@ def _go_binary_impl(ctx):
),
]

if go.mode.link in LINKMODES_EXECUTABLE:
if go.mode.linkmode in LINKMODES_EXECUTABLE:
env = {}
for k, v in ctx.attr.env.items():
env[k] = ctx.expand_location(v, ctx.attr.data)
Expand All @@ -182,7 +184,7 @@ def _go_binary_impl(ctx):
))

# If the binary's linkmode is c-archive or c-shared, expose CcInfo
if go.cgo_tools and go.mode.link in (LINKMODE_C_ARCHIVE, LINKMODE_C_SHARED):
if go.cgo_tools and go.mode.linkmode in (LINKMODE_C_ARCHIVE, LINKMODE_C_SHARED):
cc_import_kwargs = {
"linkopts": {
"darwin": [],
Expand All @@ -198,9 +200,9 @@ def _go_binary_impl(ctx):
target_file = cgo_exports[0],
)
cc_import_kwargs["hdrs"] = depset([header])
if go.mode.link == LINKMODE_C_SHARED:
if go.mode.linkmode == LINKMODE_C_SHARED:
cc_import_kwargs["dynamic_library"] = executable
elif go.mode.link == LINKMODE_C_ARCHIVE:
elif go.mode.linkmode == LINKMODE_C_ARCHIVE:
cc_import_kwargs["static_library"] = executable
cc_import_kwargs["alwayslink"] = True

Expand Down
2 changes: 2 additions & 0 deletions go/private/rules/test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ def _go_test_impl(ctx):
embed = ctx.attr.embed,
# It's a list because it is transitioned.
go_context_data = ctx.attr._go_context_data[0],
goos = ctx.attr.goos,
goarch = ctx.attr.goarch,
)

# Compile the library to test with internal white box tests
Expand Down
2 changes: 1 addition & 1 deletion go/private/rules/transition.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def _go_transition_impl(settings, attr):
#
# NOTE(bazelbuild/bazel#11409): Calling fail here for invalid combinations
# of flags reports an error but does not stop the build.
# In any case, get_mode should mainly be responsible for reporting
# In any case, validate_mode should mainly be responsible for reporting
# invalid modes, since it also takes --features flags into account.

original_settings = settings
Expand Down
2 changes: 1 addition & 1 deletion go/providers.rst
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ method. In general, only rules_go should need to build or handle these.
+--------------------------------+-----------------------------------------------------------------+
| The go library that this GoSource was generated from. |
+--------------------------------+-----------------------------------------------------------------+
| :param:`mode` | :type:`Mode` |
| :param:`mode` | :type:`GoConfigInfo` |
+--------------------------------+-----------------------------------------------------------------+
| The mode this library is being built for. |
+--------------------------------+-----------------------------------------------------------------+
Expand Down
Loading

0 comments on commit 5933f87

Please sign in to comment.