Skip to content

Replace use of ArrayList with List<T> in BamlMapTable for performance #9967

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

Merged
merged 8 commits into from
Mar 24, 2025

Conversation

h3xds1nz
Copy link
Member

@h3xds1nz h3xds1nz commented Oct 16, 2024

Description

Replaces 4x ArrayList collections with List<T> for improved performance and increased type safety.

I've also introduced init setters for the respective properties so they can be used as part of the Clone operation and the underlying fields can be converted to readonly. Rest are just NULL check removals coming from the as casting pattern or simply redundant as the values in the lists are always from freshly initialized types, no NULLs are inserted.

Sample benchmark showing difference between ArrayList and List<T> with reference type can be found f.e. in #9432.

Customer Impact

Improved performance, cleaner codebase for developers.

Regression

No.

Testing

Local build.

Risk

Low, just type swaps.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners October 16, 2024 13:40
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Oct 16, 2024
@h3xds1nz h3xds1nz changed the title Replace use of ArrayList with List<T> in BamlMapTable Replace use of ArrayList with List<T> in BamlMapTable for performance Oct 16, 2024
himgoyalmicro
himgoyalmicro previously approved these changes Mar 24, 2025
@himgoyalmicro
Copy link
Contributor

@h3xds1nz this PR is good to be merged. Can you please resolve the merge conflicts?

@h3xds1nz
Copy link
Member Author

@himgoyalmicro Yeyyyy :) Done

Copy link

codecov bot commented Mar 24, 2025

Codecov Report

Attention: Patch coverage is 0% with 67 lines in your changes missing coverage. Please review.

Project coverage is 11.07036%. Comparing base (b6914d7) to head (13de797).
Report is 5 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main       #9967         +/-   ##
===================================================
- Coverage   11.45872%   11.07036%   -0.38836%     
===================================================
  Files           3214        3352        +138     
  Lines         648458      668000      +19542     
  Branches       71511       74980       +3469     
===================================================
- Hits           74305       73950        -355     
- Misses        572989      592890      +19901     
+ Partials        1164        1160          -4     
Flag Coverage Δ
Debug 11.07036% <0.00000%> (-0.38836%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@himgoyalmicro himgoyalmicro merged commit 6514196 into dotnet:main Mar 24, 2025
8 checks passed
@himgoyalmicro
Copy link
Contributor

Thank you, @h3xds1nz, for your consistent and valuable contributions. 😄

@h3xds1nz
Copy link
Member Author

@himgoyalmicro Thank you for reviewing :)

@github-actions github-actions bot locked and limited conversation to collaborators Apr 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants