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

Ignoring unnecessarily generated docker-ready #510

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

optimizing-ci-builds
Copy link

@optimizing-ci-builds optimizing-ci-builds commented Apr 18, 2023

In our analysis, we observe that target/docker-ready is generated but not used in CI. Because the contents of docker-ready/ are not accessed after they are generation, we propose to disable the generation of this directory to save build runtime. The generation of this directory can be disabled by simply adding a property element to the pom.xml or adding an argument to the mvn command. The argument to add to the mvn command is -Dskip.docker.resources=true.

@Bert-R
Copy link
Collaborator

Bert-R commented Apr 20, 2023

Though it's true that this folder is not being used during the verification, I don't see any practical benefit in adding this condition, as the performance overhead of copying these two tiny files is negligible.

What gain are you expecting?

@optimizing-ci-builds
Copy link
Author

In our investigation, we found that with our changes it can make 14% time savings than the original run.

@Bert-R
Copy link
Collaborator

Bert-R commented May 1, 2023

@davideicardi Your thoughts?

@davideicardi
Copy link
Collaborator

davideicardi commented May 2, 2023

No strong opinion on my side, but 14% time savings is quite good.
If there aren't drawbacks why not! We will save some CO2 😁

@Bert-R
Copy link
Collaborator

Bert-R commented May 2, 2023

@davideicardi Would you mind testing it for yourself? Just checkout this PR and build with -Dskip.docker.resources=true and -Dskip.docker.resources=false and compare the times. I didn't notice any difference, so then I felt extra complexity is not justified.

@davideicardi
Copy link
Collaborator

@Bert-R You are right, I don't see any relevant difference. @optimizing-ci-builds How do you have misured this?

@optimizing-ci-builds
Copy link
Author

optimizing-ci-builds commented Aug 16, 2023

Here are two workflow where we executed this project with and without the PR changes:

With proposed change: GitHub Action Run Link
Without proposed change: GitHub Action Run Link

The with proposed change build took 3m 25s while the without proposed change build took 4m 7s -- representing an approximately 40-second reduction in overall build time.

The with proposed change log also shows the "Build with Maven" step took 1m 4s. While the without proposed change log shows the "Build with Maven" step took 1m 17s. This difference represents a 17% (13 seconds) runtime improvement with the proposed changes.

Although there may be some noise in the runtime measurement of each build, we believe the proposed change can help lower the cost of each build most of the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants