Skip to content

Turn on using LLVM threadsafely #7409

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

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

@catamorphism, this re-enables threadsafe rustpkg tests, @brson this will fail unless the bots have LLVM rebuilt, so this is a good indicator of whether that happened or not.

@catamorphism
Copy link
Contributor

Ok -- I'm remote so I don't think I can get into the bots right now. @brson , can you?

@brson
Copy link
Contributor

brson commented Jun 26, 2013

@catamorphism yes, looking into it now

@brson
Copy link
Contributor

brson commented Jun 26, 2013

I'm being asked not to do this because it will slow down the queue. I instead scheduled a build on auto with clean-llvm set.

@alexcrichton
Copy link
Member Author

Just retried, and it turned out that auto-linux-64-nopt wasn't cleaned, or as @thestinger pointed out it was more likely running on a different slave (one of the other slaves was cleaned).

@brson, this may have to wait until @graydon comes back to manually clean all workspaces on all slaves for all builders.

@alexcrichton
Copy link
Member Author

Hmm... upon further inspection, this may need some refinement. When running make check-stage2-rusti, the actual testing portion of rusti takes ~6min. Upon profiling, it turns out that ~5min is spent on contention of one LLVM lock (llvm::PassRegistry::getPassInfo). The odd part is that this isn't even running with optimizations, so rustc isn't adding any extra passes. The passes are all added by the MCJIT that we're using (it requires them). The other odd part is that the function itself isn't slow at all, it's just called a huge number of times.

When using RUST_THREADS=1, the testing time takes ~40s which is much more reasonable. This would be a lot smaller if rustc compiled small programs much more quickly (most time is spent deflating/decoding).

Upon further investigation I changed the Mutex to a RWMutex with the appropriate read/write locks and the multithreaded testing went from 6min to 20s. Long story short, it looks like that patch is a very good idea for us to include in LLVM (and perhaps even easy to push upstream?)

Edit: I sent a patch to LLVM's mailing list: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130624/179287.html

@alexcrichton
Copy link
Member Author

With newer profiles (after my LLVM change), there's still an enormous amount of time spent in contention on that rwlock, so it would probably be useful to have something like #6512 to control the level of concurrency in tests. Although this also brings up the question of why that mutex is a global mutex and not within the PassRegistry object itself...

@alexcrichton
Copy link
Member Author

@catamorphism, @brson, I've updated this using graydon's new strategy for invalidating LLVM builds, so it should be good for another go.

bors added a commit that referenced this pull request Jul 1, 2013
@catamorphism, this re-enables threadsafe rustpkg tests, @brson this will fail unless the bots have LLVM rebuilt, so this is a good indicator of whether that happened or not.
@bors bors closed this Jul 1, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 1, 2021
…te,flip1995

Added `cargo dev setup vscode-tasks` for simplicity

This PR adds a setup command to `clippy dev` that installs tasks into the Clippy vscode workspace. These might be useful as they be used via shortcuts and are accessible over the GUI. The available tasks are:
* `cargo check` (standard Linux shortcut `[ctrl] + [shift] + b`)
* `cargo dev fmt`
* `cargo uitest` (with a comment how to add the `TESTNAME` environment value)
* `cargo test`
* `cargo dev bless`

---

changelog: none

only internal changes again. cc rust-lang#5394

r? `@flip1995` This should be pretty much the same as the other `cargo dev setup` commands. Would you mind reviewing this? 🙃
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.

5 participants