Skip to content
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.

Sign up for GitHub

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

Jump to bottom

perf: Load SAPUI5 types only when needed #46

Merged
merged 7 commits into from
Mar 26, 2024
Merged

perf: Load SAPUI5 types only when needed #46

merged 7 commits into from
Mar 26, 2024

Conversation

matz3
Copy link
Member

@matz3 matz3 commented Mar 22, 2024

No description provided.

@matz3 matz3 mentioned this pull request Mar 25, 2024
Closed
27 tasks
@matz3 matz3 force-pushed the lazy-load-sapui5-dts branch from 052480c to 2432db2 Compare March 25, 2024 16:34
@matz3 matz3 marked this pull request as ready for review March 25, 2024 16:35
@matz3 matz3 requested a review from a team March 25, 2024 16:36
@matz3 matz3 force-pushed the lazy-load-sapui5-dts branch 2 times, most recently from 052480c to 878c589 Compare March 25, 2024 17:01
@d3xter666
Copy link
Contributor

LGTM

I've done a perf measurement with a lint against openui5/sap.m. Here are the results

Command Mean [s] Min [s] Max [s] Relative
ui5lint (lazy-load-sapui5-dts) 33.412 ± 0.392 33.118 34.425 1.00
ui5lint (main) 33.752 s ± 0.466 32.983 34.645 1.00

@matz3
Copy link
Member Author

matz3 commented Mar 26, 2024

Command Mean [s] Min [s] Max [s] Relative
ui5lint (lazy-load-sapui5-dts) 33.412 ± 0.392 33.118 34.425 1.00
ui5lint (main) 33.752 s ± 0.466 32.983 34.645 1.00

Thanks for doing a measurement. I just did some manual tests and for me the benefit was about 5s for sap.m.
I wonder why you didn't get such a difference.

RandomByte
RandomByte reviewed Mar 26, 2024
View reviewed changes
src/detectors/typeChecker/host.ts Outdated
Comment on lines 38 to 39
.filter((entry) => entry.isFile() && entry.name.endsWith(".d.ts") && entry.name !== "index.d.ts")
.map((entry) => entry.name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick (decide for yourself): Replace with a for-loop. For reference, see also https://romgrk.com/posts/optimizing-javascript#3-avoid-arrayobject-methods

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably never have more than 100 entries here and the function is only called once per linting, but I guess it makes sense to be consistent and prevent multiple looping over arrays in general.

@matz3 matz3 force-pushed the lazy-load-sapui5-dts branch from 9048ebd to fa4a111 Compare March 26, 2024 10:09
@matz3 matz3 force-pushed the lazy-load-sapui5-dts branch from fa4a111 to b3d57cc Compare March 26, 2024 10:13
@matz3 matz3 requested a review from RandomByte March 26, 2024 10:22
RandomByte
RandomByte approved these changes Mar 26, 2024
View reviewed changes
Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@matz3 matz3 merged commit b7e9a2b into main Mar 26, 2024
16 checks passed
@matz3 matz3 deleted the lazy-load-sapui5-dts branch March 26, 2024 13:37
@openui5bot openui5bot mentioned this pull request Mar 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RandomByte RandomByte RandomByte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matz3 @d3xter666 @RandomByte