-
Notifications
You must be signed in to change notification settings - Fork 28
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
Improve file opening times #1828
Improve file opening times #1828
Conversation
|
||
const preselectedAttributionsToResources: AttributionsToResources = {}; | ||
for (const resourceId of Object.keys(resourcesToExternalAttributions)) { | ||
const attributionIds = resourcesToExternalAttributions[resourceId]; |
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.
This would be faster if done in the FE, where we have already attributionsToResources. But it isn't worth considering big architecture changes for something that happens once per file.
const preselectedExternalAttributions = Object.fromEntries( | ||
Object.entries(externalAttributionsCopy).filter(([, packageInfo]) => { | ||
|
||
const preselectedAttributions: Attributions = {}; |
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 would have found the code much easier to understand if we had used "manualAttributions" instead of "preselectedAttributions". Then it is clear what this objects are going to be in the output. I needed to read the lines after the change to finally understand that they were not some intermediate products.
@@ -318,6 +308,4 @@ function createJsonOutputFile( | |||
resourcesToAttributions: preselectedAttributionsToResources, |
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.
This sounds extremely wrong.
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.
nice catch
} | ||
} | ||
|
||
const preselectedAttributionsToResources: AttributionsToResources = {}; |
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.
See below, the name and type are wrong.
1204a54
to
69adea8
Compare
|
||
const newUUID = uuid4(); | ||
manualAttributions[newUUID] = packageInfo; | ||
preselectedAttributionIdsToExternalAttributionIds[attributionId] = |
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.
Please also rename this one.
const preselectedAttributionIdsToExternalAttributionIds: { | ||
[attributionId: string]: string; | ||
} = {}; | ||
const preselectedAttributionIds = new Set<string>(); |
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.
Please rename this one too
Improve performance of creating output file. Signed-off-by: Benedikt Richter <benedikt.richter@tngtech.com>
69adea8
to
9d198af
Compare
Summary of changes
Improve performance of creating output file.
Context and reason for change
This diff tackles the issue pointed out in #1806.
Performance of opening a file with many pre-selected attributions - tested with a production file.
Before: 124s
After 19s
How can the changes be tested
Open a file with pre-selected attributions and observe the logs for the time required to populate the output file.