perf: walk workspace directories in parallel #1893

jbedard · 2024-08-26T23:09:11Z

What type of PR is this?
Other

What package or component does this PR mostly affect?
all

What does this PR do? Why is it needed?

Walk the bazel workspace fs directories in parallel to build a structure representing the workspace before invoking the gazelle phases.

Similar to #1820 from @dzbarsky including using ~~the same~~ a simplified pathTrie struct but using a simple custom walkDir method instead of introducing a new external dep.

Which issues(s) does this PR fix?

Fixes #1819
Closes #1820

dzbarsky · 2024-08-27T19:07:13Z

Thanks @jbedard for running with #1820 and getting it to a state where it will hopefully be acceptable. I've confirmed that this has subtantially the same performance as the original PR (except for the Configure2 bit, which we can submit separately). The parallelization is indeed important, it's roughly a 2x speedup for our repo.

In addition, Jason and I have discussed how this opens up the path to potentially reading BUILD files in the initial traversal, which can parallelize additional work as well as allow to respect exclude directives during the parallel walk.

Interestingly enough, when I profile the approach here compared to fastwalk, I am seeing that fastwalk reimplemented os.Readdir in a faster way, and that's responsible for around .6s of our 4s runtime, i.e. 15%. However, fastwalk appears to spend more time in contention/orchestration of goroutines than the errgroup approach, so the overall runtime is around the same. Potentially os.Readdir can be further optimized in upstream Go stdlib.

dzbarsky · 2024-09-01T16:31:35Z

@tyler-french wdyt of this approach? We have been trying to land this perf improvement in one shape or another for months now

jbedard · 2024-09-04T19:40:20Z

@fmeum have you had a chance to look into this and compare with the original PR from @dzbarsky?

alandonham · 2024-09-12T18:33:54Z

@tyler-french wdyt of this approach? We have been trying to land this perf improvement in one shape or another for months now

I'm also interested to see some movement on this PR. I'm working with some rather large repositories, and these changes should solve some of the performance bottlenecks I've been investigating.

walk/walk.go

fmeum

Overall this looks good to me

walk/walk.go

fmeum

LGTM, should I merge (PR is marked as draft)?

DavidZbarsky-at · 2024-09-16T02:39:33Z

We wanted to double-check the perf numbers. Here's what I see on Airtable's repo:

No parallel walk:

fastwalk implementation from #1820

This PR's parallel walk:

So this PR is roughly similar to fastwalk, maybe a little faster, and much simpler. Looks like 15% speedup overall on our repo. Let's merge it!

jbedard · 2024-09-16T05:24:00Z

LGTM, should I merge (PR is marked as draft)?

I wanted to double check the improvements were still the same as when we last measured. Looks like @DavidZbarsky-at has confirmed it's still 15% with his tests 👍

tyler-french · 2024-09-15T19:48:08Z

walk/walk.go

+	}
+
+	// A channel to limit the number of concurrent goroutines
+	limitCh := make(chan struct{}, 100)


100 feels pretty hardcoded. Can we check the number of CPUs or max processes we can use? https://pkg.go.dev/runtime#NumCPU

The operations are likely I/O bound, so a limit based on the number of CPUs would likely be far too low. Eventually we may want to make this configurable, but any reasonably high number should give a speedup. In the end, the Go runtime would know best how to schedule I/O-blocked goroutines.

Today it's all I/O so I figured hardcoding something is fine and number of CPUs isn't really relevant. However that might change soon if we start putting stuff like BUILD-parsing into these goroutines.

tyler-french · 2024-09-15T19:49:19Z

walk/walk.go

 	}

+	visit(c, cexts, knownDirectives, updateRels, trie, wf, c.RepoRoot, "", false)


Can we make parallel walk configurable? It would be good to not enable by default, and allow users to opt in/out

Sorry I didn't see this and then this was merged. We can probably do this fairly easily.

Lets just make sure we do this (disable it by default) instead of reverting if any issues come up now that there has been a release.

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

@hofbi

jbedard force-pushed the trie branch from 1fe1a7b to f4834cb Compare August 26, 2024 23:15

jbedard marked this pull request as ready for review August 27, 2024 17:31

jbedard force-pushed the trie branch 2 times, most recently from f4834cb to e0ee28b Compare August 27, 2024 18:41

