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

Optimizing travis build time and fixing benchmarks #148

Merged
merged 2 commits into from
Aug 13, 2019

Conversation

elichai
Copy link
Member

@elichai elichai commented Aug 12, 2019

Hi,
I did a couple of things here:

  1. telling rust to use ./cargo_web dir when installing cargo web and then caching that directory.
  2. Replacing the nodejs tests with chrome (in my tests the chrome ones are way faster)
  3. tried sorting the build/tests so that they makes sense close to each other (build and test with the same features should be one after the other), though I'm not sure if that really made any real difference.
  4. Added cargo test --benches --features=unstable on nightly, so that it will build the benchmarks (currently they fail compilation).
  5. Added the same features from the toml(https://github.com/rust-bitcoin/rust-secp256k1/blob/master/Cargo.toml#L19) to the cargo doc test

And cherry-picked this commit: 185284c which fixes the benchmarks

This PR contradicts with #147 and I'm hoping to open another PR today for appveyor windows tests. (that way we can keep the cache and just do the windows builds there) cc @real-or-random

@elichai elichai changed the title Optimizing travis build time Optimizing travis build time and fixing benchmarks Aug 12, 2019
@elichai
Copy link
Member Author

elichai commented Aug 12, 2019

You can see the resulting timing after the cache is built: https://travis-ci.org/elichai/rust-secp256k1/builds/570996846

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

This PR contradicts with #147 and I'm hoping to open another PR today for appveyor windows tests. (that way we can keep the cache and just do the windows builds there) cc @real-or-random

Hm, one CI suite is already annoying enough. I don't think we want two.

My view is this: The cache may speed things up but it adds complexity. If we can get away without the cache (and #147 demonstrates that we can), I don't think we want to enable it.

If your changes here change that situation and caching the web stuff really gives us a significant speedup, then I feel we should stick with Travis and just cache the web stuff.

@elichai
Copy link
Member Author

elichai commented Aug 12, 2019

@real-or-random the thing is that travis fails lately for endless amount of stuff. mostly in windows, but we cannot complain because it is in beta.
I do think that introducing a stable windows tests is a good thing, especially because this lets us test both msvc and gnu(mingw), and we can run the cargo web on windows too.

You can even see that travis failed now on linux stable even though nothing failed.

.travis.yml Outdated
@@ -1,5 +1,8 @@
language: rust
cache: cargo
cache:
cargo: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

More concretely, if you change this to false but keep the cargo_web cache enabled, maybe it's already good enough to make the Windows builds fast (I don't think so.) If it's not enough, you can add the before_cache phase and let it remove the contents of the cargo_web directory only on windows. As Travis keeps separate caches for Windows and Linux, this won't affect the Linux builds where the web cache is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

right now cargo web isn't used in windows at all. so removing the cargo cache will leave windows with no cache at all (which is fine I guess)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, that was my suggestion because the cache makes it slower on windows.

@real-or-random
Copy link
Collaborator

real-or-random commented Aug 12, 2019

@real-or-random the thing is that travis fails lately for endless amount of stuff. mostly in windows, but we cannot complain because it is in beta.
I do think that introducing a stable windows tests is a good thing, especially because this lets us test both msvc and gnu(mingw), and we can run the cargo web on windows too.

I see, but wouldn't then the proper way be then to switch to appveyor entirely? And this needs more discussion, certainly among the maintainers here and even among other repos in this project.

You can even see that travis failed now on linux stable even though nothing failed.

Failure is here:
https://travis-ci.org/rust-bitcoin/rust-secp256k1/jobs/571028953#L850

@elichai
Copy link
Member Author

elichai commented Aug 12, 2019

I agree this is something that requires discussion which is why I decided to do it in a separate PR (which I hope i'll have time to open today)

.travis.yml Outdated
- if [ "$(rustup show | grep default | grep stable)" != "" -a "$TRAVIS_OS_NAME" = "linux" ]; then cargo install --force cargo-web && cargo web build --target=asmjs-unknown-emscripten && cargo web test --target=asmjs-unknown-emscripten --nodejs; fi
- cargo build --verbose --release
- cargo test --verbose --release
- if [ ${TRAVIS_RUST_VERSION} == "stable" ]; then cargo doc --verbose --features=["rand","serde","recovery","endomorphism"]; fi
Copy link
Member

Choose a reason for hiding this comment

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

This --features=[ ] syntax is invalid

Copy link
Member Author

Choose a reason for hiding this comment

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

Ohhh So that's what made travis fail. Good catch!
I'll remove the brackets. thanks.

@real-or-random
Copy link
Collaborator

I agree this is something that requires discussion which is why I decided to do it in a separate PR (which I hope i'll have time to open today)

Sounds good. For this PR I'd suggest the following: Can you try the hack above to enable only the web cache, and see if it makes a difference for the web stuff. If yes, we should keep the hack it, otherwise (e.g., if chrome is responsible for the speedup), I'd still disable the cache entirely. In any case, this PR will then supersede #147.

Then we have a proper fix for now, and can think about appveyor in a different PR.

@elichai
Copy link
Member Author

elichai commented Aug 13, 2019

It seems to still perform quite well https://travis-ci.org/elichai/rust-secp256k1/builds/571333406

@real-or-random
Copy link
Collaborator

real-or-random commented Aug 13, 2019

It seems to still perform quite well https://travis-ci.org/elichai/rust-secp256k1/builds/571333406

I don't think that this has used the cache already:
https://travis-ci.org/elichai/rust-secp256k1/jobs/571333408#L24

Can you restart it? I'm also restarting the PR build here to try it when it really fetches the cache.

edit: For reference, before the restart:

Linux stable    6:19
Linux beta      3:19
Linux nightly   3:30
Linux 1.22      3:01
Windows stable  8:20
Windows beta    7:30
Windows nightly 8:07

@elichai
Copy link
Member Author

elichai commented Aug 13, 2019

Somehow we're seeing different URLs, but this is after restart: https://travis-ci.org/elichai/rust-secp256k1/jobs/571333407#L172 and took 6 minutes.

@real-or-random
Copy link
Collaborator

Okay, I see, I was confused. Of course the Windows builds can't restore the web cache because they never create files in that directory...
So all of this looks good. Have you also tried turning off the cache entirely? Does that make a large difference for the one Linux stable job?

@elichai
Copy link
Member Author

elichai commented Aug 13, 2019

Yes, caching that directory is what gives most of the speedup, without it it would be similar to https://travis-ci.org/rust-bitcoin/rust-secp256k1/jobs/569951734

cargo-web has 220-230 dependencies. in can take travis quite a while to build them all

@real-or-random real-or-random merged commit dfe7ee5 into rust-bitcoin:master Aug 13, 2019
@elichai elichai deleted the 2019-08-travis branch August 13, 2019 15:51
This was referenced Aug 16, 2019
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