Reduce loading time #116

mitschabaude · 2023-08-16T18:26:45Z

mitschabaude

some explanations for reviewers!

mitschabaude · 2023-08-21T14:40:47Z

crypto/bindings.ts

+};
+
+// this is put in a global variable so that ../kimchi/js/bindings.js finds it
+(globalThis as any).__snarkyTsBindings = tsBindings;


this is the new TS bindings file, explained here

Nit: Depending how you feel on comments, you could argue the explanation could be added here, mostly just elaborating on the inline comment you have already. Could be helpful for others look at this file and wondering more, wdyt?

mitschabaude · 2023-08-21T14:41:37Z

crypto/constants.ts

+    "17890046087060600378523962727948914890955845280331606421027576706277370812052",
+    "2013454333407534591106411126192774509000627947643864003868625362325108814274",
+    "11843535353803037930790320689348405921362689396018494249827525941843824404081"
+  ],


hard-coded hashes exposed to jsoo via the new TS bindings. we hard-code them to avoid computing them at the top level

Nit: If we could throw some inline comments when this (or similar) file(s) is generated that point to the OCaml file, it would really help context for other developers if debugging or adding features in this area.

mitschabaude · 2023-08-21T14:42:04Z

kimchi/js/bindings.js

+// Provides: tsBindings
+var tsBindings = globalThis.__snarkyTsBindings;
+
+// Provides: getTsBindings
+// Requires: tsBindings
+function getTsBindings() {
+    return tsBindings;
+}


expose new bindings file to jsoo

mitschabaude · 2023-08-21T14:43:13Z

ocaml/snarky_js_constants.ml

