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

Build performance improvements #3926

Merged
merged 3 commits into from
Jul 18, 2022
Merged

Build performance improvements #3926

merged 3 commits into from
Jul 18, 2022

Conversation

cdsap
Copy link
Contributor

@cdsap cdsap commented Jul 16, 2022

Description

This Pull Request proposes small improvements to the build script logic to improve performance and cache hit ratio in the project.

Issues Resolved

Build improvements

Compilation avoidance

The configuration of the task buildNoJdkRpm is:

tasks.register('buildNoJdkRpm', Rpm) {
  configure(commonRpmConfig(true, 'x64'))
}

The current configuration was setting the incorrect parameter jdk to true. This configuration was causing in incremental build scenarios the execution of the tasks. After setting the correct parameter tasks are UP-TO-DATE.
Results:
Main branch: https://scans.gradle.com/s/2wvwfoybovs3e
This PR: https://scans.gradle.com/s/jdjx4vxwgqy4w

MissingJavadoc cacheable

Caching was not enabled for the task. We are updating the task definition with:

@CacheableTask
class MissingJavadocTask extends DefaultTask {

Additionally, we are avoiding the potential overlapping outputs issue setting the output of the task in a different folder.
Results:
Main branch: https://scans.gradle.com/s/lpchzshhoe5iy/performance/build-cache
This PR: https://scans.gradle.com/s/x65oz7h3l7xks/performance/build-cache

Javadoc output reusable in different environments

When we are building from different environments(CI, different local paths), javadoc tasks were not cacheable. This was caused by the usage of the absolute path in the property javadoc.options.linksOffline:

project.javadoc.options.linksOffline artifactsHost + "/javadoc/" + artifactPath, "${upstreamProject.buildDir}/docs/javadoc/"

This PR updates the configuration to use a relative path in the property.
Results:
Main: https://scans.gradle.com/s/mpbt2xbmg3ety/performance/build-cache
This PR: https://scans.gradle.com/s/vkxooiwndw3sq/performance/build-cache

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

cdsap added 3 commits July 15, 2022 17:23
Signed-off-by: Inaki Villar <inaki.seri@gmail.com>
Signed-off-by: Inaki Villar <inaki.seri@gmail.com>
Signed-off-by: Inaki Villar <inaki.seri@gmail.com>
@cdsap cdsap marked this pull request as ready for review July 16, 2022 00:28
@cdsap cdsap requested review from a team and reta as code owners July 16, 2022 00:28
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #3926 (ce50086) into main (e724b2e) will increase coverage by 0.15%.
The diff coverage is 50.98%.

@@             Coverage Diff              @@
##               main    #3926      +/-   ##
============================================
+ Coverage     70.46%   70.62%   +0.15%     
- Complexity    56600    56684      +84     
============================================
  Files          4557     4563       +6     
  Lines        272737   272755      +18     
  Branches      40040    40040              
============================================
+ Hits         192188   192625     +437     
+ Misses        64324    63869     -455     
- Partials      16225    16261      +36     
Impacted Files Coverage Δ
...n/cluster/health/TransportClusterHealthAction.java 47.75% <ø> (ø)
...g/opensearch/cluster/ClusterStateTaskListener.java 100.00% <ø> (ø)
.../opensearch/cluster/MasterNodeChangePredicate.java 0.00% <0.00%> (ø)
...ava/org/opensearch/cluster/NotMasterException.java 0.00% <0.00%> (ø)
...rch/cluster/coordination/NoMasterBlockService.java 0.00% <0.00%> (ø)
...ter/coordination/UnsafeBootstrapMasterCommand.java 0.00% <0.00%> (ø)
...rg/opensearch/common/settings/ClusterSettings.java 91.89% <ø> (ø)
...rch/common/settings/ConsistentSettingsService.java 67.85% <ø> (ø)
.../opensearch/common/util/concurrent/BaseFuture.java 62.71% <0.00%> (ø)
...java/org/opensearch/discovery/DiscoveryModule.java 90.27% <ø> (ø)
... and 484 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e96a87...ce50086. Read the comment docs.

@@ -392,7 +392,7 @@ tasks.register('buildRpm', Rpm) {
}

tasks.register('buildNoJdkRpm', Rpm) {
configure(commonRpmConfig(true, 'x64'))
configure(commonRpmConfig(false, 'x64'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -183,6 +183,7 @@ configure(project(":server")) {
}
}

@CacheableTask
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@dblock
Copy link
Member

dblock commented Jul 18, 2022

Cool, all makes sense. Any idea on what kind of practical performance improvement we're seeing with these and in what scenario?

@dblock dblock merged commit cc10b97 into opensearch-project:main Jul 18, 2022
@dblock dblock added the backport 2.x Backport to 2.x branch label Jul 18, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 18, 2022
* set appropiate jdk boolean for buildNoJdkRpm

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>

* make missingJavadoc cacheable

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>

* using relativepath for javadoc.options.linksOffline

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>
(cherry picked from commit cc10b97)
@cdsap
Copy link
Contributor Author

cdsap commented Jul 19, 2022

hi @dblock, the immediate performance improvement happens on the compile avoidance scenario. When we are executing incremental builds the tasks buildNoJdkRpm and buildRpm are not longer executed if the are no changes on the inputs. Until now, those tasks were always executed. In my tests, without applying changes both tasks were taking around 1 minute(using M1).

dblock pushed a commit that referenced this pull request Jul 21, 2022
* set appropiate jdk boolean for buildNoJdkRpm

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>

* make missingJavadoc cacheable

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>

* using relativepath for javadoc.options.linksOffline

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>
(cherry picked from commit cc10b97)
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
dreamer-89 pushed a commit that referenced this pull request Jul 21, 2022
* set appropiate jdk boolean for buildNoJdkRpm

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>

* make missingJavadoc cacheable

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>

* using relativepath for javadoc.options.linksOffline

Signed-off-by: Inaki Villar <inaki.seri@gmail.com>
(cherry picked from commit cc10b97)
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>

Co-authored-by: Iñaki Villar <inaki.seri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants