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

Various fixes and improvements #768

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Conversation

Pauan
Copy link
Collaborator

@Pauan Pauan commented Oct 11, 2023

  • Fixing the SDK so that it works with Webpack and Rollup and Chrome / Firefox extensions.
  • Adds in new create-aleo-app/template-extension example of using Aleo in a Chrome extension.
  • Adds in CI tests for template-extension and template-react-leo.
  • Fixes the CI so that it builds the website with the local packages (instead of the published packages).
  • Removing axios from the SDK, replacing it with a regular fetch.
  • Moving the thread spawning code from @aleohq/sdk into @aleohq/wasm.
  • The 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.
  • Updating documentation for initializeWasm and initThreadPool.

@onetrickwolf
Copy link
Collaborator

Deployed preview build here: https://build-fixes--aleo-tools-3481ab.netlify.app

Looking good to me so far!

Copy link
Collaborator

@onetrickwolf onetrickwolf left a 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";
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

wasm/rollup.config.js Show resolved Hide resolved
wasm/rollup.config.js Show resolved Hide resolved
@@ -1,4 +1,4 @@
import wasm from "../Cargo.toml";
import wasm from "../dist/wasm.js";
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

wasm/js/index.js Show resolved Hide resolved
sdk/src/utils.ts Show resolved Hide resolved
Comment on lines +6 to +7
background: "./src/background.js",
worker: "./src/worker.js",
Copy link
Collaborator

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?

Copy link
Collaborator Author

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" });
Copy link
Collaborator

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?

Copy link
Collaborator Author

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" });

Copy link
Collaborator

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.

.circleci/config.yml Show resolved Hide resolved
sdk/src/utils.ts Outdated Show resolved Hide resolved
@onetrickwolf
Copy link
Collaborator

@iamalwaysuncomfortable
Copy link
Collaborator

Thanks for all your help and explanations, we're good to merge

@onetrickwolf onetrickwolf merged commit ce7dc67 into ProvableHQ:testnet3 Oct 11, 2023
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