-
Notifications
You must be signed in to change notification settings - Fork 471
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
Various fixes and improvements #768
Conversation
Deployed preview build here: https://build-fixes--aleo-tools-3481ab.netlify.app Looking good to me so far! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -1,5 +1,5 @@ | |||
import CopyPlugin from "copy-webpack-plugin"; | |||
|
|||
import TerserPlugin from "terser-webpack-plugin"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this currently required or just an optimization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently required if you want to use minification. If you don't want minification then it's not required.
Note that terser is used by default in Webpack, this is just changing some settings for terser.
@@ -1,4 +1,4 @@ | |||
import wasm from "../Cargo.toml"; | |||
import wasm from "../dist/wasm.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know vaguely that this is intended to import a single wasm build, but can you quickly explain the underlying mechanics of what's happening here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both index.js
and worker.js
import the Cargo.toml
file.
Because they're importing the Cargo.toml
file, Rollup will then compile the Rust Wasm code.
Normally Rollup would just build the Rust Wasm one time, and share it with the index.js
and worker.js
files.
However, it's using multiple builds, a separate build for index.js
and worker.js
.
Because the builds are separate, that means Rollup will not share anything between the builds.
That means Rollup will compile the Rust Wasm code twice, and it will create 2 separate .wasm
files.
Since the Rust Wasm code takes approximately 10 eternities to compile, since it's compiled twice that means it will now take 20 eternities.
So my solution for that is to build the Rust Wasm in a separate Rollup build, which will output the Rust Wasm compiled code to dist/wasm.js
and dist/assets/aleo_wasm.wasm
.
After Rollup puts the compiled Rust Wasm into dist/wasm.js
, it then builds the index.js
and worker.js
files separately.
And the index.js
and worker.js
files can then import the dist/wasm.js
file (which contains the compiled Rust Wasm code).
So that way it only needs to build the Rust Wasm one time, and it can reuse the same .wasm
for both index.js
and worker.js
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, does that mean users can start multiple workers with
new Worker(new URL("worker.js", import.meta.url), { type: "module" });
as it's needed - say if they need to run two programs in parallel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's possible, though it won't actually increase parallelism, since the Rayon threads should be fully saturating the CPU.
So instead it would probably be better for the user to just use Promise.all
for concurrency within a single Worker:
const [a, b] = await Promise.all([
localProgramExecution(),
localProgramExecution(),
]);
That should also run both programs at the same time, but it's able to share a single Wasm instance + memory, and share a single Rayon thread pool, and it's able to share the private keys.
So this should be better than spawning multiple Workers.
background: "./src/background.js", | ||
worker: "./src/worker.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document what these are/what their function is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could, but it's super standard Chrome extension stuff, which is common knowledge for anybody creating a Chrome extension.
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/background
The sample is really simple, anybody who is familiar with Chrome extensions should understand it right away.
@@ -0,0 +1 @@ | |||
new Worker(new URL("worker.js", import.meta.url), { type: "module" }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the main worker that does all of the program execution? If an extension builder wanted to spin up another - how would that be done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the SDK must be run in a Worker, so this runs the SDK code in a Worker. The SDK code is in the worker.js
file.
I don't know why an extension would want to have multiple workers running SDK code, but if they wanted to they would just create a new worker2.js
file, add it into the Rollup config:
input: {
background: "./src/background.js",
worker: "./src/worker.js",
worker2: "./src/worker2.js",
},
And then spawn it in the background.js
file:
new Worker(new URL("worker.js", import.meta.url), { type: "module" });
new Worker(new URL("worker2.js", import.meta.url), { type: "module" });
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aleo functions are compute heavy so they might need to run multiple workers to execute multiple functions concurrently (say multiple transfers at once). Some wallets need to open these workers in popup windows.
Also there is the opposite case where Aleo wasm can run in the main thread because no executions or compute heavy tasks are being done.
Deployed latest changes: https://build-fixes-get-fix--aleo-tools-3481ab.netlify.app/account |
Thanks for all your help and explanations, we're good to merge |
create-aleo-app/template-extension
example of using Aleo in a Chrome extension.template-extension
andtemplate-react-leo
.website
with the local packages (instead of the published packages).axios
from the SDK, replacing it with a regularfetch
.@aleohq/sdk
into@aleohq/wasm
.initThreadPool
function can now be called with no arguments, which will cause it to spawn threads equal to the number of cores. This is the preferred way to use it.initializeWasm
andinitThreadPool
.