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

Improve build configuration time #41392

Merged
merged 44 commits into from May 24, 2019

Conversation

Projects
None yet
4 participants
@mark-vieira
Copy link
Contributor

commented Apr 19, 2019

This PR includes a number of refactorings to generally improve the Gradle build configuration time. The most significant is the migration of the global build info logic into a cacheable task that runs at execution time vs during configuration.

Additionally a handful of other inefficiencies were fixed and some commonly executed code was ported to static groovy.

Before:

real	0m7.928s
user	0m1.060s
sys	0m0.116s

After:

real	0m4.061s
user	0m1.097s
sys	0m0.121s

The big shift here is we've moved all the expensive global build info generation out of configuration time and into execution time. This is done by two tasks generateGlobalBuildInfo, a @CacheableTask that actual does all the expensive work, and the second printGlobalBuildInfo that consumes the output of the generate task and prints it to the console as well as sets the requisite global build properties.

Since some of these properties like java versions and fips compatibility are not known until runtime, we've also had to make some changes around how we access those properties. Where necessary we deferred this evaluation, either by using a lazy GString, Provider or other Callable or by utilizing the new globalInfo.ready { } callback. I hope to eventually make this all a bit cleaner as part of addressing #42042.

mark-vieira added some commits Apr 10, 2019

wip
Merge remote-tracking branch 'mark-vieira/global-info-refactor' into …
…global-info-refactor

# Conflicts:
#	buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
Merge remote-tracking branch 'origin/master' into global-info-refactor
# Conflicts:
#	buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
Merge remote-tracking branch 'origin/master' into global-info-refactor
# Conflicts:
#	buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
Merge remote-tracking branch 'origin/master' into global-info-refactor
# Conflicts:
#	buildSrc/build.gradle
#	buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy
#	x-pack/qa/rolling-upgrade/build.gradle
Merge remote-tracking branch 'origin/master' into global-info-refactor
# Conflicts:
#	buildSrc/src/main/groovy/org/elasticsearch/gradle/BuildPlugin.groovy
@mark-vieira

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

@elasticmachine run elasticsearch-ci/2

mark-vieira added some commits May 8, 2019

@mark-vieira

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@rjernst @atorok are we happy to merge this as-is? Want to avoid any more merge conflicts 😉

@rjernst

This comment has been minimized.

Copy link
Member

commented May 21, 2019

I'm still concerned about the ready block, since it is effectively a runtime block that runs after configuration, yet is often used to change configuration?

@mark-vieira

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Essentially, yes. FWIW, it looks like this may go away soon according to some upcoming work around FIPS. Essentially testing against FIPS will work via a build flag which will load the FIPS provider vs having to use a whole different JVM implementation. At that point we can ditch the runtime check, as we'll know at configuration time whether this is a FIPS build or not.

@rjernst

This comment has been minimized.

Copy link
Member

commented May 21, 2019

If we are going to push this as-is then, I would prefer a large and visible warning comment explaining the dangers of using ready, and how it should not be copy/pasted.

@mark-vieira

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Do we want to put this at every usage site?

@rjernst

This comment has been minimized.

Copy link
Member

commented May 21, 2019

I'm torn. It looks like there are 8 callsites, 7 of which are fips. What would be the longterm plan for the remaining usage (java12 check to set reindex setting)?

@mark-vieira

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

What would be the longterm plan for the remaining usage (java12 check to set reindex setting)?

When that gets converted to use test clusters we can use a lazy Supplier for that setting instead of a static string.

@atorok
Copy link
Contributor

left a comment

Only made it to the end of BuildPlugin, here's what I have so far

@@ -263,7 +264,7 @@ allprojects {
}

