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.

Sign up for GitHub

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

Jump to bottom

Bug 1788606 - Speed up decision task by not calling gradle anymore #340

Merged
merged 7 commits into from Dec 15, 2022
Merged

Bug 1788606 - Speed up decision task by not calling gradle anymore #340

merged 7 commits into from Dec 15, 2022

Conversation

JohanLorenzo
Copy link
Collaborator

@JohanLorenzo JohanLorenzo commented Dec 15, 2022 •
edited

#210 didn't pay off. Many people and I have seen long decision tasks (> 10 minutes) since focus got migrated over. Let's stop calling calling gradle by caching its output to .buildconfig.yml and enforcing them to match with lint tasks.

The decision task is now back to 1 minute!

Example of a failed lint task[1]:

[task 2022-12-15T15:05:45.991Z] 2022-12-15 15:05:45,991 - ERROR - .buildconfig.yml file changed! Please update it by running:
[task 2022-12-15T15:05:45.991Z] 
[task 2022-12-15T15:05:45.991Z] curl --location https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/RWqWjr05R7yttsVi6dOgkQ/artifacts/public%2Fgit%2Fbuildconfig.diff | gunzip | git apply
[task 2022-12-15T15:05:45.991Z] 
[task 2022-12-15T15:05:45.991Z] Then commit and push!

Example of a green one[2]:

[task 2022-12-15T14:46:38.827Z] 2022-12-15 14:46:38,827 - INFO - Updated android-components/.buildconfig.yml with latest gradle config!
[task 2022-12-15T14:46:38.961Z] 2022-12-15 14:46:38,960 - INFO - All good! .buildconfig.yml is up-to-date with gradle.

[1] https://firefox-ci-tc.services.mozilla.com/tasks/RWqWjr05R7yttsVi6dOgkQ/runs/0/logs/live/public/logs/live.log#L71
[2] https://firefox-ci-tc.services.mozilla.com/tasks/HuQBBdnvTC6zVs_axj6emQ/runs/0/logs/live/public/logs/live.log

@JohanLorenzo JohanLorenzo changed the title Bug 1788606 - Stop calling gradle in the decision task Bug 1788606 - Speed up decision task by not calling gradle anymore Dec 15, 2022
@JohanLorenzo JohanLorenzo force-pushed the bug-1788606-2 branch 4 times, most recently from da7e6db to 4788351 Compare December 15, 2022 14:29
@JohanLorenzo JohanLorenzo force-pushed the bug-1788606-2 branch 4 times, most recently from da2633d to c57b168 Compare December 15, 2022 14:59
@JohanLorenzo JohanLorenzo marked this pull request as ready for review December 15, 2022 15:09
@JohanLorenzo
Copy link
Collaborator Author

@Mergifyio backport releases_v109 releases_v108

@mergify
Copy link
Contributor

mergify bot commented Dec 15, 2022 •
edited

backport releases_v109 releases_v108

✅ Backports have been created

@JohanLorenzo JohanLorenzo self-assigned this Dec 15, 2022
@JohanLorenzo JohanLorenzo mentioned this pull request Dec 15, 2022
Merged
jcristau
jcristau reviewed Dec 15, 2022
View reviewed changes
Copy link
Contributor

@jcristau jcristau left a comment

Choose a reason for hiding this comment

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

LGTM if it works for the mobile team.

A couple of comments, possibly for followups:

  • consider splitting out the generated bits to their own file so we don't mix human and machine written config
  • consider adding an explanatory comment to the top of the file on how to update it

taskcluster/scripts/lint/is_buildconfig_yml_up_to_date.py
artifact_url = f"{tc_root_url}/api/queue/v1/task/{task_id}/artifacts/public%2Fgit%2F{BUILDCONFIG_DIFF_FILE_NAME}" # noqa E501
message = f"""{BUILDCONFIG_FILE_NAME} file changed! Please update it by running:

curl --location {artifact_url} | gunzip | git apply
Copy link
Contributor

Choose a reason for hiding this comment

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

Make that curl --location --compressed {artifact_url} | git apply to let curl handle the compression if necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe consider also having the generated .buildconfig.yml as an artifact in addition to the diff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TIL: curl --location --compressed. Done!

Maybe consider also having the generated .buildconfig.yml as an artifact in addition to the diff.

Good idea. I'll handle that in a followup.

gbrownmozilla
gbrownmozilla approved these changes Dec 15, 2022
View reviewed changes
Copy link
Collaborator

@gbrownmozilla gbrownmozilla left a comment

Choose a reason for hiding this comment

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

LGTM and I could not break it in local testing. I am very pleased with the massive improvement in decision task speed. I am a little concerned about the buildconfig maintenance issues, but I don't have a better idea; nothing comes for free!

@JohanLorenzo
Copy link
Collaborator Author

I am a little concerned about the buildconfig maintenance issues

Yeah, that's the reason why I had wanted to use a single source of truth for that long (namely gradle). That said, I have some good confidence the new lint tasks will help maintaining these files so we don't let them drift away from the gradle config

csadilek
csadilek approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

🔥 Amazing work, @JohanLorenzo! 🚢 it.

@JohanLorenzo JohanLorenzo merged commit 0ea1dfe into mozilla-mobile:main Dec 15, 2022
@JohanLorenzo JohanLorenzo deleted the bug-1788606-2 branch December 15, 2022 21:06
This was referenced Dec 15, 2022
Merged
Merged
@JohanLorenzo
Copy link
Collaborator Author

@Mergifyio backport releases_v108

@mergify
Copy link
Contributor

mergify bot commented Dec 16, 2022

backport releases_v108

✅ Backports have been created

@JohanLorenzo JohanLorenzo mentioned this pull request Sep 21, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabrielluong gabrielluong gabrielluong left review comments

@jcristau jcristau jcristau left review comments

@csadilek csadilek csadilek approved these changes

@gbrownmozilla gbrownmozilla gbrownmozilla approved these changes

Assignees

@JohanLorenzo JohanLorenzo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@JohanLorenzo @csadilek @gabrielluong @jcristau @gbrownmozilla