energywebfoundation / iam-client-lib

TypeScript library to be used within decentralised applications for authentication and authorisation using DIDs (Decentralised Identifiers) and VCs (Verifiable Credentials)
GNU General Public License v3.0
19 stars 9 forks source link

task/ICL-323 high resource usage of importing iam-lib #595

Closed JGiter closed 2 years ago

JGiter commented 2 years ago

Description

Contributor checklist

Harasz commented 2 years ago

I think that this fix will only fix an error during initialization. But problem will still remain when will try to use VC features, so during issuing claim request and spike will be always when invoking one of the VC features

JGiter commented 2 years ago

I think that this fix will only fix an error during initialization. But problem will still remain when will try to use VC features, so during issuing claim request and spike will be always when invoking one of the VC features

From my opinion high usage was caused by blocking cpu on reading from disk. Now reading is asynchronous and Node.js will be able to yield control from io operation

Harasz commented 2 years ago

From my opinion high usage was caused by blocking cpu on reading from disk. Now reading is asynchronous and Node.js will be able to yield control from io operation

I have to test it and see if it really is

Harasz commented 2 years ago

Issue still occurs, but i think it's not that big at it was before: 160MB and 400%CPU, previously it was 800MB. Hovewever issue occurs every time you use one of the features of VC module @JGiter @jrhender

JGiter commented 2 years ago

Issue still occurs, but i think it's not that big at it was before: 160MB and 400%CPU, previously it was 800MB. Hovewever issue occurs every time you use one of the features of VC module @JGiter @jrhender

You are right. Issue still remains, though there is some difference. Here test of verifiable credentials with static and dynamic import image image

jrhender commented 2 years ago

@JGiter I'm confused by what your graph is comparing? Is the top the behaviour without this PR and the bottom is the behaviour with the PR?

You mentioned "static and dynamic import", but I believe both with this PR and before this PR, "dynamic" imports are being used. "Dynamic import" just refers to the use of import() at runtime, right?

JGiter commented 2 years ago

@JGiter I'm confused by what your graph is comparing? Is the top the behaviour without this PR and the bottom is the behaviour with the PR?

The top graph is testing with dynamic import, the bottom - with static. As you can see the duration of peak load is twice less with dynamic import

You mentioned "static and dynamic import", but I believe both with this PR and before this PR, "dynamic" imports are being used. "Dynamic import" just refers to the use of import() at runtime, right?

Now we are statically importing members from didkit, for example here https://github.com/energywebfoundation/iam-client-lib/blob/9fd459e9d3b8d092880adc17065e2651d0dca812/src/modules/verifiable-credentials/verifiable-credentials-node.service.ts#L6

jrhender commented 2 years ago

@JGiter

You mentioned "static and dynamic import", but I believe both with this PR and before this PR, "dynamic" imports are being used. "Dynamic import" just refers to the use of import() at runtime, right?

Could you clarify which code each of your graphs was executing? Is one graph the current develop branch and one graph is this PR branch? Or if it is some different code that was executed, could you point to that code please?

JGiter commented 2 years ago

@jrhender

Could you clarify which code each of your graphs was executing? Is one graph the current develop branch and one graph is this PR branch? Or if it is some different code that was executed, could you point to that code please?

Top graph is result of execution of code in this PR, the bottom graph corresponds to develop

jrhender commented 2 years ago

@JGiter

Top graph is result of execution of code in this PR, the bottom graph corresponds to develop

Ok, so I think in both cases "dynamic" imports were being used, right?

In this PR branch: https://github.com/energywebfoundation/iam-client-lib/blob/bf8e6edf8cb066f09e44be0cd0fdb05cf76b6788/src/modules/verifiable-credentials/index.ts#L15 and https://github.com/energywebfoundation/iam-client-lib/blob/bf8e6edf8cb066f09e44be0cd0fdb05cf76b6788/src/modules/verifiable-credentials/verifiable-credentials-node.service.ts#L16

In develop branch: https://github.com/energywebfoundation/iam-client-lib/blob/fcbd34e8abe6fb79ae440b5a5c3646ae4731aa5d/src/modules/verifiable-credentials/index.ts#L15

So I am confused by what you mean by the top graph being "with static imports". If it was using the code from this PR, then it is still using dynamic imports, right?

JGiter commented 2 years ago

@jrhender

So I am confused by what you mean by the top graph being "with static imports". If it was using the code from this PR, then it is still using dynamic imports, right?

Ah, I see. Indeed, verifiable-credential is imported dynamically in both branches. But verifiable-credential doesn't contain resource intensive operations, so I think the way of importing of vc is not important. This PR is differs from develop by importing didkit. This import is static in develop, so it is occurs when iam is imported through iam index.ts -> modules/verifiable-credentials -> VerifiableCredentialServiceWeb

JGiter commented 2 years ago

@jrhender Should we close this?

jrhender commented 2 years ago

@JGiter yes, I think so, thanks