Skip to content

ci: reduce build time #3519

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

Merged
merged 40 commits into from
Oct 23, 2023
Merged

ci: reduce build time #3519

merged 40 commits into from
Oct 23, 2023

Conversation

zoli
Copy link
Contributor

@zoli zoli commented Sep 24, 2023

This started to reduce Appflowy desktop app build time but in the middle, It became more about enhancements on CI. I have some ideas about reducing user build time but first, it should be decided what is the best option overall.

Changes I made

Common

Flutter build

Building "lib_super_native_extension.so" static library which is used for "super_clipboard" dependency took noticeably time during the flutter build. In the new version, there is an option available for downloading a precompiled static library. I updated to new version and added "cargokit_options.yaml" file to make downloading precompiled libraries as default option.
Time-saving: this saves about 1:20

Code generation

First I removed generate_env script because it was doing the same as generate freezed script (am I right?).
Second in bash scripts, I made a flag available for not running flutter packages pub get each time. Only run it once before running "generate.sh". For "generate.cmd" I didn't know how to make a flag available so I just commented out the flutter packages pub get.
Time-saving: this saves about 1:50

Flutter CI

The changes I'll mention made it possible to merge the whole flutter ci into one workflow. And run flutter build, unit tests, integration tests job parallel.

Cache rust build and code generation

Created an artifact from libdart_ffi and code generations so to eliminate the need for running "appflowy-core-dev" and code generation task for each of flutter unit tests and integration tests.
I added "dart_unit_test_no_build" task to make it possible just copy the built dart ffi static library to ".sandbox" directory which is used for unit tests.
Note, I couldn't do this for mac os unit tests because somehow it wasn't possible to make macos use ffi ".a" file it had to be ".dylib" file. So for macos unit tests, cargo make runs two times.
Also added "appflowy-make-product-dev" task to enable running the flutter build part separate from the "appflowy-core-dev" and code generation task.

Use prebuilt dockscript binary

Used "taiki-e/install-action" github action to install duckscipt_cli and avoid compiling it each time.
Time-saving: this saves about 2:20 for each job.

Docker CI

Use precompiled rocksdb, zstd static libraries

I will discuss this in more details later. But as Appflowy dockerfile is based on Arch Linux image. I changed it so it wouldn't compile librocksdb, libzstd. Couldn't make it to use ssl precompiled static library because we directly ask it to use vendored one as a cargo feature.
Time-saving: docker ci workflow is about 19 minutes faster now

What are other options to reduce build time, especially for first-time users?

Building from scratch still takes a lot of time. Just take a look Docker CI workflow it takes about 43 minutes on average and 35 minutes of it is "appflowy-linux" task. I think the dev version even takes a bit more.
Here is how much time each of "appflowy-linux" sub tasks takes (these are before my changes):

  1. 24 minutes "appflowy-core-release" which is cargo build
  2. 7.5 minutes "code_generation"
  3. 3 minutes flutter build

Cargo build

If you run cargo build with timing flag (cargo build —timings) you will see that about 65 percent (14-15 minutes) of the 24 minutes is spend on building librocksdb, libzstd, libssl. Providing them as pre-compiled libraries in the repo or in a web server and downloading them would do the job. Or at least putting it in documentation like here which has done it. They are available as package in Arch Linux and probably Arch based distros. But the version on Ubuntu packages doesn't match what Appflowy requires (Openssl 3.1.2, ZSTD 1.5.5, Rocksdb 8.1.1). I don't know how is the case for Windows and MacOS.
Nathan suggested creating a precompiled library for collab-* which also Rocksdb and ZSTD are from there. Cargo doesn't support precompiled dependencies as far as I know (look here). I think the only available option for this is creating a static library and maintaining some c code to define specified functionalities.

Code generation

Removing generate_env script and redundant flutter packages pub get saved about 2 minutes. But I suggest another option for better performance.
Why not commit generated freezed files? Also ".g.dart" files. This saves another 3-4 minutes. And in CI we can check if there is a not committed change in generated files. We just need to run "code_generation" task and check it with git status that the code does not get dirty. So if the CI passes we will be sure the generated files are rightly generated from the latest changes.

Flutter build

I don't see any other enhancement than I made for using the precompiled library for super_clipboard dependency which reduced the time by 1:20.

PR Checklist

  • My code adheres to the AppFlowy Style Guide
  • I've listed at least one issue that this PR fixes in the description above.
  • I've added a test(s) to validate changes in this PR, or this PR only contains semantic changes.
  • All existing tests are passing.

@zoli zoli temporarily deployed to SUPABASE_CI September 24, 2023 16:03 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Sep 24, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8e5b6b6) 64.64% compared to head (3c1d244) 64.99%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3519      +/-   ##
==========================================
+ Coverage   64.64%   64.99%   +0.35%     
==========================================
  Files         542      568      +26     
  Lines       25171    25995     +824     
==========================================
+ Hits        16271    16896     +625     
- Misses       8900     9099     +199     
Flag Coverage Δ
appflowy_flutter_integrateion_test 63.01% <ø> (-1.63%) ⬇️
appflowy_flutter_unit_test 11.71% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 76 files with indirect coverage changes

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

@zoli
Copy link
Contributor Author

zoli commented Sep 25, 2023

Just wanted to note this is not review-ready yet. I didn't make it as a draft because I wanted to see how it affects github actions.
It is an attempt to reduce build time for developers and the overall CI pipeline time in github actions.

@zoli zoli temporarily deployed to SUPABASE_CI September 25, 2023 19:24 — with GitHub Actions Inactive
Trying to reduce the build time with providing static libraries
for rocksdb zstd and openssl.
cargo build timings showed that these are the most time consuming
for cargo build cmd.
@zoli zoli force-pushed the ci/reduce-build-time branch from dc39683 to 51c9c6d Compare September 26, 2023 07:56
@zoli zoli temporarily deployed to SUPABASE_CI September 26, 2023 07:56 — with GitHub Actions Inactive
@zoli zoli marked this pull request as draft October 2, 2023 21:25
@zoli zoli marked this pull request as ready for review October 7, 2023 07:27
@zoli
Copy link
Contributor Author

zoli commented Oct 7, 2023

@LucasXu0, @richardshiue Kindly wanted to ask for a review on this and your opinion on the options I suggested for lowering user build time.

@zoli zoli changed the title ci: reduce rust-lib build time ci: reduce build time Oct 7, 2023
@richardshiue
Copy link
Collaborator

Wow, very impressive

@LucasXu0 LucasXu0 temporarily deployed to SUPABASE_CI October 13, 2023 02:19 — with GitHub Actions Inactive
@LucasXu0
Copy link
Collaborator

@zoli Really appreciate that. I retriggered the CI jobs to check the reduced time.

@richardshiue
Copy link
Collaborator

Note, I couldn't do this for mac os unit tests because somehow it wasn't possible to make macos use ffi ".a" file it had to be ".dylib" file. So for macos unit tests, cargo make runs two times.

Oh.. that's a bottleneck then

@rileyhawk1417
Copy link
Collaborator

I wonder would this improvement help with local builds or its just mainly for the CI?

@zoli
Copy link
Contributor Author

zoli commented Oct 14, 2023

I wonder would this improvement help with local builds or its just mainly for the CI?

Not that much on the local build only 3 minutes. Which are saved because of code generation enhancement and using a prebuilt super clipboard library. I've suggested some options for reducing local build in the PR.

@zoli
Copy link
Contributor Author

zoli commented Oct 14, 2023

Note, I couldn't do this for mac os unit tests because somehow it wasn't possible to make macos use ffi ".a" file it had to be ".dylib" file. So for macos unit tests, cargo make runs two times.

Oh.. that's a bottleneck then

Yes, Its possible to separate it in another workflow. But I guess maybe there is a way to use .a library without problem I think on running appflowy binary Mac OS uses .a library not the dylib. I don't use Mac OS and haven't tried to check it on sort of a VM yet.

@zoli
Copy link
Contributor Author

zoli commented Oct 14, 2023

@zoli Really appreciate that. I retriggered the CI jobs to check the reduced time.

The Flutter-CI workflow is worse and that's because of caching I think. So it's hard to make any comparison. The prepare job in my fork repo didn't take more than 20m on average but here it took 47m and the difference is in "appflowy-core-dev" task (cargo build) which is affected by rust cache.
The Docker-CI workflow is good for making comparisons because it doesn't use any sort of cache. It takes 24m on this branch but the average on main is 40m.

@zoli
Copy link
Contributor Author

