Decrypt keystores in a thread pool #5357
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
previously approved these changes
Apr 12, 2023
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts
Outdated
Show resolved
Hide resolved
|
the results importing 2000 keys on unstable: 19.7 keys/m with transferable objects: 100.5 keys/m without transferable objects: 102.2 keys/m |
nflaig
reviewed
Apr 15, 2023
There was a problem hiding this comment.
Issues found during testing
- incorrect initial log
info: 100% of keystores imported. current=0 total=2000 rate=0.00keys/m, should show 0 instead of 100% - keystore cache does not seem to work, the file is created after import but on next startup is removed and keys are loaded from scratch
|
I think we can do better than 1 decryption per thread at a time. ChatGPT was helpful here: -- The memory usage of Scrypt depends on the specific parameters chosen. These parameters include: The memory usage can be roughly calculated with the following formula: Memory Usage = 128 * N * r * p bytes For example, using the parameters N=2^14 (16,384), r=8, and p=1, the memory usage would be: Memory Usage = 128 * 16,384 * 8 * 1 = 16,777,216 bytes, or approximately 16 MB. -- The default used by the deposit cli is here Comes out to ~256MB. So since each thread has its own heap, we can really do quite a bit more decryptions at a time. Ofc taking into account (conservatively) the total amount of free memory available ( I'd like to see what this looks like and add some benchmarks here so we can keep track of this. |
👀 will look into that |
this happens due to the fact that It seems like the problem is just how we calculate the percentage here if current=0 (which is the default) then this will always result in 100 due to 0 being falsy > 0 && 2000 ? (0 / 2000) * 100 : 100
100 |
nflaig
reviewed
Apr 16, 2023
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts
Show resolved
Hide resolved
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts
Show resolved
Hide resolved
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/worker.ts
Show resolved
Hide resolved
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/types.ts
Outdated
Show resolved
Hide resolved
I believe there was a test for this case, if missed we should add it. |
|
@nflaig curious to see what your benchmark shows now after the latest commit |
It does not look great on my machine (or at least not better than before), it is overloaded which I think just causes bad overall performance (note: I am also running geth and several lodestar instance on that host)
This is the result of Those took around 30GB of RAM and put all CPUs under full load (also happened before) as you can see above, probably need to adjust the params somehow to be a bit more conservative. Just as reference,
It does look like memory utilization is better but does not seem to improve performance, I think CPU is just the bottleneck? This is the log output from latest commit, it looks like decrypt batches take too long now Some logs show negative rate for keys/m, likely related to how Problems mentioned earlier are fixed now
|
nflaig
reviewed
Apr 18, 2023
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/types.ts
Show resolved
Hide resolved
Ok will revert the last commit so we just have a job concurrency of 1.
Lets keep the top-level CLI flag for now (so as to not introduce a breaking change to users) and just use And explicitly assign |
nflaig
reviewed
Apr 20, 2023
There was a problem hiding this comment.
lgtm, just few final remarks after more in-depth review. Everything works as expected now and the performance boost is pretty amazing, on average it is now 5x faster and in combination with the keystore cache I think the UX for big operators is pretty good now
| const pool = Pool( | ||
| () => | ||
| spawn<DecryptKeystoreWorkerAPI>(new Worker("./worker.js"), { | ||
| // A Lodestar Node may do very expensive task at start blocking the event loop and causing |
There was a problem hiding this comment.
This comment seems a bit out of place, should probably be revised. In case of loading the keys there is really nothing else going on as this is a blocking operation
packages/cli/src/cmds/validator/keymanager/decryptKeystoreDefinitions/index.ts
Outdated
Show resolved
Hide resolved
| if (typeof navigator !== "undefined") { | ||
| defaultPoolSize = navigator.hardwareConcurrency ?? 4; | ||
| } else { | ||
| defaultPoolSize = (await import("node:os")).cpus().length; |
There was a problem hiding this comment.
noting here that we should change this line to use os.availableParallelism() once we upgrade to node v20
| /** | ||
| * Decrypt a single keystore definition, returning the secret key as a Uint8Array | ||
| * | ||
| * NOTE: This is memory-intensive process, since decrypting the keystore involves running a key derivation function (either pbkdf2 or scrypt) |
There was a problem hiding this comment.
this note is correct but based on the performance tests it looks like CPU is rather the bottleneck that prevents us from increasing the job concurrency to further speed up the key import
With job concurrency of 1
packages/cli/test/unit/validator/decryptKeystoreDefinitions.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Nico Flaig <[email protected]>
nflaig
approved these changes
Apr 20, 2023
|
@wemeetagain I noticed that we have a similar logic in the keymanager API. Is supporting thread pool decryption something we should consider there as well or do we expect that this API will generally be used to import less overall keys? |
|
Probably wouldn't hurt to support decryption in parallel here. 🤷 |
Yes definitely, that should be done in a thread and limit with a queue to prevent killing the vc process |
|
🎉 This PR is included in v1.8.0 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Resolves #4179
Description
decryptKeystoreDefinitionsfunction