From de3604453f61e4740afa5a85a1b09f0b8e793792 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Wed, 20 Apr 2022 21:21:55 -0700 Subject: [PATCH 1/2] Fix some problems with soong metrics loading If we didn't need to run soong_build during the current run, we still try to load the soong metrics. But in the case of `dist`, that's in a directory that is not guaranteed to persist between runs. Make loading the soong metrics optional if the file does not exist. Also fixes a variable shadowing issue that meant we never passed it into ctx.Metrics. Test: treehugger Change-Id: Ic836282f4d13e91daa0e7241ad7c488de3293d8b --- ui/build/soong.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/ui/build/soong.go b/ui/build/soong.go index c7f22f946..8992b4f07 100644 --- a/ui/build/soong.go +++ b/ui/build/soong.go @@ -15,7 +15,9 @@ package build import ( + "errors" "fmt" + "io/fs" "io/ioutil" "os" "path/filepath" @@ -491,10 +493,14 @@ func runSoong(ctx Context, config Config) { ninja("bootstrap", "bootstrap.ninja", targets...) - var soongBuildMetrics *soong_metrics_proto.SoongBuildMetrics if shouldCollectBuildSoongMetrics(config) { soongBuildMetrics := loadSoongBuildMetrics(ctx, config) - logSoongBuildMetrics(ctx, soongBuildMetrics) + if soongBuildMetrics != nil { + logSoongBuildMetrics(ctx, soongBuildMetrics) + if ctx.Metrics != nil { + ctx.Metrics.SetSoongBuildMetrics(soongBuildMetrics) + } + } } distGzipFile(ctx, config, config.SoongNinjaFile(), "soong") @@ -504,9 +510,6 @@ func runSoong(ctx Context, config Config) { distGzipFile(ctx, config, config.SoongMakeVarsMk(), "soong") } - if shouldCollectBuildSoongMetrics(config) && ctx.Metrics != nil { - ctx.Metrics.SetSoongBuildMetrics(soongBuildMetrics) - } if config.JsonModuleGraph() { distGzipFile(ctx, config, config.ModuleGraphFile(), "soong") } @@ -538,8 +541,12 @@ func shouldCollectBuildSoongMetrics(config Config) bool { func loadSoongBuildMetrics(ctx Context, config Config) *soong_metrics_proto.SoongBuildMetrics { soongBuildMetricsFile := filepath.Join(config.LogsDir(), "soong_build_metrics.pb") - buf, err := ioutil.ReadFile(soongBuildMetricsFile) - if err != nil { + buf, err := os.ReadFile(soongBuildMetricsFile) + if errors.Is(err, fs.ErrNotExist) { + // Soong may not have run during this invocation + ctx.Verbosef("Failed to read metrics file, %s: %s", soongBuildMetricsFile, err) + return nil + } else if err != nil { ctx.Fatalf("Failed to load %s: %s", soongBuildMetricsFile, err) } soongBuildMetrics := &soong_metrics_proto.SoongBuildMetrics{} From 80d72618217a5ba988a8d944e241eb147f184e0f Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Wed, 20 Apr 2022 21:45:00 -0700 Subject: [PATCH 2/2] Background distGzipFile to speed up CI builds These can take a minute or more of build time, and currently hold up other processing. We could move to a parallel compression scheme to speed them up, but it's simpler just to background them and continue the build, waiting to make sure they've finished before exiting. Testing this on git_master/flame-userdebug: This brings a local no-op `m nothing dist` build from 2m1s to 1m39s, even though most of that time is still spent waiting on these (since there is very little else happening in a `nothing` build when Soong and Kati don't need to run). Running `touch art/Android.mk; m nothing dist` (so the above, but with Kati) goes from 3m39s to 2m2s. We spent 9 seconds at the end waiting for these to finish. CI cases that almost always run Kati plus some number of other actions are likely to completely hide the time taken to dist these files. Bug: 229932999 Test: Check $DIST_DIR, files still exist Change-Id: I1fb78d7c4d5103d72b5d71d9277dea0d452f8968 --- ui/build/build.go | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/ui/build/build.go b/ui/build/build.go index d261f8947..aadf4af4e 100644 --- a/ui/build/build.go +++ b/ui/build/build.go @@ -18,6 +18,7 @@ import ( "io/ioutil" "os" "path/filepath" + "sync" "text/template" "android/soong/ui/metrics" @@ -205,6 +206,8 @@ func Build(ctx Context, config Config) { return } + defer waitForDist(ctx) + // checkProblematicFiles aborts the build if Android.mk or CleanSpec.mk are found at the root of the tree. checkProblematicFiles(ctx) @@ -329,8 +332,18 @@ func Build(ctx Context, config Config) { } } +var distWaitGroup sync.WaitGroup + +// waitForDist waits for all backgrounded distGzipFile and distFile writes to finish +func waitForDist(ctx Context) { + ctx.BeginTrace("soong_ui", "dist") + defer ctx.EndTrace() + + distWaitGroup.Wait() +} + // distGzipFile writes a compressed copy of src to the distDir if dist is enabled. Failures -// are printed but non-fatal. +// are printed but non-fatal. Uses the distWaitGroup func for backgrounding (optimization). func distGzipFile(ctx Context, config Config, src string, subDirs ...string) { if !config.Dist() { return @@ -343,13 +356,17 @@ func distGzipFile(ctx Context, config Config, src string, subDirs ...string) { ctx.Printf("failed to mkdir %s: %s", destDir, err.Error()) } - if err := gzipFileToDir(src, destDir); err != nil { - ctx.Printf("failed to dist %s: %s", filepath.Base(src), err.Error()) - } + distWaitGroup.Add(1) + go func() { + defer distWaitGroup.Done() + if err := gzipFileToDir(src, destDir); err != nil { + ctx.Printf("failed to dist %s: %s", filepath.Base(src), err.Error()) + } + }() } // distFile writes a copy of src to the distDir if dist is enabled. Failures are printed but -// non-fatal. +// non-fatal. Uses the distWaitGroup func for backgrounding (optimization). func distFile(ctx Context, config Config, src string, subDirs ...string) { if !config.Dist() { return @@ -362,7 +379,11 @@ func distFile(ctx Context, config Config, src string, subDirs ...string) { ctx.Printf("failed to mkdir %s: %s", destDir, err.Error()) } - if _, err := copyFile(src, filepath.Join(destDir, filepath.Base(src))); err != nil { - ctx.Printf("failed to dist %s: %s", filepath.Base(src), err.Error()) - } + distWaitGroup.Add(1) + go func() { + defer distWaitGroup.Done() + if _, err := copyFile(src, filepath.Join(destDir, filepath.Base(src))); err != nil { + ctx.Printf("failed to dist %s: %s", filepath.Base(src), err.Error()) + } + }() }