project.afterEvaluate {
configurations.all {
configurations.matching { it.canBeResolved }.all {

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

why wouldn't the config be necessary? Maybe worth adding a comment to explain.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

There's not point in doing substitutions on internal configurations that cannot be resolved. I think this is a bit moot anyway since all this is going to get ripped out as part of #42093.


targetCompatibility = '11'

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

We can't do this yet without breaking the hadoop plugin, but the right way to do it is to use the value red from resources for minimum runtime.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

Should this be 8 then? The value in minimumRuntimeVersion in master is 11.

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

yes it should be 8 until @jbaiera does the work to support newer

// wait until global info is populated because we don't know if we are running in a fips jvm until execution time
globalInfo.ready {
project.subprojects { Project subproject ->
ExtraPropertiesExtension ext = subproject.extensions.getByType(ExtraPropertiesExtension)

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

Why aren't we storing inFipsJvm in the GlobalInfoExtension ? Would be nice to stop using extra properties here.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

That is a potential solution #42042. In any case, ditching all these extra properties is to be addressed by that issue.

throw new GradleException("JAVA${version}_HOME required to run task:\n${task}")
}
} else {
rootProject.requiredJavaVersions.getOrDefault(version, []).add(task)
Map<Integer, List<Task>> requiredJavaVersions = (Map<Integer, List<Task>>) ext.get('requiredJavaVersions')

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

lazy variable, used only once, would be better in-lined.

result.rethrowFailure()
}
return stdout.toString('UTF-8').trim()
def javaVersions = task.project.property('javaVersions') as List<JavaHome>

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

let's use a specific type here

project.mkdir(testOutputDir)
project.mkdir(heapdumpDir)
project.mkdir(test.workingDir)

if (project.property('inFipsJvm')) {
nonInputProperties.systemProperty('runtime.java', "${-> (ext.get('runtimeJavaVersion') as JavaVersion).getMajorVersion()} FIPS")

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

extra space before FIPS

* to the GStrings containing references to non-serializable objects.
*
* We bypass this by instead passing this system properties vi a CommandLineArgumentProvider. This has the added
* side-effect that these properties are NOT treated as inputs, therefore they don't influence things like the

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

We don't want that here, that would mean that changing from one version of java to annother ( runtime.java ) would not cause the test to be re-run as these are not treated as input.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

The runtime Java version is tracked by the Test task via the bootstrapClasspath property, not a system property. This property is simply informative and used in the reproduction line. In theory I could set runtime.java to "99" and the tests would care less. What actually sets the runtime java version is below where we set test.executable.

mark-vieira added some commits May 22, 2019

return stdout.toString(UTF_8).trim();
}

@Input

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

nit: could we move all inputs and outputs up please, followed by task action and all private methods, I think that makes tasks much easier to read

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

Yep. This is habit from Gradle core conventions.

}

public void setMinimumCompilerVersion(JavaVersion minimumCompilerVersion) {
this.minimumCompilerVersion = minimumCompilerVersion;

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

how about reading this from resources here and not even provide a setter to make this self contained. I don't think we want users to configure this so it would be better to make it non configurable.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

That logic belongs in the plugin not the task. We really need to stop jumping through hoops on the premise of "but a user could...". It's Gradle. The entire project model is visible and mutable from every build script. There are an infinite number of things a user could break in a build script. Groovy makes this worse as member visibility is also mostly irrelevant.

In the end what would possess a user to grab this task and fiddle with it? This is what code reviews and tests are for. IMO, between absolute control of configuration and proper design and separation of concerns I choose the latter over the former.

}

@InputDirectory
@PathSensitive(PathSensitivity.RELATIVE)

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

I'm not sure this would work with relative for example we have ~/.java/java11 in CI. My understanding is that this would use java11 as a cache key, but patch level can change so in one point in time we might have 11.0.1 and then 11.0,.2 at that location.
I think up to date checks are fine, since something will surely change in the directory for sure, but not if something is pulled from the cache.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

This path sensitivity means that we don't care about the root directory name. Only the paths below that directory.

For example. Let's say my java home is ~/.java/java11.0.1 and there exists a file ~/.java/java11.0.1/bin/java. If that file were to move to ~/.java/java11.0.1/jre/bin/java that would be considered a change in the cache key. However, if I were to simply rename ~/.java/java11.0.1 to ~/.java/java no relative paths would change so no change in the cache key.

So in CI if ~/.java/java11 and ~/.java/java both point to the same place, they will both have identical cache keys since the actual root folder name is not considered.

Also, to clarify, the actual contents of this directory are also hashed so any change there would invalidate the cache entry.

task.setMinimumRuntimeVersion(minimumRuntimeVersion);
task.setCompilerJavaHome(compilerJavaHome);
task.setRuntimeJavaHome(runtimeJavaHome);
task.getOutputFile().set(new File(project.getBuildDir(), "global-build-info"));

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

these shouldn't really be configurable either.

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

See my above comment. We need to get out of this mindset of low-configurability and hard-coding everything. It makes reasoning about this code a pain in the ass because core logic is split between plugins and task implementations. Task implementations should be dumb. They should know nothing about the project or conventions. The plugin should apply conventions. In 99.9999% of cases no one will ever override this logic and the concerns that this will cause issues are grossly overstated. Not to mention that in cases where we do want to do this for whatever reason we then have to put hacky conditionals in our task implementation.

}

private static String findJavaHome(String version) {
String versionedVarName = getJavaHomeEnvVarName(version);

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

nit: lazy variable

JavaVersion runtimeJavaVersionEnum = JavaVersion.current();
File gradleJavaHome = Jvm.current().getJavaHome();
boolean inFipsJvm = false;

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

We should do a check on compilerJavaHome and runtimeJava home somewhere in here to make sure they exist so we don't only run into an error running jrunscript this has generated a lot of confusion in the past

thirdPartyAudit.ignoreMissingClasses (
'org.bouncycastle.asn1.x500.X500Name'
)
rootProject.globalInfo.ready {

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

we already skip third party audit on fips so might as well do this here and for others that would otherwise need ready. There's not much to gain from running it if we single out the differences between fips and non fips anyway.

// FIPS JVM includes many classes from bouncycastle which count as jar hell for the third party audit,
// rather than provide a long list of exclusions, disable the check on FIPS.
thirdPartyAudit.enabled = false
rootProject.globalInfo.ready {

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

This makes me wonder if it's worth running forbidden apis and third party audit at all on fips as there's no additional value. As in centrally disable all of them so we don't ave all this fisp specific logic in build scripts. //cc @jkakavas
The tests and testing conventions could be an onlyIf

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

👍 I'm generally in favor of this. These checks are compile-time and really shouldn't be dependent on the runtime JVM. I would be in favor of disabling these across the board for FIPS JVMs.

} else {
setting 'xpack.security.transport.ssl.keystore.path', 'testnode.jks'
setting 'xpack.security.transport.ssl.keystore.password', 'testnode'
rootProject.globalInfo.ready {

This comment has been minimized.

Copy link
@atorok

atorok May 22, 2019

Contributor

I think you'll be able to get rid of ready with "${-> }" here, and with that, we wouldn't need ready at all

This comment has been minimized.

Copy link
@mark-vieira

mark-vieira May 22, 2019

Author Contributor

The issue here is not that the system property values are different. The set of system properties themselves look to be different depending on FIPS or not.

mark-vieira added some commits May 22, 2019

Merge remote-tracking branch 'origin/master' into global-info-refactor
# Conflicts:
#	buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy
@rjernst
Copy link
Member

left a comment

I'm still not sold on this, but the configuration time improvements are worthwhile, so let's continue iterating.

@mark-vieira mark-vieira merged commit eda3da3 into elastic:master May 24, 2019

8 checks passed

CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019

Improve build configuration time (elastic#41392)
This commit moves the expensive configuration-time calculation of Java runtime version information
to runtime instead and also makes that work cacheable. This roughly equates to about a 50% 
reduction in project configuration time.
@atorok

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

@mark-vieira could we backport this to 7.x, #42313 can't be back-ported without it and I suspect it will block other back-ports too

@mark-vieira

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I started to, but it involved a lot of hacky usages of globalInfo.ready { }. If we think its worthwhile I can work on it some more.

@mark-vieira

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

I've opened #42674. The thing with 7.x is it has a lot more conditional logic around runtime java versions since master has moved to required Java 11 which simplifies things.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.