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

CI: Speeding up jobs #296

Merged
merged 14 commits into from
Nov 16, 2023
Merged

CI: Speeding up jobs #296

merged 14 commits into from
Nov 16, 2023

Conversation

marcospereira
Copy link
Contributor

@marcospereira marcospereira commented Nov 8, 2023

What?

A few changes to speed up the CI execution:

  • Use actions/cache to cache Gradle and Maven dependencies: jte is relatively lightweight regarding transitive dependencies, but we can still optimize it here. For example, there are Maven/Gradle plugins that can be cached.
  • Use maven-build-cache-extension to eliminate Maven task executions when possible. This is an opt-in feature that will only affect CI at the moment. This was removed from the scope of this PR.
  • Skipping tests when installing snapshots for tests: it was done for one of the workflows but was missing on the other one.. This is where the tests were run, so they should be kept.

Required changes

  • Maven Wrapper was updated to version 3.9.5 (latest), which is required by maven-build-cache-extension, which is a good update anyway.
  • Re-add/update Windows runner to windows-2022: it was not part of the "Test native builds" workflow because of a bug that is now fixed.

Results

Here is a run with no cache: https://github.com/marcospereira/jte/actions/runs/6803428787/job/18498736885. The "Build with Maven" step took almost 3 minutes.

Here is the same commit, but running with a cache: https://github.com/marcospereira/jte/actions/runs/6803428787/job/18499856271. The "Build with Maven" step took 11 seconds.

This is not the most realistic scenario because there will be code changes preventing the build cache from being fully reutilized, but it shows the possible gains.

Decisions

  • I did not use actions/setup-java caching configuration because it needs to cover the workflows here better.. We can now use setup-java/setup-graalvm cache features.
  • The caches are not Cross OS because they are not that large, and GitHub has its way of evicting cache entries.

Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (827e4e7) 91.15% compared to head (aa33348) 91.15%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #296   +/-   ##
=========================================
  Coverage     91.15%   91.15%           
  Complexity     1196     1196           
=========================================
  Files            76       76           
  Lines          3132     3132           
  Branches        484      484           
=========================================
  Hits           2855     2855           
  Misses          168      168           
  Partials        109      109           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@casid
Copy link
Owner

casid commented Nov 10, 2023

@marcospereira thanks!

Caching Gradle and Maven dependencies is fantastic, this alone should speed up CI quite a bit.

However, I'm very sceptical regarding maven-build-cache-extension. Caching is a hard problem and I'm wondering if maven is really always able to do the correct thing here (for instance, when we change CI parameters that maven is not aware of). I'd feel better if we would always do a dead simple clean build.

The main problem of slow builds are the ridiculously slow Kotlin compile times, but I haven't found a way to speed those up so far.

@marcospereira
Copy link
Contributor Author

However, I'm very sceptical regarding maven-build-cache-extension. Caching is a hard problem and I'm wondering if maven is really always able to do the correct thing here (for instance, when we change CI parameters that maven is not aware of). I'd feel better if we would always do a dead simple clean build.

I understand the sentiment, and that is why I made it an opt-in that only runs in the CI, without interfering in local env unless the cache is manually enabled. But I'm leaning towards its benefits are bigger than the risks.

Also, I cannot think of a good example of CI configuration requiring a cache eviction. One thing we can do, though, is to have the workflow YAML files being part of the cache key (including them in the hashFiles here) so that there will be a cache miss when those files change.

Makes sense?

@marcospereira
Copy link
Contributor Author

The main problem of slow builds are the ridiculously slow Kotlin compile times, but I haven't found a way to speed those up so far.

I also noticed that and plan to investigate it when I have the spare cycles. I suspect using Kotlin daemon (not currently used by jte) would be helpful not only for tests but for developer experience too.

@casid
Copy link
Owner

casid commented Nov 15, 2023

I understand the sentiment, and that is why I made it an opt-in that only runs in the CI

IMHO, ideally CI should always be the source of truth, thus it should always create a fresh, clean build if possible.

I suspect using Kotlin daemon (not currently used by jte) would be helpful not only for tests but for developer experience too.

I don't think this will help. During development (aka a server is running), jte + the Kotlin compiler used by jte are part of the already running server process. I think it won't get any faster than passing the templates to compile directly to the embedded Kotlin compiler. The problem isn't that we don't use a daemon. If I compare with Java compile times, which does not use a daemon and is almost an order of magnitude faster.

@marcospereira
Copy link
Contributor Author

@casid, this is now rebased on top of main branch, and I removed the maven build cache. The pr description is also updated with such changes.

@casid casid merged commit 1fe8790 into casid:main Nov 16, 2023
8 checks passed
@casid
Copy link
Owner

casid commented Nov 16, 2023

Thank you so much @marcospereira!

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.

2 participants