search

bazel-contrib / bazel-gazelle

Gazelle is a Bazel build file generator for Bazel projects. It natively supports Go and protobuf, and it may be extended to support new languages and custom rule sets.
Apache License 2.0
1.21k stars 381 forks source link

perf: Improve merger.Mergefile #1920

Closed dzbarsky closed 2 months ago

dzbarsky commented 2 months ago

What type of PR is this? perf improvement

What package or component does this PR mostly affect? gazelle core

What does this PR do? Why is it needed? This slightly improve runtime perf due to avoiding allocations of the error object and reduced GC pressure Profile

Which issues(s) does this PR fix?

Fixes #

Other notes for review

fmeum commented 2 months ago

Do you have a lot of errors in your repo? The profile link doesn't work for me, so I'm not sure how much this actually saves.

dzbarsky commented 2 months ago

It saves around 30ms for us, which isn't a huge amount but these little things add up :)

I think it points to a bit of a bigger issue though in terms of scaling # of languages/plugins. What is happening in our case is that we have a predominantly Typescript repo with some Go, Rust, and Java code sprinkled in. We make the default target in each package (:foo in package foo) a ts_library if there are any TS files in the package, which there are in the majority of packages. The Go plugin tries to do the same thing, which is what leads to the creation of these errors (go_library generated target has same name but different kind as existing ts_library target). We currently aren't using Rust or Java with Gazelle, but presumably if we were, they might suffer from the same issue of trying to use the same target names.

One way to solve this would be to restrict which paths apply which plugins, but we typically group our code by functionality rather than by language, so it would require a lot of tuning of inclusions/exclusions.

Perhaps this can be addressed with a bigger restructuring of the Gazelle code that could allow to avoid more processing of the Go plugin if there are no Go files in the package?

fmeum commented 2 months ago

Thanks for the context. I also don't see how we could easily improve the high-level situation. But this low-level optimization is simple enough that I'm now convinced we should merge it.