Skip to content
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

language/proto: move known_imports from compiletime to runtime #1307

Closed
wants to merge 1 commit into from

Conversation

pcj
Copy link
Contributor

@pcj pcj commented Jul 19, 2022

What type of PR is this?

Bug fix / Improvement

What package or component does this PR mostly affect?

language/proto

Which issues(s) does this PR fix?

Fixes #1306

What does this PR do? Why is it needed?

This moves the known_imports feature from compiletime to runtime. Rather than generating go source code from the proto.csv file, it embeds the csv file into the binary and parses it in a lazy manner.

Procedure:

  1. bazel build //cmd/gazelle
  2. make a trivial change in the language/proto package
  3. bazel build //cmd/gazelle again.

Before (master):

$ bazel build //cmd/gazelle
Target //cmd/gazelle:gazelle up-to-date:
  bazel-bin/cmd/gazelle/gazelle_/gazelle
INFO: Elapsed time: 17.915s, Critical Path: 17.28s
INFO: 5 processes: 1 internal, 4 darwin-sandbox.
INFO: Build completed successfully, 5 total actions
$ du -sh bazel-bin/cmd/gazelle/gazelle_/gazelle 78M	bazel-bin/cmd/gazelle/gazelle_/gazelle

After (this PR):

$ bazel build //cmd/gazelle
Target //:gazelle up-to-date:
  bazel-bin/gazelle-runner.bash
  bazel-bin/gazelle
INFO: Elapsed time: 2.458s, Critical Path: 1.82s
INFO: 5 processes: 1 internal, 4 darwin-sandbox.
INFO: Build completed successfully, 5 total actions
$ du -sh bazel-bin/cmd/gazelle/gazelle_/gazelle 10M	bazel-bin/cmd/gazelle/gazelle_/gazelle

Benchmark

Running tool: /opt/homebrew/opt/go/libexec/bin/go test -benchmem -run=^$ -bench ^BenchmarkParseCSV$ github.com/bazelbuild/bazel-gazelle/language/proto

goos: darwin
goarch: arm64
pkg: github.com/bazelbuild/bazel-gazelle/language/proto
BenchmarkParseCSV-10    	     105	  11253252 ns/op	 2454368 B/op	   12792 allocs/op
PASS
ok  	github.com/bazelbuild/bazel-gazelle/language/proto	2.157s

So, with this PR there is a 15s decrease in compile time, 68MB decrease in compiled binary size in exchange for 11ms increase at runtime (which might be avoided depending on user configuration).

I did not profile the memory requirements during bazel build compilation, but I suspect it is significantly lower.

@achew22
Copy link
Member

achew22 commented Jul 20, 2022

Maybe I'm mistaken, but I believe this means that you can no longer go install gazelle. You need to have a working Bazel in order to bootstrap your way in. Also, out of curiosity, does this mean you have to distribute the binary with a CSV describing the known imports?

@achew22
Copy link
Member

achew22 commented Jul 20, 2022

Oh, I'm also curious, how does this impact performance? Is there any perceptible difference in startup time for Gazelle since it has to parse this?

@pcj
Copy link
Contributor Author

pcj commented Jul 20, 2022

but I believe this means that you can no longer go install gazelle

No, it's still go installable. My initial attempt used go:embed to embed the csv file, but it turned out that was incompatible with the method used by internal/go_repository_tools.bzl (for the go_repository rule). The sources are symlinked prior to the compile step, but go:embed refuses any files across a symlink boundary. As such, there is a new file proto_csv.go which simply contains the CSV string.

how does this impact performance? Is there any perceptible difference in startup time

I can't seem to tell a difference. From the benchmark test, it takes 11-12ms to parse the CSV. This is done lazily, so startup time is not affected. The first time a call to knownImport will incur the 11ms penalty.

@pcj
Copy link
Contributor Author

pcj commented Aug 22, 2022

@achew22 ping. Any remaining questions to address here?

@jfirebaugh
Copy link

Anything we can help with to get this PR merged?

We've been running a patched version of Gazelle at Figma with this change to avoid it getting OOM killed. Seems to be working.

@achew22
Copy link
Member

achew22 commented Sep 19, 2022

I've just merged #1333. Thanks for all your hard work on this one @pcj. It was great to see people engaging with the problems they're having and sending in fixes.

@achew22 achew22 closed this Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

language/proto: high compile time memory requirements
3 participants