Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Wrapping task_pool for platform independent interface #4346

Closed
wants to merge 5 commits into from

Conversation

MiniaczQ
Copy link
Contributor

@MiniaczQ MiniaczQ commented Mar 27, 2022

Objective

As described in #4304

Solution

  • Both wasm and non-wasm files were renamed to task_pool_wasm and task_pool_other.
  • task_pool was made as the wrapper and only public interface
  • All non-wasm documentation (that's not platform specific) and tests were moved to the wrapper file (and removed from original)

Iffy stuff

  • Scope wrapper is used in platform specific TaskPool::scope implementations to ensure user always sees user-sided types
  • TaskPool::spawn and TaskPool::spawn_local were replaced with TaskPool::spawn_local_detached to normalize wasm and non-wasm interfaces without a major rework, while still satisfying the internal use cases

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 27, 2022
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Tasks Tools for parallel and async work and removed S-Needs-Triage This issue needs to be labelled labels Mar 27, 2022
@MiniaczQ MiniaczQ marked this pull request as ready for review March 28, 2022 01:15
@MiniaczQ
Copy link
Contributor Author

Alright, here's the situation.
Wrappers are mostly done and ready for reviews,
but the PR is not ready for merge, as it requires major wasm tasks rework. (to reintroduce TaskPool::spawn)

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Oct 10, 2022
@alice-i-cecile
Copy link
Member

@MiniaczQ is there an issue to link to for that WASM tasks rework?

@MiniaczQ
Copy link
Contributor Author

Right here #4301

@alice-i-cecile
Copy link
Member

Given that #4466 is now merged, is this still blocked?

@MiniaczQ
Copy link
Contributor Author

Shouldn't be, I'll try to get a draft done today, just gonna reapply changes to a fresh main since it'll be quicker

@MiniaczQ
Copy link
Contributor Author

MiniaczQ commented Oct 10, 2022

Kudos to hymm, the refactor was very smooth 😃
I'll tweak the docs a bit more tomorrow, combine what's good from both.

@alice-i-cecile alice-i-cecile requested a review from hymm October 10, 2022 22:04
@MiniaczQ
Copy link
Contributor Author

Status for today:

  • found out the other problem I had originally: wasm returns FakeTask, that doesn't wrap well; I'm thinking return an already completed task
  • SIGSEGV: need to figure out why the memory is invalid; first time using transmute

@MiniaczQ
Copy link
Contributor Author

Bevy-local fix to FakeTask would be to provide a spawn_detached, but I don't think that's a long term solution

}
self.0.scope(|s| {
let scope = Scope(s);
let scope_ref: &'env Scope<'_, 'env, T> = unsafe { mem::transmute(&scope) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert with unsafe Rust, but '_ probably creates an unbounded lifetime (See the unbounded lifetimes part). You should specify a lifetime for 'scope in Scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with <'scope, 'env, T>, same SIGSEGV

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I posted this on discord, but will repost here so it doesn't get lost. The problem is that scope_ref only lives for the lifetime of the closure it's in, but needs to live until .scope returns. I'm not sure how to get around this as it's important that the scope object is constructed inside the scope function, so that the 'scope lifetime is only available inside the scope function. Basically this needs to be self.0.scope(f), but I'm not sure how you do that with the wrappers.

executor,
results,
scope: PhantomData,
env: PhantomData,
};

let scope_ref: &'env mut Scope<'_, 'env, T> = unsafe { mem::transmute(&mut scope) };
let scope_ref: &'env mut PlatformScope<'_, 'env, T> = unsafe { mem::transmute(&mut scope) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you can see here this was only refactored by name, so that isn't an issue

@MiniaczQ MiniaczQ closed this Jan 14, 2024
@MiniaczQ MiniaczQ deleted the wrap-task-pools branch January 14, 2024 00:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-Tasks Tools for parallel and async work C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants