-
Notifications
You must be signed in to change notification settings - Fork 531
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
[Do Not Merge] optimize memory consumption on bootstrap #16201
base: compatible
Are you sure you want to change the base?
[Do Not Merge] optimize memory consumption on bootstrap #16201
Conversation
@@ -960,12 +960,30 @@ module For_tests = struct | |||
let consensus_constants = precomputed_values.consensus_constants | |||
end | |||
|
|||
module T = Transaction_snark.Make (struct |
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.
Although it is located in the For_unit_tests
module, I suspect it can be initialized even by the production code if it's somehow referenced by some function somewhere.
I'm not sure if making the module is an expensive operation, but I'd suggest:
- Making it singleton for usage in unit tests (this would also reduce the code duplication)
- Measure the time to initialize it, and if it's non-negligble, surround with some laziness
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.
Previously initialization would happen from within a separate process, only when it's launched. Now it will happen if the module that contains this is somehow referenced by some code that is invoked in initialization, and it's possible that this is executed every time.
Once again, I'm not sure if making a functor is expensive itself or it is cheap, just generating a bunch of lazy values.
Lazy.force B.Proof.verification_key ) | ||
|
||
let get_blockchain_verification_key () = Lazy.force vk | ||
let get_blockchain_verification_key () = Deferred.return verification_key |
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.
Maybe we could just remove this method?
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.
oh yes, forgot to do it
@@ -21,6 +21,7 @@ roots: [ | |||
"js_of_ocaml-toplevel.4.0.0" | |||
"lmdb.1.0" | |||
"menhir.20210419" | |||
"memtrace_viewer.0.16.0" |
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.
Was this dependency used somehow?
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.
only locally for memory trace. i will remove it
src/lib/verifier/for_test.ml
Outdated
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'm still unsure if this is purely for tests purposes. Maybe we should also reuse part of this method in prover ? then there will be a single place where we create BlockhainSnark and TransactionSnark modules. Prover bootstrap code is anyhow executed in mina main thread.
@@ -56,6 +56,9 @@ module Worker_state = struct | |||
val toggle_internal_tracing : bool -> unit | |||
|
|||
val set_itn_logger_data : daemon_port:int -> unit | |||
|
|||
val get_blockchain_verification_key : |
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 wonder also what to do in Check | No_check modes. Where we return dummy keys. Maybe this method should return Pickles.Verification_key.t Deferred.t
but then in Mina_grahpql modules we are exposing Verifiaction key from verifier so we would need to either break the interface and also there wrap repsone in optional or handle returning diummy as well
dd5ef45
to
081b26d
Compare
Experimental branch for optimizing memory usage at mina bootstrap