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

Travis: Enable cpu-specific optimizations. #195

Merged
merged 2 commits into from
Sep 4, 2018

Conversation

c0gent
Copy link
Contributor

@c0gent c0gent commented Aug 6, 2018

This is off by default so that binaries can be shared. Enabling this
flag could improve test speed on Travis (will check).

@c0gent c0gent force-pushed the c0gent-travis-opt branch from 76bbadc to 949e6c5 Compare August 7, 2018 00:08
Copy link
Collaborator

@afck afck left a comment

Choose a reason for hiding this comment

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

🏁 12 min 39 sec 🏁
That was among the fastest test runs. 🏆
(Unfortunately there is a high variance because of the randomness in the tests, so it's hard to tell how much of a difference the optimizations make.)

@c0gent
Copy link
Contributor Author

c0gent commented Aug 7, 2018

Indeed.

@c0gent
Copy link
Contributor Author

c0gent commented Aug 7, 2018

I'm going to wait until the mlock issues are sorted before rebasing and merging this.

@afck
Copy link
Collaborator

afck commented Aug 10, 2018

This could probably be rebased now. (Which will give us another data point. ⏱️)

@c0gent c0gent force-pushed the c0gent-travis-opt branch from 949e6c5 to f2f75f1 Compare August 10, 2018 13:15
@c0gent
Copy link
Contributor Author

c0gent commented Aug 10, 2018

Done.

@afck
Copy link
Collaborator

afck commented Aug 13, 2018

Needs to be rebased again; then it can be merged, I think.

@c0gent c0gent force-pushed the c0gent-travis-opt branch from f2f75f1 to 1edf9bd Compare August 13, 2018 15:09
@mbr
Copy link
Contributor

mbr commented Aug 14, 2018

I'm going to wait until the mlock issues are sorted before rebasing and merging this.

I'd +1 this. Should we give the SIGILL a bit more time to crop up?

@mbr
Copy link
Contributor

mbr commented Aug 14, 2018

Also: rust-lang/rust#53322 (only nightly - but will that envvar mess with clippy compilation?).

@c0gent c0gent force-pushed the c0gent-travis-opt branch from 7d39a99 to f41b94a Compare August 29, 2018 13:57
@c0gent
Copy link
Contributor Author

c0gent commented Aug 29, 2018

This is rebased. Will merge if tests pass.

@c0gent c0gent force-pushed the c0gent-travis-opt branch from f41b94a to fde2fae Compare August 29, 2018 14:03
@afck
Copy link
Collaborator

afck commented Aug 29, 2018

SIGILL 😩
Did MLOCK_SECRETS somehow not work (not exported? outdated threshold_crypto dependency?) or is this a separate issue…?

@c0gent
Copy link
Contributor Author

c0gent commented Aug 29, 2018

The dependency should always be up to date on travis. I'm not sure what the deal is.

@c0gent c0gent force-pushed the c0gent-travis-opt branch from fde2fae to 477697b Compare August 31, 2018 18:54
@c0gent
Copy link
Contributor Author

c0gent commented Aug 31, 2018

Added --test-threads 1. I'm not sure if this is being added elsewhere or what has been tried or discussed about it.

@c0gent c0gent force-pushed the c0gent-travis-opt branch from 7e976d1 to c55d58a Compare August 31, 2018 18:57
@c0gent
Copy link
Contributor Author

c0gent commented Aug 31, 2018

I'm going to rerun the CI check a few times just to see what happens. Do we want to keep the --test-threads 1 part?

@c0gent
Copy link
Contributor Author

c0gent commented Aug 31, 2018

We may want to experiment with setting it to 2 as well since the Travis env uses 2 cores.

We could also use a mutex to selectively prevent the tests that use the most mlock memory from running in parallel.

@afck
Copy link
Collaborator

afck commented Sep 3, 2018

In either case, the SIGILL isn't related to -cpu=native at all, is it? I've also seen it in other PRs; so this is good to merge?

This is off by default so that binaries can be shared.
@c0gent c0gent merged commit 56198e3 into poanetwork:master Sep 4, 2018
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