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

Parallelize local base image compression #2022

Merged
merged 2 commits into from
Sep 25, 2019
Merged

Conversation

TadCordle
Copy link
Contributor

@TadCordle TadCordle commented Sep 25, 2019

Towards #1913. This only really makes a difference for un-cached local base image layers, but it provides a decent speedup in that case. Here were the build times using maven + a clean cache on the master branch vs. these changes:

docker://gcr.io/distroless/java

  • master: 7.6 seconds
  • this PR: 6.8 seconds

docker://openjdk:8

  • master: 29.2 seconds
  • this PR: 16.4 seconds

progressEventDispatcher.newChildProducer();

cachedLayers.add(
executorService.submit(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, the code here won't deal with concurrency and StepsRunner should be able to maximize concurrency by individually compressing and caching each layer. For example, right now it is possible that some registry base image layers are being pulled in while some base layers can be pushed simultaneously. (I am sure constructing a base image manifest can be delayed, since I believe the reason we need them at an early stage is to be able to download the layers from a registry.) Also this would be nice for the consistent async framework design where each step doesn't get an ExecutorService.

But I think this is OK for short-term simple improvement for a low-hanging fruit. We should continue to track for the ideal async handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree this should be iterated on to be more consistent/less self-contained. But I figured it'd be nice to at least get the small performance gain in without adding too much complexity at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if we change the build steps more it'd be nice to keep the second bullet in #1913 in mind.

Instead of computing the layer digest and then pushing to a registry, push base image layers while compressing on-the-fly (#1906 (comment))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm already getting a headache over all these. Sounds like a fun challenge!

progressEventDispatcher.newChildProducer();

cachedLayers.add(
executorService.submit(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm already getting a headache over all these. Sounds like a fun challenge!

@TadCordle TadCordle merged commit 930d911 into master Sep 25, 2019
@TadCordle TadCordle deleted the concurrent-local-layers branch September 25, 2019 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants