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

feat(driver-adapters): enable Wasm on query-core #4445

Merged
merged 45 commits into from
Nov 20, 2023

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Nov 14, 2023

This PR closes https://github.com/prisma/team-orm/issues/280, and depends on #4444.
It also deprecates #4120.

query-core now has an optional metrics feature, which will be turned off when imported on query-engine-wasm, and turned on otherwise.

@jkomyno jkomyno self-assigned this Nov 14, 2023
…github.com:prisma/prisma-engines into feat/sql-query-connector-on-wasm32-unknown-unknown
query-engine/core/Cargo.toml Outdated Show resolved Hide resolved
Copy link

codspeed-hq bot commented Nov 14, 2023

CodSpeed Performance Report

Merging #4445 will improve performances by 5.73%

Comparing feat/query-core-on-wasm32-unknown-unknown (19e0aef) with main (efbc865)

Summary

⚡ 1 improvements
✅ 10 untouched benchmarks

Benchmarks breakdown

Benchmark main feat/query-core-on-wasm32-unknown-unknown Change
large_read 7.6 ms 7.2 ms +5.73%

jkomyno and others added 2 commits November 15, 2023 17:52
…github.com:prisma/prisma-engines into feat/sql-query-connector-on-wasm32-unknown-unknown
@@ -34,3 +37,6 @@ schema = { path = "../schema" }
lru = "0.7.7"
enumflags2 = "0.7"

