Multithreading support for lazy-jit by eggyal · Pull Request #1166 · rust-lang/rustc_codegen_cranelift

eggyal · 2021-04-26T20:25:54Z

Fixes #1124, in which @bjorn3 commented:

It should be possible to use a channel between the rustc thread and the thread requesting the compilation and then wait for the function to be compiled.

The problem I had using a channel was that I couldn't get the Sender to the child threads other than through using a global static (in some way), which requires synchronisation... and then the channel just added additional synchronisation on top—which by that point was superfluous. This PR uses a Mutex<VecDeque> for message passing instead.

bjorn3 · 2021-04-27T08:36:27Z

Thanks! I will take a look tomorrow.

eggyal · 2021-04-27T09:27:53Z

Woke this morning with a realisation that it's probably better to use a channel after all—still requires synchronisation to get a clone, but that clone can then be stored thread-local which will then use far less locking.

eggyal · 2021-04-27T16:15:02Z

Ran some tests with more threads and started hitting BAD_ACCESS errors... placing this in draft while I investigate.

bjorn3 · 2021-04-28T08:09:01Z

I think bytecodealliance/wasmtime#2786 is required for this to work. I need to implement bytecodealliance/wasmtime#2786 (comment) though before it can be merged.

eggyal · 2021-04-28T08:15:07Z

I removed my previous comments as I'm now encountering different variations... but yes, I'm sure you're right that that's the cause here.

eggyal · 2021-04-28T16:14:30Z

I extended your branch to implement deferred GOT updates in https://github.com/eggyal/wasmtime/tree/atomic_jit — and that did indeed fix the problem here... but unless Cranelift is to itself run multithreaded, there's no chance of the GOT being updated from multiple threads so I don't think there's any need for AtomicPtr? Certainly other threads are reading from the GOT and you don't want a dirty read, but those aren't atomic loads (unless it's your intention to introduce them?) and don't most (if not all?) architectures guarantee that reading an aligned usize will never be dirty anyway?

Or is the issue about ensuring atomic updates of multiple GOT entries, such as data objects and related functions, so that they're never witnessed in an inconsistent state? Not sure AtomicPtr helps with that either. Maybe the whole GOT needs to be behind a RwLock...

bjorn3 · 2021-04-28T17:16:49Z

Data races are UB in Rust and LLVM. LLVM is for example allowed to write arbitrary values (say 42) before writing the final value. This would crash the jitted code. Data races are not UB in Cranelift. For this reason the rust code needs to use atomic stores to prevent UB, while the Cranelift side doesn't need to use atomic loads to prevent UB. At least on all architectures currently supported by Cranelift.

By the way I think you will need to ensure that two simultaneous requests to compile the same function will result in only a single compilation.

bjorn3 · 2021-04-28T17:20:15Z

Instead of storing a GotEntryKey in GotEntry could you store an *const AtomicPtr pointing directly to the got entry instead? Also do you want me to incorporate the changes into my PR or do you want to open a replacement PR for Cranelift?

eggyal · 2021-04-28T18:05:20Z

Data races are UB in Rust and LLVM. LLVM is for example allowed to write arbitrary values (say 42) before writing the final value. This would crash the jitted code. Data races are not UB in Cranelift. For this reason the rust code needs to use atomic stores to prevent UB, while the Cranelift side doesn't need to use atomic loads to prevent UB. At least on all architectures currently supported by Cranelift.

Aha, got it. Thanks.

By the way I think you will need to ensure that two simultaneous requests to compile the same function will result in only a single compilation.

D'oh! Well caught. Considered holding some list of pending *const Instances in a static, but since that would require locking on every jit request it brought me full circle back to the original idea of just handling messages via a static Mutex<VecDeque> and saving on the additional synchronisation provided by a channel. Will push an update shortly.

Instead of storing a GotEntryKey in GotEntry could you store an *const AtomicPtr pointing directly to the got entry instead? Also do you want me to incorporate the changes into my PR or do you want to open a replacement PR for Cranelift?

I've used NonNull<AtomicPtr<u8>>, which is quite neat as it just copies directly through the Options, and of course enforces the additional semantics. Have pushed that one to my branch already. Don't mind however you want to do it... can open a PR to your branch in your repo if that's any help?

bjorn3 · 2021-04-28T19:43:04Z

I force pushed it directly to my PR branch.

eggyal · 2021-04-29T05:45:23Z

Requires exposure of JITModule::get_got_address in Cranelift, for which I'll submit a PR.

bjorn3 · 2021-06-10T11:23:49Z

bytecodealliance/wasmtime#2786 has been merged. @eggyal can you please rebase this PR?

eggyal · 2021-06-25T15:40:26Z

@bjorn3 Not sure whether you saw that I rebased this as requested, but pinging you just in case.

bjorn3 · 2021-06-25T15:41:16Z

Completely missed it. Thanks for the ping.

bjorn3 · 2021-06-25T15:50:07Z

Pushed a commit to test this feature.

bjorn3 · 2021-06-25T16:32:20Z

CI seems to be having some issues with installing wine for windows testing. Merging as I don't expect any failures from this CI job.

…jorn3 Sync rustc_codegen_cranelift The main hightlight this sync is basic support for AArch64. Most things should work on Linux, but there does seem to be an ABI incompatibility causing proc-macros to crash, see https://github.com/bjorn3/rustc_codegen_cranelift/issues/1184. Thanks to `@afonso360` for implementing all Cranelift features that were necessary to compile for AArch64 using cg_clif. Also thanks to `@shamatar` for implementing the `llvm.x86.addcarry.64` and `llvm.x86.subborrow.64` llvm intrinsics used by num-bigint (https://github.com/bjorn3/rustc_codegen_cranelift/pull/1178) and `@eggyal` for implementing multi-threading support for the lazy jit mode. (https://github.com/bjorn3/rustc_codegen_cranelift/pull/1166) r? `@ghost` `@rustbot` label +A-codegen +A-cranelift +T-compiler

…jorn3 Sync rustc_codegen_cranelift The main hightlight this sync is basic support for AArch64. Most things should work on Linux, but there does seem to be an ABI incompatibility causing proc-macros to crash, see https://github.com/bjorn3/rustc_codegen_cranelift/issues/1184. Thanks to ``@afonso360`` for implementing all Cranelift features that were necessary to compile for AArch64 using cg_clif. Also thanks to ``@shamatar`` for implementing the `llvm.x86.addcarry.64` and `llvm.x86.subborrow.64` llvm intrinsics used by num-bigint (https://github.com/bjorn3/rustc_codegen_cranelift/pull/1178) and ``@eggyal`` for implementing multi-threading support for the lazy jit mode. (https://github.com/bjorn3/rustc_codegen_cranelift/pull/1166) r? ``@ghost`` ``@rustbot`` label +A-codegen +A-cranelift +T-compiler

…jorn3 Sync rustc_codegen_cranelift The main hightlight this sync is basic support for AArch64. Most things should work on Linux, but there does seem to be an ABI incompatibility causing proc-macros to crash, see https://github.com/bjorn3/rustc_codegen_cranelift/issues/1184. Thanks to ```@afonso360``` for implementing all Cranelift features that were necessary to compile for AArch64 using cg_clif. Also thanks to ```@shamatar``` for implementing the `llvm.x86.addcarry.64` and `llvm.x86.subborrow.64` llvm intrinsics used by num-bigint (https://github.com/bjorn3/rustc_codegen_cranelift/pull/1178) and ```@eggyal``` for implementing multi-threading support for the lazy jit mode. (https://github.com/bjorn3/rustc_codegen_cranelift/pull/1166) r? ```@ghost``` ```@rustbot``` label +A-codegen +A-cranelift +T-compiler

src/driver/jit.rs

Found by m-ou-se in https://github.com/bjorn3/rustc_codegen_cranelift/pull/1166#discussion_r730468919

eggyal force-pushed the lazy-jit-multithreaded branch from 80da4a5 to 1b9dcbb Compare April 26, 2021 20:35

eggyal force-pushed the lazy-jit-multithreaded branch 9 times, most recently from 4bc1872 to 00ccca8 Compare April 27, 2021 15:27

eggyal marked this pull request as draft April 27, 2021 15:33

eggyal force-pushed the lazy-jit-multithreaded branch 2 times, most recently from 96425ef to 23c6b56 Compare April 27, 2021 18:00

eggyal marked this pull request as ready for review April 28, 2021 16:27

eggyal marked this pull request as draft April 28, 2021 23:01

eggyal force-pushed the lazy-jit-multithreaded branch 2 times, most recently from 256a892 to c87ea65 Compare April 29, 2021 05:44

This was referenced Apr 29, 2021

Atomic hotswapping in JIT mode bytecodealliance/wasmtime#2786

Merged

Expose new JITModule::read_got_entry function bytecodealliance/wasmtime#2865

Merged

eggyal force-pushed the lazy-jit-multithreaded branch from c87ea65 to ad8b191 Compare April 29, 2021 08:45

bjorn3 force-pushed the master branch from 9ea11d9 to 40dd3e2 Compare May 29, 2021 18:07

eggyal added 3 commits June 17, 2021 09:42

Multithreading support for lazy-jit

2945b96

Ensure Instances are only jitted once

4a7068d

Remove lack of mulithreading support from usage.md

5c78324

eggyal force-pushed the lazy-jit-multithreaded branch from ad8b191 to 5c78324 Compare June 17, 2021 08:53

eggyal marked this pull request as ready for review June 17, 2021 08:54

Test multithreading support in lazy-jit

57c6eb7

bjorn3 merged commit 0d1cede into rust-lang:master Jun 25, 2021

eggyal deleted the lazy-jit-multithreaded branch June 25, 2021 17:40

bjorn3 mentioned this pull request Jul 7, 2021

Sync rustc_codegen_cranelift rust-lang/rust#86927

Merged

m-ou-se reviewed Oct 17, 2021

View reviewed changes

src/driver/jit.rs Show resolved Hide resolved

bjorn3 referenced this pull request Oct 17, 2021

Remove unnecessary Lazy

8ed0d0d

Found by m-ou-se in https://github.com/bjorn3/rustc_codegen_cranelift/pull/1166#discussion_r730468919

Conversation

eggyal commented Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn3 commented Apr 27, 2021

Uh oh!

eggyal commented Apr 27, 2021

Uh oh!

eggyal commented Apr 27, 2021

Uh oh!

bjorn3 commented Apr 28, 2021

Uh oh!

eggyal commented Apr 28, 2021

Uh oh!

eggyal commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn3 commented Apr 28, 2021

Uh oh!

bjorn3 commented Apr 28, 2021

Uh oh!

eggyal commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn3 commented Apr 28, 2021

Uh oh!

eggyal commented Apr 29, 2021

Uh oh!

bjorn3 commented Jun 10, 2021

Uh oh!

eggyal commented Jun 25, 2021

Uh oh!

bjorn3 commented Jun 25, 2021

Uh oh!

bjorn3 commented Jun 25, 2021

Uh oh!

bjorn3 commented Jun 25, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eggyal commented Apr 26, 2021 •

edited

Loading

eggyal commented Apr 28, 2021 •

edited

Loading

eggyal commented Apr 28, 2021 •

edited

Loading