Skip to content

Speed up CI by 7x#315

Merged
jvican merged 9 commits intosbt:1.0from
scalacenter:fast-scripted
Jun 15, 2017
Merged

Speed up CI by 7x#315
jvican merged 9 commits intosbt:1.0from
scalacenter:fast-scripted

Conversation

@jvican
Copy link
Member

@jvican jvican commented Jun 13, 2017

Recycle the compiler bridge and the scala instance (with its parent
classloader) for every scripted test that has the same options (the
scala version). This way, we avoid resolving the bridge and classloading
it every time per scripted test.

Recycle the compiler bridge and the scala instance (with its parent
classloader) for every scripted test that has the same options (the
scala version). This way, we avoid resolving the bridge and classloading
it every time per scripted test.
@jvican
Copy link
Member Author

jvican commented Jun 13, 2017

c2cbf0d alone makes the execution of scripted more than twice faster.

Before: [error] Total time: 533 s, completed Jun 13, 2017 2:21:06 PM.
After: [error] Total time: 237 s, completed Jun 13, 2017 2:09:28 PM.

Granularity is not a solution because of the intertwined dependencies in
the project.
These are extracted from the sbt .travis.yml.
@jvican
Copy link
Member Author

jvican commented Jun 13, 2017

This is so frustrating. For some reason, Travis is not running the tests...

@jvican jvican force-pushed the fast-scripted branch 12 times, most recently from 9ed4f7b to b593de3 Compare June 14, 2017 12:16
@jvican jvican changed the title Speed up scripted tests Speed up CI by 7x Jun 14, 2017
@jvican
Copy link
Member Author

jvican commented Jun 14, 2017

Summary of changes:

The following commits make the CI 7x faster than before. This is achieved
through two main improvements:

  • Scripted tests are not resolving the compiler and class-loading all the jars
    per test (we only do it once per scripted invocation).
  • It uses Drone instead of Travis, which does not impose any constrain on
    resources. Tests are run in a machine with 48 cores and 228GB of memory.

Parallelization at the build level has been removed because modules are too
intertwined with each other, and would require compilation of almost all the
modules before proceeding to test things. Therefore, .drone.yml runs everything
together.

@jvican jvican requested a review from eed3si9n June 14, 2017 14:11
@jvican jvican requested a review from Duhemm June 14, 2017 14:11
@@ -0,0 +1 @@
scalac.options = -Xmax-classfile-name 240
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary to work around an issue with large file paths in Docker AUFS, limit is 240 bytes. It only happens with 2.11.x because the compactify test makes an intensive use of synthesized classes with huge names.

@jvican jvican mentioned this pull request Jun 14, 2017
@jvican
Copy link
Member Author

jvican commented Jun 14, 2017

Closing and opening again to update status of PR, that should be passing.

Copy link
Contributor

@Duhemm Duhemm left a comment

Choose a reason for hiding this comment

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

2 small details, otherwise LGTM! Thanks for making CI great again 👍🏻

bin/run-ci.sh Outdated
-J-Xmx3046M -J-Xms3046M -J-server \
zincRoot/test:compile scalafmtCheck \
"publishBridgesAndSet $SCALA_VERSION" \
crossTestBridges zincRoot/test zincRoot/scripted
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you put each distinct command on a separate line? It makes it easier to read and less scary to configure the CI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

sys.error(s"$command")
override def apply(command: String,
arguments: List[String],
state: Option[IncInstance]): Option[IncInstance] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

state: State and returns a State too

Copy link
Member Author

Choose a reason for hiding this comment

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

It's true, using State here makes it waaaay clearer.

Copy link
Contributor

@Duhemm Duhemm left a comment

Choose a reason for hiding this comment

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

LGTM!

@jvican jvican closed this Jun 15, 2017
@jvican jvican reopened this Jun 15, 2017
@jvican
Copy link
Member Author

jvican commented Jun 15, 2017

Merging, travis hook has been disabled.

@jvican jvican merged commit 866d9fd into sbt:1.0 Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants