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

Major improvements to wasm-bindgen-futures #1760

Merged
merged 11 commits into from Sep 26, 2019

Conversation

Pauan
Copy link
Contributor

@Pauan Pauan commented Sep 8, 2019

Sorry I'm late, I struggled with the multi-threading atomic stuff.

This PR contains a few major improvements:

  • Code duplication has been removed.

  • Everything has been refactored so that the implementation is much easier to understand.

  • future_to_promise is now implemented with spawn_local rather than the other way around (this means spawn_local is faster since it doesn't need to create an unneeded Promise).

  • Both the single threaded and multi threaded executors have been rewritten from scratch:

    • They only create 1-2 allocations in Rust per Task, and all of the allocations happen when the Task is created.

    • The singlethreaded executor creates 1 Promise per tick, rather than 1 Promise per tick per Task.

    • Both executors do not create Closures during polling, instead all needed Closures are created ahead of time.

    • Both executors now have correct behavior with regard to spurious wakeups and waking up during the call to poll.

    • Both executors cache the Waker so it doesn't need to be recreated all the time.

I believe both executors are now optimal in terms of both Rust and JS performance.

@Pauan
Copy link
Contributor Author

Pauan commented Sep 8, 2019

(The build failures don't seem related to this PR, except for the unix tests, which seem... really weird. It almost seems like a bug with anyref)

Copy link
Contributor

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Very nice! Does indeed read quite well and seems simple enough!

crates/futures/src/lib.rs Outdated Show resolved Hide resolved
crates/futures/src/task/multithread.rs Outdated Show resolved Hide resolved
crates/futures/src/task/multithread.rs Outdated Show resolved Hide resolved
crates/futures/src/task/multithread.rs Outdated Show resolved Hide resolved
crates/futures/src/task/multithread.rs Outdated Show resolved Hide resolved
crates/futures/src/task/multithread.rs Outdated Show resolved Hide resolved
crates/futures/src/queue.rs Outdated Show resolved Hide resolved
crates/futures/src/queue.rs Outdated Show resolved Hide resolved
crates/futures/src/queue.rs Outdated Show resolved Hide resolved
@Pauan
Copy link
Contributor Author

Pauan commented Sep 15, 2019

(@alexcrichton I fixed all your feedback btw)

@alexcrichton
Copy link
Contributor

I pushed up some stylistic feedback and tweaks that I figured would be easiest to go ahead and do myself. I debugged the issue with anyref and I think we'll need to fix that somehow before this lands. Looks like Promise.resolve wasn't previously used in the wasm-bindgen test suite, and it's now used transitively through wasm-bindgen-futures. The anyref pass means that Promise.resolve is directly imported, as-is, into the wasm module. This is, however, invalid:

> Promise.resolve(1)
Promise { 1 }
> x = Promise.resolve
[Function: resolve]
> x(1)
Thrown:
TypeError: PromiseResolve called on non-object
    at resolve (<anonymous>)

We have to call resolve via dot-notation or somehow set the this parameter, so the wasm-bindgen optimization to directly import Promise.resolve is not actually valid. I'm not sure the best way to fix that at this time.

@Pauan
Copy link
Contributor Author

Pauan commented Sep 16, 2019

I'm not sure the best way to fix that at this time.

Yeah, that's tricky. Can we somehow special-case static_method_of so it doesn't get the optimization? Just as a temporary solution until we get proper WebIDL integration.

@alexcrichton
Copy link
Contributor

Yeah that seems reasonable to me!

@alexcrichton
Copy link
Contributor

@Pauan have you had a chance to take a look at this again? Do you want some help in fixing importing Promise?

@Pauan
Copy link
Contributor Author

Pauan commented Sep 25, 2019

@alexcrichton I did take a look, but the anyref optimizations are over my head, so some help would be appreciated.

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this pull request Sep 26, 2019
This came up during rustwasm#1760 where `Promise.resolve` must be invoked with
`this` as the `Promise` object, but we were erroneously importing it in
such a way that it didn't have a shim and `this` was `undefined`.
@alexcrichton
Copy link
Contributor

Sure thing, I've posted #1795 to fix imports of static_method_of to ensure that this is set correctly.

alexcrichton added a commit that referenced this pull request Sep 26, 2019
This came up during #1760 where `Promise.resolve` must be invoked with
`this` as the `Promise` object, but we were erroneously importing it in
such a way that it didn't have a shim and `this` was `undefined`.
@alexcrichton alexcrichton merged commit bdcf27c into rustwasm:master Sep 26, 2019
@alexcrichton
Copy link
Contributor

👍

All green now!

@Pauan Pauan deleted the futures-optimization branch September 27, 2019 11:27
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.

None yet

2 participants