[target.'cfg(target_arch = "wasm32")'.dependencies]
pin-project = "1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This enables pin projection, and is used to spawn tasks in a Wasm-compatible fashion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity. Why is it so that Wasm requires memory pinning? what is what it's pinning? is it internals to memory addressess of .TEXT or .DATA sections?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 155 to 156
#[pin_project::pin_project]
pub struct JoinHandle<T>(#[pin] tokio::sync::oneshot::Receiver<T>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

task::JoinHandle<T> is a Wasm-compatible alternative to tokio::task::JoinHandle<T>.

It uses the pin_project macro to enable pin-projection.

Copy link
Contributor

@miguelff miguelff Nov 17, 2023

Choose a reason for hiding this comment

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

This might be related to my previous comment, actually: #4445 (comment)

Can you provide me with references as of how you realized this was needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. So, this is not Wasm-specific. It's rather related to the implementation of a Future trait from scratch for a tokio::sync::oneshot::Receiver (i.e., a single-use received end of a synchronized tokio channel.

Consider:

use tokio::sync::oneshot::{self};

pub struct JoinHandle<T>(oneshot::Receiver<T>);

impl<T> Future for JoinHandle<T> {
    type Output = Result<T, oneshot::error::RecvError>;

    fn poll(self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Self::Output> {
        todo!()
    }
}

Let's try replacing the todo!:

self.0.poll(cx)

yields

error[E0599]: no method named `poll` found for struct `tokio::sync::oneshot::Receiver` in the current scope
   --> 
    |
37  |             self.0.poll(cx)
    |                    ^^^^ method not found in `Receiver<T>`

    |
105 |     fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output>;
    |        ---- the method is available for `Pin<&mut tokio::sync::oneshot::Receiver<T>>` here
    |
help: consider wrapping the receiver expression with the appropriate type
    |
37  |             Pin::new(&mut self.0).poll(cx)
    |             +++++++++++++       +

Let's apply the suggestion:

core::pin::Pin::new(&mut self.0).poll(cx)

which however yields

error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
  --> 
   |
37 |             core::pin::Pin::new(&mut self.0).poll(cx)
   |                                      ^^^^ cannot borrow as mutable
   |
help: consider changing this to be mutable
   |
35 |         fn poll(mut self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Self::Output> {
   |                 +++

Applying the new suggestion, we get:

```rust
use tokio::sync::oneshot::{self};

pub struct JoinHandle<T>(oneshot::Receiver<T>);

impl<T> Future for JoinHandle<T> {
    type Output = Result<T, oneshot::error::RecvError>;

    fn poll(self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Self::Output> {
        std::pin::Pin::new(&mut self.0).poll(cx)
    }
}

which compiles, but leaves us exposed to the risk of pins being used "the wrong way".

pin-project is a utility commonly used when dealing with pin requirements around futures (there's a cool blog post on the matter).
There's also a similar, slimmer crate, pin-project-lite, which could be worth taking a look at.

@jkomyno jkomyno marked this pull request as ready for review November 15, 2023 19:54
@jkomyno jkomyno requested a review from a team as a code owner November 15, 2023 19:54
@jkomyno jkomyno requested review from Weakky and Druue and removed request for a team November 15, 2023 19:54
Copy link
Contributor

@miguelff miguelff left a comment

Choose a reason for hiding this comment

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

Nothing blocking

@@ -259,6 +269,7 @@ async fn execute_on<'a>(
query_schema: &'a QuerySchema,
trace_id: Option<String>,
) -> crate::Result<ResponseData> {
#[cfg(feature = "metrics")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't the metrics macros shimmed as the tracing macros where shimmed in query-connector?

Copy link
Contributor Author

@jkomyno jkomyno Nov 17, 2023

Choose a reason for hiding this comment

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

For convenience: in sql-query-connector, the trace! macro was used twice in a single file (src/database/operations/write.rs) among the files included for the wasm32 compilation.

That shim is effectively a shortcut to avoid adding a currently un-needed metrics feature flag in sql-query-connector.

In query-core, metrics usage relies on several different functions and on he query-engine-metrics crate (which doesn't compile on wasm32-*).

Should we want conditional compilation based on the target architecture, that should be added to the source (query-engine-metrics) rather than to the consumer (query-core). But since we decided not to care too much about metrics for the vertical slice, feature-gating metrics within query-core seemed the most adapt choice.

Comment on lines 136 to 196
pub(crate) mod task {
pub use arch::{spawn, JoinHandle};
use futures::Future;

// On native targets, `tokio::spawn` spawns a new asynchronous task.
#[cfg(not(target_arch = "wasm32"))]
mod arch {
use super::*;

pub type JoinHandle<T> = tokio::task::JoinHandle<T>;

pub fn spawn<T>(future: T) -> JoinHandle<T::Output>
where
T: Future + Send + 'static,
T::Output: Send + 'static,
{
tokio::spawn(future)
}
}

// On Wasm targets, `wasm_bindgen_futures::spawn_local` spawns a new asynchronous task.
#[cfg(target_arch = "wasm32")]
mod arch {
use super::*;
use tokio::sync::oneshot::{self};

// Wasm-compatible alternative to `tokio::task::JoinHandle<T>`.
// `pin_project` enables pin-projection and a `Pin`-compatible implementation of the `Future` trait.
#[pin_project::pin_project]
pub struct JoinHandle<T>(#[pin] oneshot::Receiver<T>);

impl<T> Future for JoinHandle<T> {
type Output = Result<T, oneshot::error::RecvError>;

fn poll(self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> std::task::Poll<Self::Output> {
// the `self.project()` method is provided by the `pin_project` macro
let receiver: std::pin::Pin<&mut oneshot::Receiver<T>> = self.project().0;
receiver.poll(cx)
}
}

impl<T> JoinHandle<T> {
pub fn abort(&mut self) {
// abort is noop on Wasm targets
}
}

pub fn spawn<T>(future: T) -> JoinHandle<T::Output>
where
T: Future + Send + 'static,
T::Output: Send + 'static,
{
let (sender, receiver) = oneshot::channel();
wasm_bindgen_futures::spawn_local(async move {
let result = future.await;
sender.send(result).ok();
});
JoinHandle(receiver)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to extract this into its own task.rs file?

Nice, clear conditional code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, done.

Base automatically changed from feat/sql-query-connector-on-wasm32-unknown-unknown to main November 17, 2023 21:23
@jkomyno jkomyno removed their assignment Nov 20, 2023
@jkomyno jkomyno merged commit ebb702b into main Nov 20, 2023
51 of 52 checks passed
@jkomyno jkomyno deleted the feat/query-core-on-wasm32-unknown-unknown branch November 20, 2023 03:20
janpio added a commit that referenced this pull request Jun 2, 2024
Lockfile was added as part of #4445 without equivalent package.json, current package.json was added later and does not seem connected to lockfile at all.
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