Skip to content

Makefile: cache go env values#25870

Merged
marcoandredinis merged 1 commit intomasterfrom
marco/makefile_cache_goenv
May 9, 2023
Merged

Makefile: cache go env values#25870
marcoandredinis merged 1 commit intomasterfrom
marco/makefile_cache_goenv

Conversation

@marcoandredinis
Copy link
Copy Markdown
Contributor

Spawning shells is expensive for Makefile targets.
We use the OS and ARCH variables in a lot of places.
They come from spawning a subshell and then executing go env GOOS or go env GOARCH.

So, everytime we need them, we spawn a subshell.
Just parsing the Makefile adds quite some time, even if the target itself is fast.

This PR caches those values, so that we only need to spawn a subshell once per execution.

Tests show that running make print-version is now much faster:
Using linux: drop from 56s to 0.240s
Using MacOS M1: drop from 291ms to 119ms
YMMV

@github-actions github-actions Bot requested review from espadolini and hugoShaka May 9, 2023 08:24
Spawning shells is expensive for Makefile targets.
We use the `OS` and `ARCH` variables in a lot of places.
They come from spawning a subshell and then executing `go env GOOS` or
`go env GOARCH`.

So, everytime we need them, we spawn a subshells.
Just parsing the Makefile adds quite some time, even if the target
itself is fast.

This PR caches those values, so that we only need to spawn a subshell
once per execution.

Tests show that running `make print-version` is now much faster:
Using linux: drop from 56s to 0.240s
Using MacOS M1: drop from 291ms to 119ms
YMMV
@marcoandredinis marcoandredinis force-pushed the marco/makefile_cache_goenv branch from 9bd5868 to d94841e Compare May 9, 2023 08:32
Copy link
Copy Markdown
Contributor

@camscale camscale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really odd that you're seeing 56s to run print-version. Takes 100ms for me. But with this change it takes 25ms, so that's worth it.

Note for others reading this: $(OS) and $(ARCH) were always being evaluated anyway because they are used in the structure of the Makefile (ifeq ("$(ARCH)","linux") and we have a bare export directive, so it would be evaluated for export anyway. Usually a recursively expanded var is only expanded if it is referenced, so on first glance it looks like it would required go be installed regardless of target being run, but that is already the case because of the aforementioned reasons.

Copy link
Copy Markdown
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to figure out why the go env invocation is so much slower on Linux. @marcoandredinis, were you in docker and/or emulating x86 from arm?

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from hugoShaka May 9, 2023 09:26
@marcoandredinis
Copy link
Copy Markdown
Contributor Author

marcoandredinis commented May 9, 2023

I'm seeing a lot of calls to go env ... when running in linux:
(this is using master, current branch has less calls)

$ uname -a
Linux rfc1178 6.3.1 #1-NixOS SMP PREEMPT_DYNAMIC Sun Apr 30 23:32:26 UTC 2023 x86_64 GNU/Linux
$ make --version
GNU Make 4.4.1
Built for x86_64-pc-linux-gnu
...
$ make print-version -d 2>&1 | wc -l
160949

Compared to Mac

$ uname -a
Darwin MarcoM1.local 22.4.0 Darwin Kernel Version 22.4.0: Mon Mar  6 20:59:28 PST 2023; root:xnu-8796.101.5~3/RELEASE_ARM64_T6000 arm64
$ make --version
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

This program built for i386-apple-darwin11.3.0
$ make print-version -d 2>&1 | wc -l
    1307

@marcoandredinis
Copy link
Copy Markdown
Contributor Author

marcoandredinis commented May 9, 2023

were you in docker and/or emulating x86 from arm?

I was using a linux laptop, no other layer (no docker nor using any virtualization)

@marcoandredinis marcoandredinis enabled auto-merge May 9, 2023 12:38
@marcoandredinis marcoandredinis added this pull request to the merge queue May 9, 2023
Merged via the queue into master with commit 9138260 May 9, 2023
@marcoandredinis marcoandredinis deleted the marco/makefile_cache_goenv branch May 9, 2023 13:49
@zmb3 zmb3 mentioned this pull request Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants