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

Run conan install in parallel #363

Merged
merged 6 commits into from
Jul 26, 2024
Merged

Run conan install in parallel #363

merged 6 commits into from
Jul 26, 2024

Conversation

ViliusSutkus89
Copy link
Contributor

I've changed the conan install task.
Moved it into a Java Task class with inputs and output, so it could be reused for different architectures.
Having separate instances of conan install tasks for each architecture allows running them in parallel.

Did a bit of benchmarking, without this PR ./gradlew clean; time ./gradlew assembleDebug usually takes 30s for me, best case I ever saw was 23s. With this PR, the same build for me takes 20 to 15s, sometimes as low as 10s.
These numbers are with all conan packages available in cache or artifactory. In cases where conan packages are actually being build from source, this PR would give even more speed-up, since a lot of time is spent on single threaded CMake execution.

Enabling parallel execution requires enabling gradle's configuration-cache. It should work without errors, but if we do get errors, we can always disable it in gradle.properties and return to regular serial task execution.

Gradle's configuration-cache requires that tasks don't reference project variables, so I've had to update conanProfile and .cxx cleaner task. Also, upgraded Gradle wrapper to 8.9, for good measure, since this is very related.

@ViliusSutkus89
Copy link
Contributor Author

Build failure, lol

@ViliusSutkus89
Copy link
Contributor Author

Conan install doesn't like being called in parallel with an empty conan cache. Made sure to call conan install armv8 before calling conan install on all the other arches. Should be good now

Copy link
Member

@TomTasche TomTasche left a comment

Choose a reason for hiding this comment

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

Nice idea! Works for me locally, so good to go in my opinion. @andiwand thoughts?

Crazy idea: is it possible to turn that into a separate plugin that other projects could make use of without much setup? That would be a nice project for the opendocument-organization!

@ViliusSutkus89
Copy link
Contributor Author

Yes. ConanAndroid Gradle plugin would be way cooler than just keeping this class in build.gradle of all our projects. I should look into it

@TomTasche TomTasche requested a review from andiwand July 23, 2024 07:30
"-s", "odrcore/*:build_type=RelWithDebInfo",
)
}
["armv8", "armv7", "x86", "x86_64"].each { architecture ->
Copy link
Member

Choose a reason for hiding this comment

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

should we put this in a list somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Architecture list? Well, it is going to change sometime in the future, once we drop armv7 support, but I doubt that's anytime soon

Copy link
Member

Choose a reason for hiding this comment

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

I just saw that we have the same list twice so we might put this in a variable or somewhere. But nothing crucial

Copy link
Contributor Author

@ViliusSutkus89 ViliusSutkus89 Jul 26, 2024

Choose a reason for hiding this comment

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

True, arches are defined in two places. However, once our plugin is published, most of this will be removed, so I'll just keep this as is and merge, because we don't know how soon it will be published

@ViliusSutkus89 ViliusSutkus89 merged commit 014b84a into main Jul 26, 2024
5 checks passed
@ViliusSutkus89 ViliusSutkus89 deleted the conanInstallParallel branch July 26, 2024 18:18
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