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

Merge our thread code back into std? #45

Closed
AzureMarker opened this issue Feb 6, 2022 · 11 comments · Fixed by #46
Closed

Merge our thread code back into std? #45

AzureMarker opened this issue Feb 6, 2022 · 11 comments · Fixed by #46
Assignees

Comments

@AzureMarker
Copy link
Member

AzureMarker commented Feb 6, 2022

Some context: #44

  • Pros
    1. Keeps us up-to-date with std.
    2. Not duplicating code. We could define an extension trait for 3ds specific data like affinity.
    3. Other code can still use std threads (though we should generally avoid this since the 3ds CPU works differently).
  • Cons
    1. This allows 3rd party code to use threads, which might break since the default core is not preemptive (and we only get one thread on the preemptive system core).
      • TODO: if it's not preemptive, what is priority used for? Research how the application core scheduler works.
    2. We can't directly link std to libctru. So we'd need to pass the extra data (ex. affinity) through pthread or something.
@Meziu
Copy link
Member

Meziu commented Feb 6, 2022

Priority and affinity are all we have to pass through. Surely this becomes problematic when thinking about third-party crates, but J guess it’s just another thing we can only worry our users not to do.

Can we implement extensions on pre-existing std structs?

@Meziu
Copy link
Member

Meziu commented Feb 6, 2022

TODO: if it's not preemptive, what is priority used for? Research how the application core scheduler works.

From testing I can say: exactly that.
Threads with higher priority will always run before threads with lower one, and those lower will run only once all threads with a higher priority are either locked or sleeping. I’m pretty sure a simple “yield” (which in libctru is basically a sleep for 0 time) won’t yield to a lower priority, though I’m not that sure.

Threads on a same priority seem to run in a very “preemptive” way. It probably just uses a random one from those available.

@AzureMarker
Copy link
Member Author

AzureMarker commented Feb 6, 2022

Can we implement extensions on pre-existing std structs?

Yeah, see for example https://doc.rust-lang.org/stable/std/os/unix/fs/trait.MetadataExt.html

Edit: oh, maybe you meant outside of std. Yeah we could do that, but we wouldn't be able to modify how std's implementation works (ex. the pthread calls, or access the private fields)

@Meziu
Copy link
Member

Meziu commented Feb 6, 2022

Ok, an OS struct to work with that... I bet the Rust maintainers won't be please to see a cooperative threading model be used as a preemptive, but what can we do.

@AzureMarker
Copy link
Member Author

I'm mostly through implementing this I think. Haven't tested it yet though 🙃.

@ian-h-chamberlain
Copy link
Member

I noticed something about the test runner added in #41, which is that a panic in the single-threaded test runner pauses everything silently until the user presses SELECT (not ideal).

This is due to a combination of the default ctru::init panic handler and the fact that the test both a) has stdout captured and b) is running in the main thread, which triggers this code to loop while waiting for input.

I think running with threads would mostly resolve this, since the background thread panics would simply exit the threads as usual, but I'm open to other workarounds meanwhile if we'd rather keep the single-threaded behavior for now.

@AzureMarker
Copy link
Member Author

AzureMarker commented Feb 10, 2022

We can just disable the handler with #[cfg(not(test))]

Edit: Or use the cfg macro like this:

if cfg!(not(test)) && main_thread == thread::current().id() && console::Console::exists() {
    // ...
}

@ian-h-chamberlain
Copy link
Member

Edit: Or use the cfg macro like this:

This is a good idea, and it works well enough, but doesn't behave very nicely if the test_runner code panics, which unfortunately seems to happen for certain combinations of configuration options. I think it's fine for now but perhaps multi threaded testing would be more robust as a future enhancement.

@AzureMarker
Copy link
Member Author

This is a good idea, and it works well enough, but doesn't behave very nicely if the test_runner code panics

We could technically work around this with a global boolean wrapped in a mutex, to signal if the tests have started or not. But that might be messy.

@Meziu
Copy link
Member

Meziu commented Feb 10, 2022

We should work toward the std threads instead. Our ctru hook is already messy enough.

@AzureMarker
Copy link
Member Author

@Meziu nice timing: #46

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 a pull request may close this issue.

3 participants