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

Offscreen API #303

Merged
merged 35 commits into from
Dec 24, 2023
Merged

Offscreen API #303

merged 35 commits into from
Dec 24, 2023

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Dec 14, 2023

References #146

This PR supports building transactions in the web in parallel using web-workers! Additionally, the serial build method has been deprecated.

The output console logs can be monitored by navigating to chrome://extensions, clicking details, and then inspect view.

@TalDerei TalDerei marked this pull request as draft December 14, 2023 08:14
@TalDerei TalDerei self-assigned this Dec 14, 2023
@TalDerei
Copy link
Contributor Author

TalDerei commented Dec 14, 2023

Tackling some internal complications with spawning workers in a service-worker context, but should be ready for review soon. We'll need to additionally consider fallback scenarios and the proper error handling / propagation (reference #295?) for serial builds.

@TalDerei
Copy link
Contributor Author

TalDerei commented Dec 16, 2023

@grod220 building the actions in parallel in the offscreen document and passing the prebuilt vector of actions back to the extension is working, but calls to the build_parallel method is failing. It's throwing "Error: invalid type: byte array, expected a base64 string". Think it's a protobuf change related error?

@grod220
Copy link
Contributor

grod220 commented Dec 16, 2023

Ah, you've run into our classic wasm serialization issue. We've each run into this independently 😅 . Think we should document this somewhere.

When crossing the javascript->wasm boundary, you can pass javascript objects. However, it's not a guarantee that serde can deserialize it into structs. For that to be successful, it has to be in json format (protobuf rules though). Bufbuild provides the helper .toJson() on our proto-classes to get in this specific format. So try doing this for each argument like so:

const transaction = build_parallel(
    batchActions.map(action => action.toJson()), 
    req.transactionPlan.toJson(), 
    witnessData.toJson(), 
    req.authorizationData.toJson()
  )

@TalDerei
Copy link
Contributor Author

TalDerei commented Dec 17, 2023

Benchmarks on my machine (2020 M1 Mac, 8-core CPU and GPU) for a typical transaction (2 spend and 2 output actions):
- Parallel transaction: 12s build + 6s broadcast = ~18s total
- Serial transaction: 30s build + 6s broadcast = ~36s total

The broadcasting operation viewClient.broadcastTransaction is currently taking around 1/3 of the time for constructing a transaction in parallel. @grod220 @hdevalence Can the perceived time of the broadcasting be somehow reduced?

In terms of outstanding work before this PR can be merged, @turbocrime we should implement a fallback mechanism for serial builds in cases where web workers are not supported. One idea is to introduce a global variable like webWorkersSupported and check against that. Where should this ideally be included considering the changes in #255?

We can further consider load-balancing the workload distributed to the web-workers using a queue when the number of ActionPlan exceeds the number of logical CPU cores on the device. Alternatively, spawning more web-workers than the available cores leads to excessive context switching and resource contention, which degrades the performance.

@TalDerei TalDerei requested review from grod220 and turbocrime and removed request for grod220 December 17, 2023 06:24
@hdevalence
Copy link
Member

The "broadcast" step in the web code is currently a hardcoded 6s timeout. Two points:

  1. The hardcoded timeout is a stand-in for the logic we have in the Rust view server, where we await detection of the transaction during sync, closing the loop on the broadcast (not just waiting for "did the server say it was accepted" but "did it end up in a block we scanned"). This means the time will be between ~0 and ~5s, depending on when the transaction was broadcast relative to the block interval.
  2. For this reason, I think it makes sense to expose this to users as a different phase ("Building Transaction..." vs "Broadcasting Transaction...") in the UI.

@grod220 is currently working on replacing the hardcoded timeout with detection logic.

@hdevalence
Copy link
Member

Going from 30s build time to 12s build time is amazing!

If we can also cut down some of the wasm overhead, we're stacking improvements that get us pretty close to the ideal scenario of being able to build the transaction in the time it takes the user to review the transaction in the approval dialog.

@TalDerei
Copy link
Contributor Author

The "broadcast" step in the web code is currently a hardcoded 6s timeout. Two points:

  1. The hardcoded timeout is a stand-in for the logic we have in the Rust view server, where we await detection of the transaction during sync, closing the loop on the broadcast (not just waiting for "did the server say it was accepted" but "did it end up in a block we scanned"). This means the time will be between ~0 and ~5s, depending on when the transaction was broadcast relative to the block interval.
  2. For this reason, I think it makes sense to expose this to users as a different phase ("Building Transaction..." vs "Broadcasting Transaction...") in the UI.

@grod220 is currently working on replacing the hardcoded timeout with detection logic.

ah I was wondering why it was hard-coded specifically to 6s!

