Feature/workers optimised #256
Conversation
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
Signed-off-by: sivanov <[email protected]>
ivanovSPvirtru
force-pushed
the
feature/workers_unoptimised
branch
from
f3c4784
to
223e9a0
Compare
|
SonarCloud Quality Gate failed.
|
| @@ -981,6 +982,10 @@ export async function sliceAndDecrypt({ | |||
| cryptoService: CryptoService; | |||
| segmentIntegrityAlgorithm: IntegrityAlgorithm; | |||
| }) { | |||
| const limit = pLimit(navigator.hardwareConcurrency || 4); // save fallback number | |||
There was a problem hiding this comment.
We might want to make this customizable by the caller somehow? (See also: chunksInOneDownload)
There was a problem hiding this comment.
I dont think we do tbh. Ive benchmarked it tried multiple configurations. More plimit with hardwareConcurrency amount of workers, vice-verca, more both of them. What i got:
- number of Workers more than amount of cores really didnt bosted performance
- pLimit ether didnt had any effect ether were so big that gained small improvement but froze ui
|
|
||
| const workerBlob = new Blob([workerScript], { type: 'application/javascript' }); | ||
| const workerUrl = URL.createObjectURL(workerBlob); | ||
| const workersArray: Worker[] = Array.from({ length: maxWorkers }, () => new Worker(workerUrl)); |
There was a problem hiding this comment.
Ideally we'd be doing this at build time, not runtime. Do we have any methods for doing this? I can't find a good way to do this without doing a two-pass build.
There was a problem hiding this comment.
This code gets executate in the instance when we import entry point of sdk to any frontend app. Because this file eventually gets imported from entry point further down the nodes. And imports are synchronous. ANd code above too.
I dont think it could be initiated earlier that that in js world tbh
There was a problem hiding this comment.
Ideally the bundler will do all this work for you can you can treat this like a dynamic import almost. It looks like both vitejs and webpack 5 support this:
https://vitejs.dev/guide/features.html#web-workers
https://webpack.js.org/guides/web-workers/
I guess the question is are they smart enough to work through all the intermediate translations we do. Most notably, virtru-sdk uses webpack5 to bundle, and then secure-lib.js loads that (webpack 5 again) as does secure-share (webpack 5 as well), but our only test library here uses vite.
There was a problem hiding this comment.
Oh yes i tried it and i had constant problems on a app side. Ive tuned it on Virtru-sdk side but after webpack on secure share it had constant errors. Spend a wile on that.
Thats why i decided to go with this aproach. Its makes sdk independent of bundlers issues and doesnt slow down performance, since its a singleton instance that initializes workers as soon as sdk imported it. So when youll start download stuff its already be there.
| pop: () => Promise<Worker>; | ||
| } | ||
|
|
||
| const workersQueue: WorkersQueue = { |
There was a problem hiding this comment.
Can you add some test coverage for this?
No description provided.