+let prefix_hashes =
+  let open Hash_prefixes in
+  `Assoc
+    (List.map ~f:(prefix_hash_entry Kimchi)
+       [ (receipt_chain_user_command :> string)
+       ; (receipt_chain_zkapp :> string)


these changes are about computing some hashes at compile time and put them in the constants.ts file

A great addition! Just wondering, is there any documentation that links these constants to this file? Going through some READMEs and I don't see an explicit mention of this. I think it would really help with we could point to a piece of documentation that establishes this sort of generation. Doesn't have to be in this PR but even inline comments would help. Wdyt?

mitschabaude · 2023-08-21T14:45:51Z

js/web/web-backend.js


 async function initSnarkyJS() {
  const memory = allocateWasmMemoryForUserAgent(navigator.userAgent);
  await init(undefined, memory);

  let module = init.__wbindgen_wasm_module;
-  let numWorkers = await getEfficientNumWorkers();


change 1 to this file: we don't call getEfficientNumWorkers() right away, because it's very expensive (inspects the device GPU via WebGL). Instead, we call it before we need workers the first time, which is likely a slow operation anyway

Great optimization! Great for cases where we only need constants since that can all be done in JS, right?

mitschabaude · 2023-08-21T14:48:57Z

js/web/web-backend.js

-
-  worker = inlineWorker(srcFromFunctionModule(mainWorker));
-  await workerCall(worker, 'start', { memory, module, numWorkers });
-  globalThis.plonk_wasm = overrideBindings(wasm, worker);


change 2 to this file: creating the main worker and waiting for its promise to return (which is after all its child workers are started) is very slow. Therefore, we do this in a way that is not blocking our top-level await. This is fine, the main worker will only be needed when we do our first expensive operation and we await the promise at that time

mitschabaude · 2023-08-21T14:49:58Z

ocaml/lib/consistency_test.ml

-    Account_update.Graphql_repr.deriver @@ Fields_derivers_zkapps.Derivers.o ()
+    lazy
+      ( Account_update.Graphql_repr.deriver
+      @@ Fields_derivers_zkapps.Derivers.o () )


we compute a bunch of stuff lazily in consistency_test.ml and local_ledger.ml which was computed at the top level before

mitschabaude · 2023-08-21T14:51:02Z

ocaml/lib/snarky_bindings.ml

-    Sponge.Params.(
-      map pasta_p_kimchi
-        ~f:(Fn.compose Impl.Field.constant Impl.Field.Constant.of_string))
+  let sponge_params = Kimchi_pasta_basic.poseidon_params_fp


don't recompute stuff that is computed in elsewhere already

MartinMinkov

Great use of lazy and compiling prefixes ahead of time instead of hitting runtime cost!

I mostly just had comments about documentation, feel free to address or make issues for later

MartinMinkov · 2023-08-21T16:57:06Z

ocaml/snarky_js_constants.ml

+let prefix_hashes =
+  let open Hash_prefixes in
+  `Assoc
+    (List.map ~f:(prefix_hash_entry Kimchi)
+       [ (receipt_chain_user_command :> string)
+       ; (receipt_chain_zkapp :> string)


A great addition! Just wondering, is there any documentation that links these constants to this file? Going through some READMEs and I don't see an explicit mention of this. I think it would really help with we could point to a piece of documentation that establishes this sort of generation. Doesn't have to be in this PR but even inline comments would help. Wdyt?

MartinMinkov · 2023-08-21T17:07:40Z

js/web/web-backend.js


 async function initSnarkyJS() {
  const memory = allocateWasmMemoryForUserAgent(navigator.userAgent);
  await init(undefined, memory);

  let module = init.__wbindgen_wasm_module;
-  let numWorkers = await getEfficientNumWorkers();


Great optimization! Great for cases where we only need constants since that can all be done in JS, right?

MartinMinkov · 2023-08-21T17:10:45Z

crypto/constants.ts

+    "17890046087060600378523962727948914890955845280331606421027576706277370812052",
+    "2013454333407534591106411126192774509000627947643864003868625362325108814274",
+    "11843535353803037930790320689348405921362689396018494249827525941843824404081"
+  ],


Nit: If we could throw some inline comments when this (or similar) file(s) is generated that point to the OCaml file, it would really help context for other developers if debugging or adding features in this area.

MartinMinkov · 2023-08-21T17:14:49Z

crypto/bindings.ts

+};
+
+// this is put in a global variable so that ../kimchi/js/bindings.js finds it
+(globalThis as any).__snarkyTsBindings = tsBindings;


Nit: Depending how you feel on comments, you could argue the explanation could be added here, mostly just elaborating on the inline comment you have already. Could be helpful for others look at this file and wondering more, wdyt?

Trivo25 · 2023-08-21T18:00:16Z

ocaml/dune

@@ -72,6 +72,7 @@
 (libraries
  mina_base
  core_kernel
+  base


do you know how much base impacts the size? is it a lot? 🤔 i wonder how smart JSOO handles these things

In general terms, jsoo does proper tree-shaking, but often OCaml modules contain methods that aren't used and those (I believe) can't be compiled away, similar to if you'd populate a JS class with an unused method.

Concretely, this new import only affects the snarky_js_constants.exe executable which we don't even compile to JS. It's an OCaml script, which is run to print out the constants.ts file

Trivo25 · 2023-08-21T18:02:06Z

ocaml/lib/dune

@@ -58,6 +58,7 @@
  ;; js-specific overrides ;;
  cache_dir.fake
  digestif.ocaml
+  hash_prefix_create.js


oh you can just import js file like that :o

no that's an ocaml library which is suffixed with .js lol

oh i was confused for a second! lol

mitschabaude added 3 commits August 16, 2023 20:14

expose new ts bindings file to jsoo

34704ed

use existing converted poseidon params

f987821

get number of workers only when needed (bc expensive)

7e2696b

mitschabaude mentioned this pull request Aug 16, 2023

Reduce loading time o1-labs/o1js#1073

Merged

mitschabaude added 9 commits August 17, 2023 02:38

start main worker in a non-blocking way

b0e0d3d

generate prefix hashes at compile time and bundle

7764a03

add more prefixes

f7a6983

don't create example payment at the top level

4d4d958

lazy account update deriver

79ffc5e

more lazy derivers

5f348d7

minor

5f11259

fix web backend

85be5ef

update bindings

430b9df

mitschabaude force-pushed the perf/loading-time branch from 6b724db to 430b9df Compare August 18, 2023 01:20

mitschabaude marked this pull request as ready for review August 18, 2023 01:25

mitschabaude requested a review from a team as a code owner August 18, 2023 01:25

mitschabaude mentioned this pull request Aug 21, 2023

Reduce loading time - BERKELEY #119

Closed

mitschabaude commented Aug 21, 2023

View reviewed changes

mitschabaude mentioned this pull request Aug 21, 2023

Snarkyjs: Reduce loading time MinaProtocol/mina#13878

Closed

MartinMinkov approved these changes Aug 21, 2023

View reviewed changes

mrmr1993 approved these changes Aug 21, 2023

View reviewed changes

Trivo25 approved these changes Aug 21, 2023

View reviewed changes

mitschabaude added 2 commits August 23, 2023 08:42

update bindings

afa5ef7

address feedback - add documentation in comments

7abec27

mitschabaude mentioned this pull request Aug 23, 2023

Reduce snarkyjs loading time - DEVELOP #124

Merged

mitschabaude merged commit 549fd78 into main Aug 28, 2023

mitschabaude deleted the perf/loading-time branch August 28, 2023 19:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce loading time #116

Reduce loading time #116

mitschabaude commented Aug 16, 2023 •

edited

Loading

mitschabaude left a comment

mitschabaude Aug 21, 2023

MartinMinkov Aug 21, 2023

mitschabaude Aug 21, 2023

MartinMinkov Aug 21, 2023

mitschabaude Aug 21, 2023

mitschabaude Aug 21, 2023

MartinMinkov Aug 21, 2023

mitschabaude Aug 21, 2023

MartinMinkov Aug 21, 2023

mitschabaude Aug 21, 2023

mitschabaude Aug 21, 2023

mitschabaude Aug 21, 2023

mitschabaude Aug 21, 2023

MartinMinkov left a comment

MartinMinkov Aug 21, 2023

MartinMinkov Aug 21, 2023

MartinMinkov Aug 21, 2023

MartinMinkov Aug 21, 2023

Trivo25 Aug 21, 2023

mitschabaude Aug 21, 2023

Trivo25 Aug 21, 2023

mitschabaude Aug 21, 2023

Trivo25 Aug 21, 2023

Reduce loading time #116

Reduce loading time #116

Conversation

mitschabaude commented Aug 16, 2023 • edited Loading

mitschabaude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinMinkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitschabaude commented Aug 16, 2023 •

edited

Loading