-
Notifications
You must be signed in to change notification settings - Fork 118
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
Start workers only when needed #872
Conversation
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.
some comments to help with reviewing!
@@ -33,12 +33,13 @@ | |||
"type-check": "tsc --noEmit", | |||
"dev": "npx tsc -p tsconfig.node.json && cp src/snarky.d.ts dist/node/snarky.d.ts", | |||
"make": "make -C ../../.. snarkyjs", | |||
"bindings": "cd ../../.. && ./scripts/update-snarkyjs-bindings.sh && cd src/lib/snarkyjs", |
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.
handy new script to update the bindings
@@ -81,7 +82,6 @@ | |||
"dependencies": { | |||
"blakejs": "1.2.1", | |||
"detect-gpu": "^5.0.5", | |||
"env": "^0.0.2", |
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.
finally getting rid of this stupid dependency
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.
Hey, I also want to contribute to this project, Any pre-requisites, I have decent experience with Solidity and Javascript
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.
Yeah sure, JS experience is enough to start contributing! Maybe you find something you can do labeled with good first issue
.
But if you notice something else that should be improved, were always happy to take suggestions! Also, what snarkyjs needs are more community-maintained libraries
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.
thank you for your interest @0xvicky
see https://github.com/o1-labs/snarkyjs/blob/main/CONTRIBUTING.md
@@ -60,12 +59,12 @@ function makeNodeModulesExternal() { | |||
} | |||
|
|||
function makeJsooExternal() { | |||
let isJsoo = /bc.cjs$/; | |||
let isJsoo = /(bc.cjs|plonk_wasm.cjs)$/; |
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.
unrelated: this PR fixes the commonjs version of snarkyjs, which seems to have been broken for a while
@@ -9,7 +9,7 @@ import glob from 'glob'; | |||
export { buildWeb }; | |||
|
|||
const entry = './src/index.ts'; | |||
const target = 'es2021'; | |||
const target = 'es2022'; |
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.
bump target because we use top level await
@@ -0,0 +1,27 @@ | |||
import fs from 'node:fs/promises'; |
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.
this file is a new script that is part of the build and patches the wasm-bindgen output
expect(tx.send()).rejects.toThrow('Token_owner_not_caller'); | ||
|
||
shutdown(); | ||
await expect(tx.send()).rejects.toThrow( | ||
'can not use or pass on token permissions' | ||
); |
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.
these tests were actually not working 🤦🏻♂️ because expect.rejects.toThrow
is an async function, and wasn't awaited before calling shutdown, it never finished
@@ -0,0 +1,221 @@ | |||
import { snarkContext } from './proof_system.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.
new file for Circuit
which wraps SnarkyCircuit
which comes from OCaml
@@ -374,97 +382,6 @@ function matrixProp<T>( | |||
}; | |||
} | |||
|
|||
function public_(target: any, _key: string | symbol, index: number) { |
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.
code here moved to circuit.ts
/** | ||
* Adds a constraint to the circuit. | ||
*/ | ||
static addConstraint( |
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.
turns out that all these deleted methods never actually existed on the Circuit
class 🤦🏻♂️
let isReady = Promise.resolve(); | ||
let isItReady = () => isReadyBoolean; | ||
|
||
function shutdown() {} |
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.
isReady and shutdown are noops, only kept for backwards compatibility during deprecation period
This PR refactors how SnarkyJS uses web workers / Node.js worker threads. It thereby significantly reduces memory consumed by SnarkyJS. As a welcome side effect, we get rid of having to call
await isReady
andshutdown()
before / after using SnarkyJS.Examples of memory reduction with 16 threads in Node.js:
src/examples/simple_zkapp.ts
)src/examples/zkapps/dex/happy-path-with-proofs.ts
)This was achieved because workers are no longer running by default -- they are only started when compiling or proving, and shut down after that. In addition, we achieved a reduction in memory consumption of each worker in the Node.js version, by reducing the JS dependency graph of each worker down to a minimum.
closes #674
closes #696
closes #609
mina: MinaProtocol/mina#13102
snarkyjs-bindings: o1-labs/o1js-bindings#1
main changes:
src/snarky
down into snarkyjs-bindingswithThreadPool()
function which wraps all operations that need workers. it takes care of starting and shutting down workers before/after the wrapped function.withThreadPool()
is used to wrap compile / prove / verify both inproof_system.ts
(wrapping Pickles / SmartContract methods) and in the newcircuit.ts
(wrapping plain kimchi /Circuit
methods)Circuit
, we had to lift its API surface from OCaml to TS; lives in a newcircuit.ts
file; prove/compile/verify become async, becausewithThreadPool()
is asyncshutdown()
andisReady
which become no-opsmore details are in the PR comments below!