zoli commented Oct 14, 2023

Also, any comments on the options I suggested that enhance local build time (committing freezer files, providing precompiled rocksdb, ...)?

@LucasXu0
Copy link
Collaborator

Also, any comments on the options I suggested that enhance local build time (committing freezer files, providing precompiled rocksdb, ...)?

I agree with the second option, which involves providing a precompiled RocksDB. And the frozen files may sometimes conflict because we're using different Flutter versions(different linter).

@LucasXu0 LucasXu0 temporarily deployed to SUPABASE_CI October 23, 2023 03:43 — with GitHub Actions Inactive
@LucasXu0 LucasXu0 force-pushed the ci/reduce-build-time branch from 4115bf6 to 96bb0b4 Compare October 23, 2023 05:27
@LucasXu0 LucasXu0 temporarily deployed to SUPABASE_CI October 23, 2023 05:27 — with GitHub Actions Inactive
@LucasXu0
Copy link
Collaborator

Before
integration test, linux, total 38m3s
Screenshot 2023-10-23 at 14 27 11

After
integration test, linux, prepare 13m4s, test 26m29s, total 39m33s
Screenshot 2023-10-23 at 14 28 26
Screenshot 2023-10-23 at 14 28 17

Actually, for the longest time-consuming tasks, this PR doesn't optimize them; it only reduces the duplicated steps. @zoli, right?

@LucasXu0 LucasXu0 temporarily deployed to SUPABASE_CI October 23, 2023 09:22 — with GitHub Actions Inactive
@LucasXu0 LucasXu0 force-pushed the ci/reduce-build-time branch from 1d47d53 to 3c1d244 Compare October 23, 2023 10:38
@LucasXu0 LucasXu0 temporarily deployed to SUPABASE_CI October 23, 2023 10:38 — with GitHub Actions Inactive
@LucasXu0 LucasXu0 merged commit 25a98cd into AppFlowy-IO:main Oct 23, 2023
@zoli
Copy link
Contributor Author

zoli commented Oct 24, 2023

Actually, for the longest time-consuming tasks, this PR doesn't optimize them; it only reduces the duplicated steps. @zoli, right?

In theory, I expect 3-4 minutes of saving on integration tests (because of removing generate_env script and precompiled super_clipboard static library). But as far as I see it's not showing itself. The comparison should be on average of multiple workflows. A commit that changes the rust code versus the dart code can have a notable difference in workflow time because of the rust cache.
Overall the flutter_ci is more about integrating all in a single workflow and removing repetitive tasks.

I agree with the second option, which involves providing a precompiled RocksDB.

I will work on this.

Also, any IDEA how the flutter integration tests can get faster?

@LucasXu0
Copy link
Collaborator

LucasXu0 commented Oct 25, 2023

Also, any IDEA how the flutter integration tests can get faster?

Not yet.

@zoli I believe it would be beneficial to segregate the tests across different platforms. Although the unit tests, integration tests, and build tasks all depend on the 'prepare' task, the time required for the 'prepare' tasks varies across different platforms. Consequently, other tasks are held up by the longest time-consuming 'prepare' task.

For instance, in the 'prepare' task, Linux takes 10 minutes, macOS takes 15 minutes, and Windows takes 20 minutes. Consequently, the remaining tasks can only be executed after the Windows task has been completed.

As a solution, I suggest that the other macOS tasks should only be dependent on the completion of the macOS 'prepare' task, and likewise for other platforms.

@zoli
Copy link
Contributor Author

zoli commented Oct 25, 2023

As a solution, I suggest that the other macOS tasks should only be dependent on the completion of the macOS 'prepare' task, and likewise for other platforms.

@LucasXu0 I was wary of creating a separate workflow for each platform but your solution is better. Creating separate jobs (e.g. prepare-macos) for each OS and just depending on that in the specific OS next job (e.g. integration-test-macos). I will work on this too.

Also using a docker image with pre-installed flutter and rust will enhance Windows CI time by about 3-4 minutes.

@zoli
Copy link
Contributor Author

zoli commented Nov 2, 2023

Hi @LucasXu0. While working on providing static libraries, I was wondering if the flowy_codegen result (dart, protobuf, ts) also depends on user OS or STH like that?

@zoli zoli deleted the ci/reduce-build-time branch November 6, 2023 19:45
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.

4 participants