jbedard mentioned this pull request Sep 4, 2024

Configure - Starlark Roadmap aspect-build/aspect-cli#740

Open

3 tasks

jbedard force-pushed the trie branch 9 times, most recently from 12abec7 to 3df4f70 Compare September 13, 2024 06:38

DavidZbarsky-at reviewed Sep 13, 2024

View reviewed changes

walk/walk.go Show resolved Hide resolved

DavidZbarsky-at reviewed Sep 13, 2024

View reviewed changes

walk/walk.go Show resolved Hide resolved

DavidZbarsky-at reviewed Sep 13, 2024

View reviewed changes

walk/walk.go Outdated Show resolved Hide resolved

DavidZbarsky-at reviewed Sep 13, 2024

View reviewed changes

walk/walk.go Outdated Show resolved Hide resolved

jbedard force-pushed the trie branch 2 times, most recently from 3e46727 to cc4bede Compare September 13, 2024 22:19

jbedard marked this pull request as draft September 13, 2024 23:43

jbedard force-pushed the trie branch from 3a7d01a to 8d94a76 Compare September 14, 2024 02:09

fmeum reviewed Sep 14, 2024

View reviewed changes

walk/walk.go Outdated Show resolved Hide resolved

walk/walk.go Show resolved Hide resolved

walk/walk.go Outdated Show resolved Hide resolved

jbedard force-pushed the trie branch from 8d94a76 to 91c5c0d Compare September 14, 2024 20:58

jbedard requested a review from fmeum September 14, 2024 20:59

jbedard added 2 commits September 14, 2024 14:37

perf: walk workspace directories in parallel

7c9e195

limit using channel

e696e95

jbedard force-pushed the trie branch from 91c5c0d to e696e95 Compare September 14, 2024 21:37

use filepath.Join for os.* path

b12f792

fmeum approved these changes Sep 15, 2024

View reviewed changes

jbedard marked this pull request as ready for review September 16, 2024 05:20

jbedard requested a review from fmeum September 16, 2024 05:24

fmeum approved these changes Sep 16, 2024

View reviewed changes

fmeum enabled auto-merge (squash) September 16, 2024 06:18

fmeum merged commit bf2bb89 into bazel-contrib:master Sep 16, 2024
15 checks passed

jbedard deleted the trie branch September 16, 2024 06:24

tyler-french reviewed Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: walk workspace directories in parallel #1893

perf: walk workspace directories in parallel #1893

jbedard commented Aug 26, 2024 •

edited by fmeum

Loading

dzbarsky commented Aug 27, 2024

dzbarsky commented Sep 1, 2024

jbedard commented Sep 4, 2024

alandonham commented Sep 12, 2024 •

edited

Loading

fmeum left a comment

fmeum left a comment

DavidZbarsky-at commented Sep 16, 2024

jbedard commented Sep 16, 2024

tyler-french Sep 15, 2024

fmeum Sep 16, 2024

jbedard Sep 16, 2024

tyler-french Sep 15, 2024

jbedard Sep 24, 2024

		}

		visit(c, cexts, knownDirectives, updateRels, trie, wf, c.RepoRoot, "", false)

perf: walk workspace directories in parallel #1893

perf: walk workspace directories in parallel #1893

Conversation

jbedard commented Aug 26, 2024 • edited by fmeum Loading

dzbarsky commented Aug 27, 2024

dzbarsky commented Sep 1, 2024

jbedard commented Sep 4, 2024

alandonham commented Sep 12, 2024 • edited Loading

fmeum left a comment

Choose a reason for hiding this comment

fmeum left a comment

Choose a reason for hiding this comment

DavidZbarsky-at commented Sep 16, 2024

jbedard commented Sep 16, 2024

tyler-french Sep 15, 2024

Choose a reason for hiding this comment

fmeum Sep 16, 2024

Choose a reason for hiding this comment

jbedard Sep 16, 2024

Choose a reason for hiding this comment

tyler-french Sep 15, 2024

Choose a reason for hiding this comment

jbedard Sep 24, 2024

Choose a reason for hiding this comment

jbedard commented Aug 26, 2024 •

edited by fmeum

Loading

alandonham commented Sep 12, 2024 •

edited

Loading