@TalDerei
Copy link
Contributor Author

If we can also cut down some of the wasm overhead, we're stacking improvements that get us pretty close to the ideal scenario of being able to build the transaction in the time it takes the user to review the transaction in the approval dialog.

Agreed #113 shouldn't be blocked for much longer.

@hdevalence
Copy link
Member

In terms of outstanding work before this PR can be merged, @turbocrime we should implement a fallback mechanism for serial builds in cases where web workers are not supported. One idea is to introduce a global variable like webWorkersSupported and check against that.

In what case would web workers not be supported?

@TalDerei
Copy link
Contributor Author

TalDerei commented Dec 17, 2023

In what case would web workers not be supported?

It's generally a good idea to check for web worker support (https://www.w3schools.com/html/html5_webworkers.asp). I referenced this chart (https://caniuse.com/webworkers), yet it suggests that 98% of devices wouldn't have an issue.

@TalDerei
Copy link
Contributor Author

TalDerei commented Dec 17, 2023

@hdevalence If we don't implement the check, there's no reason to keep the serial build method in the web code, which means we can further reduce the WASM binary size from 8.5 MB to 6.2 MB.

@hdevalence
Copy link
Member

From the caniuse.com page, we can unconditionally use web workers, since we're only targeting Chrome

@TalDerei TalDerei requested a review from turbocrime December 19, 2023 18:17
@turbocrime
Copy link
Contributor

hey, i think have quite a lot of feedback on the event and message flow, so i'm just going to add some commits to this pr. we should pair when possible

@TalDerei
Copy link
Contributor Author

TalDerei commented Dec 20, 2023

hey, i think have quite a lot of feedback on the event and message flow, so i'm just going to add some commits to this pr. we should pair when possible

I paired with Gabe yesterday, and am currently refactoring the underlying serialization formats as JsonValue. Let's pair later about the event and message flow.

TalDerei and others added 3 commits December 22, 2023 10:04
* messy wip

* cool wip

* pair L + Tal changes

* satisfy linter

* removed timers

* fix action length

* add comments

---------

Co-authored-by: turbocrime <[email protected]>
apps/extension/src/routes/offscreen/offscreen.ts Outdated Show resolved Hide resolved
apps/extension/src/routes/offscreen/web-worker.ts Outdated Show resolved Hide resolved
apps/extension/src/web-worker.ts Outdated Show resolved Hide resolved
apps/extension/tsconfig.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
apps/extension/src/web-worker.ts Outdated Show resolved Hide resolved
packages/wasm/src/utils.ts Outdated Show resolved Hide resolved
apps/extension/src/web-worker.ts Outdated Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Outdated Show resolved Hide resolved
apps/extension/webpack/webpack.common.js Show resolved Hide resolved
packages/wasm/src/build.ts Outdated Show resolved Hide resolved
packages/wasm/src/build.ts Outdated Show resolved Hide resolved
packages/types/src/transaction/schema.ts Outdated Show resolved Hide resolved
apps/extension/src/offscreen.ts Outdated Show resolved Hide resolved
apps/extension/src/offscreen.ts Outdated Show resolved Hide resolved
apps/extension/src/wasm-task.ts Outdated Show resolved Hide resolved
apps/extension/src/wasm-task.ts Outdated Show resolved Hide resolved
apps/extension/src/wasm-task.ts Outdated Show resolved Hide resolved
apps/extension/src/wasm-task.ts Outdated Show resolved Hide resolved
apps/extension/src/wasm-task.ts Outdated Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Outdated Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Outdated Show resolved Hide resolved
packages/types/src/indexed-db.ts Outdated Show resolved Hide resolved
@TalDerei
Copy link
Contributor Author

I think we're close to be able to merge once the last few comments are addressed.

@TalDerei
Copy link
Contributor Author

TalDerei commented Dec 24, 2023

The offscreen lifecycle management is sufficient for now. In a separate issue, we'll make it more complete and enable servicing multiple simultaneous requests.

Copy link
Contributor

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

Great job! Have some remaining points from me. Really excited to get this in though. After this (and Christmas), we should do a release and get users on the new fast builds.

apps/extension/src/offscreen.ts Show resolved Hide resolved
apps/extension/src/wasm-task.ts Outdated Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Outdated Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Outdated Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Outdated Show resolved Hide resolved
packages/types/src/internal-msg/offscreen.ts Outdated Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Outdated Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Show resolved Hide resolved
packages/router/src/grpc/offscreen-client.ts Outdated Show resolved Hide resolved
@TalDerei TalDerei merged commit 5834251 into main Dec 24, 2023
3 checks passed
@turbocrime turbocrime deleted the offscreen-api branch December 29, 2023 09:36
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.

4 participants