-
Notifications
You must be signed in to change notification settings - Fork 405
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
Build each platform concurrently #527
Conversation
At least on my machine, this is slightly faster on a cold build and saves significant time on a warm build.
Codecov Report
@@ Coverage Diff @@
## main #527 +/- ##
==========================================
+ Coverage 48.11% 48.16% +0.05%
==========================================
Files 42 42
Lines 2170 2182 +12
==========================================
+ Hits 1044 1051 +7
- Misses 950 954 +4
- Partials 176 177 +1
Continue to review full report at Codecov.
|
@@ -872,9 +873,12 @@ func (g *gobuild) buildAll(ctx context.Context, ref string, baseIndex v1.ImageIn | |||
return nil, err | |||
} | |||
|
|||
errg, ctx := errgroup.WithContext(ctx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want this to share it's threadpool with the --jobs
pool, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plumbed this down into buildOne
if bo.ConcurrentBuilds == 0 { | ||
bo.ConcurrentBuilds = runtime.GOMAXPROCS(0) | ||
} | ||
innerBuilder = build.NewLimiter(innerBuilder, bo.ConcurrentBuilds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this now dead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, but build.NewLimiter
is public so I'm somewhat reluctant to just delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we mark it as Deprecated
at least, and delete it in the future? If someone depends on it (highly unlikely) and it's not actively used by the codebase that's not a great place to be in either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -123,6 +131,7 @@ func (gbo *gobuildOpener) Open() (Interface, error) { | |||
buildToDiff: map[string]buildIDToDiffID{}, | |||
diffToDesc: map[string]diffIDToDescriptor{}, | |||
}, | |||
semaphore: semaphore.NewWeighted(int64(gbo.jobs)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, do we Open()
once per ko
invocation or once per build? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's once per "builder", which means the --jobs
isn't shared across build configs... which is annoying, but I'm happy to fix that if someone actually complains...
At least on my machine, this is slightly faster on a cold build and
saves significant time on a warm build.
Fixes #192