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

Revert #63649 - "Upgrade Emscripten targets to use upstream LLVM backend" #65151

Merged
merged 1 commit into from
Oct 6, 2019

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Oct 6, 2019

This change caused the runtime of the linux-asmjs builder to nearly double from 2+ hours to about 4 hours, which happens to be the bors timeout. (It made it in barely under 4 hours when it was merged.) This is causing timeouts on all new changes.

This reverts commit 7870050, reversing
changes made to 2e72448.

…pgrade, r=alexcrichton"

This reverts commit 7870050, reversing
changes made to 2e72448.
@tmandry
Copy link
Member Author

tmandry commented Oct 6, 2019

Self-approving since this is a revert to unblock bors, and no one seems to be around.
@bors r+ p=3

@bors
Copy link
Contributor

bors commented Oct 6, 2019

📌 Commit d16b7f7 has been approved by tmandry

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 6, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Oct 6, 2019
…r=tmandry

Revert rust-lang#63649 - "Upgrade Emscripten targets to use upstream LLVM backend"

This change caused the runtime of the linux-asmjs builder to nearly double from 2+ hours to about 4 hours, which happens to be the bors timeout. (It made it in barely under 4 hours when it was merged.) This is causing timeouts on all new changes.

This reverts commit 7870050, reversing
changes made to 2e72448.
@bors bors added the rollup A PR which is a rollup label Oct 6, 2019
bors added a commit that referenced this pull request Oct 6, 2019
Rollup of 18 pull requests

This contains changes from all the successful runs that bors marked as timed out, plus a revert of #63649 which appears to be the immediate cause of the timeouts.

Successful merges:

 - #64708 (Stabilize `Option::as_deref` and `Option::as_deref_mut`)
 - #64728 (Stabilize UdpSocket::peer_addr)
 - #64765 (std: Reduce checks for `feature = "backtrace"`)
 - #64909 (When encountering chained operators use heuristics to recover from bad turbofish)
 - #65011 (Do not ICE when dereferencing non-Copy raw pointer)
 - #65064 (permit asyncawait-ondeck to be added by anyone)
 - #65066 ([const-prop] Fix ICE when trying to eval polymorphic promoted MIR)
 - #65100 (Replace GeneratorSubsts with SubstsRef)
 - #65105 (Split out some passes from librustc)
 - #65106 (Allow unused attributes to avoid incremental bug)
 - #65113 (Fix lonely backtick)
 - #65116 (Remove unneeded visit_statement definition)
 - #65118 (Update the documented default of -Z mutable-noalias)
 - #65123 (Account for macro invocation in `let mut $pat` diagnostic.)
 - #65124 (Replace some instances of `as *[const | mut] _` with `.cast()`)
 - #65126 (Fix typo on `now()` comments)
 - #65130 (lint: extern non-exhaustive types are improper)
 - #65151 (Revert #63649 - "Upgrade Emscripten targets to use upstream LLVM backend")

Failed merges:

r? @ghost
@alexcrichton
Copy link
Member

Analysis here looks spot on and agree with reverting while this is figured out. This shows a 1.5 he jump in testing time for the asmjs builder

https://alexcrichton.github.io/rust-ci-timing-tracker/commit.html#job=asmjs&commit=2e7244807a7878f6eca3eb7d97ae9b413aa49014,7870050796e5904a0fc85ecbe6fa6dde1cfe0c91&relative=true&col=1&desc=true

@bors
Copy link
Contributor

bors commented Oct 6, 2019

⌛ Testing commit d16b7f7 with merge 5a8fb7c...

@bors bors merged commit d16b7f7 into rust-lang:master Oct 6, 2019
@tmandry tmandry removed the rollup A PR which is a rollup label Oct 6, 2019
@tlively
Copy link
Contributor

tlively commented Oct 8, 2019

@alexcrichton I did some profiling and the bulk of the Emscripten link time for the tests is spent in the JS optimizer, optimizing the output of wasm2js. This time can be eliminated or reduced by changing the optimization level Emscripten uses when running the tests. Another option is to replace the asmjs-unknown-emscripten testing with wasm32-unknown-emscripten testing, which would skip the wasm2js and JS optimizing steps entirely.

I took some measurements on a sample of 489 tests with various optimization options.

-O2 asmjs: finished in 922s
-O1 asmjs: finished in 361s
-O0 asmjs: finished in 305s

-O2 wasm: finished in 354s
-O1 wasm: finished in 254s
-O0 wasm: finished in 228s

Based on this analysis I think it would be best to just switch over to testing wasm32-unknown-emscripten or to switch to using -O1 with asmjs. What do you think would be best?

@alexcrichton
Copy link
Member

@tlively I think it should be fine to switch to wasm32-unknown-emscripten and testing with -O1, that sounds like a good sweet spot!

@tlively
Copy link
Contributor

tlively commented Oct 9, 2019

@alexcrichton, rust-lang/cargo#7476 was pulled into the cargo submodule in #65186, but when I test locally it uses the stage0 cargo which does not include that fix. Will the CI use the in-tree cargo to run tests or the old stage0 cargo? If the latter, when can I expect the stage0 cargo to be updated to include rust-lang/cargo#7476? If it will be a while, we probably want to stick with testing asmjs with -O1 for now.

@alexcrichton
Copy link
Member

@tlively we always use the beta channel Cargo on the master branch's CI for tests, but if we temporarily ignore testing on wasm/emscripten for now to reenable later that's ok too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants