Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Ignoring unnecessarily generated docker-ready #510

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

Open
optimizing-ci-builds wants to merge 1 commit into obsidiandynamics:master
base: master
Choose a base branch
Loading
from optimizing-ci-builds:optimization

Conversation

Copy link

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

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.

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?

Copy link
Author

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

Copy link
Collaborator

Bert-R commented May 1, 2023

@davideicardi Your thoughts?

Copy link
Collaborator

davideicardi commented May 2, 2023
edited
Loading

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 😁

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.

Copy link
Collaborator

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

Copy link
Author

optimizing-ci-builds commented Aug 16, 2023
edited
Loading

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /