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

Add a basic futures example #36

Merged
merged 5 commits into from
Feb 5, 2022
Merged

Add a basic futures example #36

merged 5 commits into from
Feb 5, 2022

Conversation

AzureMarker
Copy link
Member

This example runs a basic future executor from the futures library.
Every 60 frames (about 1 second) it prints "Tick" to the console.
The executor runs on a separate thread. Internally it yields when it has no more work to do, allowing other threads to run.
The example also implements clean shutdown by using a oneshot channel to end the future, thus ending the executor and the thread it runs on.

I was surprised that this all works as-is, without any new linker fixes needed.

I also started on a Tokio port of this example, using their timer runtime feature, but that requires rust3ds/shim-3ds#9

@Meziu
Copy link
Member

Meziu commented Feb 1, 2022

Can Tokio actually run??? This PR will have to wait for that 😂

@AzureMarker
Copy link
Member Author

AzureMarker commented Feb 1, 2022

It compiles at least. Might even run if I disable the time feature and count frames like this PR (it panics when creating the runtime because Instant doesn't work). But we're pretty close to getting monotonic clock working so stay tuned 😎

@Meziu
Copy link
Member

Meziu commented Feb 1, 2022

You know, a system with clear problems with threading running one of the biggest multithread frameworks, THAT is something to make C/C++ users burn of envy 😈

@Meziu
Copy link
Member

Meziu commented Feb 1, 2022

I am not an expert in threading, so I don't know how much my question makes sense but: would it have an higher performance if we ran the task executor on the SysCore (index 1 in libctru). That core can only run a single thread, and it is fully preemptive. It looks like the best option for the executor. (And even if it isn't much of a change, I wouldn't know how else to use that core honestly).

@AzureMarker
Copy link
Member Author

That makes sense. Either way the executor will yield/park the thread when it's not doing work. I'll make the change soon.

@AzureMarker
Copy link
Member Author

Weird... I get a panic when I change the executor thread affinity to 1:

Failed to create executor thread: Os { code: 11, kind: WouldBlock, message: "No more processes" }

I checked the main thread's details (luckily ctru-rs already provides an extended thread API) and it's running on CPU 0 with priority 48.

@AzureMarker
Copy link
Member Author

Ah OK, so we have to set a time limit for how much of the system core we can use before we're allowed to spawn a thread on it. Updated the PR to use the system core at up to 30% (range is 5-89 according to https://www.3dbrew.org/wiki/APT:SetApplicationCpuTimeLimit).

@Meziu
Copy link
Member

Meziu commented Feb 2, 2022

rust3ds/shim-3ds#9 has been merged, and it works wonderfully. Update us on the progress 😄

@AzureMarker
Copy link
Member Author

AzureMarker commented Feb 3, 2022

So the tokio example gets a little farther now, after updating dependencies and calling psInit (manually for now until #39 merges). However it panics during the runtime's block_on due to a failed assertion in VecDeque, here:
https://github.com/Meziu/rust-horizon/blob/bbaf4bfbdb91c768e86a4bbd8222474279fe9709/library/alloc/src/collections/vec_deque/mod.rs#L2758

It executes our panic handler fine (doesn't wait for input since it was not the main thread), but during unwinding it calls the VecDeque's Drop impl and panics again:
Line in the Drop impl: https://github.com/Meziu/rust-horizon/blob/bbaf4bfbdb91c768e86a4bbd8222474279fe9709/library/alloc/src/collections/vec_deque/mod.rs#L147
Line where it goes down the panic branch: https://github.com/Meziu/rust-horizon/blob/bbaf4bfbdb91c768e86a4bbd8222474279fe9709/library/core/src/slice/index.rs#L256

Since there was a panic while already panicking, Rust aborts the process (causing an ugly ARM11 exception).

This is all done with a debug build. Since the first panic was caused by a debug_assert, it doesn't get triggered in release mode.

The release mode build doesn't immediately crash, but it also doesn't print any "Tick" messages. Eventually the runtime thread does panic here (condvar timeout):
https://github.com/Meziu/rust-horizon/blob/bbaf4bfbdb91c768e86a4bbd8222474279fe9709/library/std/src/sys/unix/condvar.rs#L116

The main thread is still running fine, so pressing Start to exit works.

AzureMarker added a commit that referenced this pull request Feb 3, 2022
There's some issues with the example right now. See
#36 (comment)
@AzureMarker
Copy link
Member Author

AzureMarker commented Feb 3, 2022

@Meziu
Copy link
Member

Meziu commented Feb 3, 2022

536                  core.tasks
(gdb) p core.tasks
$8 = VecDeque(size=-132873225)

HMMMMMM... 🤔
What could the problem be?

@Meziu
Copy link
Member

Meziu commented Feb 3, 2022

I can't put my finger on where (because hardware watchpoints don't keep up), but at one point during the block_on function the tasks VecDeque gets filled with stuff and its length is wrongly updated. Using watch tells me I may have set too many hardware watchpoints, which is probably due to the big amount of updates the VecDeque goes through. If the function wrap_index panics, it's not because of a problem directly in alloc, but some math probably goes wrong and the VecDeque results with a wrong capacity value.

Edit: The capacity value is wrong, but the tasks does get filled with a lot of entries, most of which seem invalid (like holding NULL pointers).

@Meziu
Copy link
Member

Meziu commented Feb 4, 2022

I'm circling around the problem.
It looks to be in tokio-1.16.1/src/macros/scoped_tls.rs:50:

        let prev = self.inner.with(|c| {
            let prev = c.get();
            c.set(t as *const _ as *const ());
            prev
        });

This is a very specific function, and it uses std::thread::local::LocalKey impls. Before this function, the core.tasks vector is empty (you can check it here via gdb with p f.core.tasks), but right after it results completely full and with a broken size value. I bet the LocalKey implementation screws up the pointers and points to unrelated memory, but I don't know how.

@AzureMarker
Copy link
Member Author

I guess our hack to get thread keys working in pthread-3ds might have a bug...

@AzureMarker
Copy link
Member Author

So using a modified pthread-3ds (I'll open a PR) it now aborts here:
https://github.com/Meziu/rust-horizon/blob/bbaf4bfbdb91c768e86a4bbd8222474279fe9709/library/core/src/sync/atomic.rs#L2381

If you move the runtime creation into the thread, it aborts here:
https://github.com/Meziu/ctru-rs/blob/54085101f33d56099afe3f45341f712a758418fe/ctru-rs/src/thread.rs#L204
(it seems to call drop_in_place, which then segfaults??)

@AzureMarker
Copy link
Member Author

In my latest iteration, it still has the weird stuff happening with VecDeque when the runtime is created outside the thread, but it acts more like the release build if you move the runtime creation into the thread. It just sits there after "Runtime started!" doing nothing. However, once you click "Start" to exit, it aborts here (when calling free in libctru):
https://github.com/Meziu/ctru-rs/blob/54085101f33d56099afe3f45341f712a758418fe/ctru-rs/src/thread.rs#L984

@AzureMarker
Copy link
Member Author

AzureMarker commented Feb 5, 2022

Opened the pthread-3ds PR: rust3ds/pthread-3ds#5

Made a few more pthread-3ds changes since the last comment. If you run this example in release mode with the runtime created on the main thread, it cleanly exits when you press Start (but doesn't print "Tick").

@AzureMarker
Copy link
Member Author

Oh yeah, and there's a rustc fix needed too: Meziu/rust-horizon#8

@AzureMarker AzureMarker mentioned this pull request Feb 5, 2022
3 tasks
@AzureMarker
Copy link
Member Author

Moving discussion about Tokio to #42. Maybe we can merge this basic futures PR in the meantime?

@Meziu Meziu merged commit 24ec4f2 into rust3ds:master Feb 5, 2022
@AzureMarker AzureMarker deleted the example/futures-basic branch February 5, 2022 19:25
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.

2 participants