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.

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

Large memory dependency in OrdinalCasing::ToUpper #43732

Closed
marek-safar opened this issue Oct 22, 2020 · 6 comments · Fixed by #46061
Closed

Large memory dependency in OrdinalCasing::ToUpper #43732

marek-safar opened this issue Oct 22, 2020 · 6 comments · Fixed by #46061
Assignees
Labels
area-System.Globalization enhancement Product code improvement that does NOT require public API changes/additions size-reduction Issues impacting final app size primary for size sensitive workloads tenet-performance Performance related issue
Milestone

Comments

@marek-safar
Copy link
Contributor

marek-safar commented Oct 22, 2020

ushort[]? casingTable = s_casingTable[pageNumber];
has a dependency on the large array which is unnecessary. The whole array is about 25k 2k and aggressively allocated in the static constructor but it needs to actually store only 32 bytes of data. Please replaced the multi-dimensional array with simple binary map (0 for existing null, 1 for s_noCasingPage).

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Oct 22, 2020
@ghost
Copy link

ghost commented Oct 22, 2020

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

@tarekgh tarekgh added question Answer questions and provide assistance, not an issue with source code or documentation. and removed untriaged New issue has not been triaged by the area owner labels Oct 22, 2020
@tarekgh tarekgh added this to the Future milestone Oct 22, 2020
@tarekgh
Copy link
Member

tarekgh commented Oct 22, 2020

@marek-safar

The whole array is about 25k and aggressively allocated in the static constructor

This is not true. the whole static array size is 256 * sizeof(IntPtr) which maximum static allocation is 2K. how did you come up with the 25K?

@tarekgh
Copy link
Member

tarekgh commented Oct 22, 2020

Please replaced the multi-dimensional array with simple binary map (0 for existing null, 1 for s_noCasingPage).

This is not going to work, at runtime we allocate more dynamic tables and add to this static array.

@marek-safar marek-safar changed the title Excessive memory dependency in OrdinalCasing::ToUpper Large memory dependency in OrdinalCasing::ToUpper Oct 22, 2020
@marek-safar
Copy link
Contributor Author

This is not true. the whole static array size is 256 * sizeof(IntPtr) which maximum static allocation is 2K

Sorry, that was a typo, it's 2k which is still very large.

This is not going to work, at runtime we allocate more dynamic tables and add to this static array.

The existing code only replaces the null entries. I made one suggestion and there are other solutions to remove the large initialization dependency (about 1k of IL) in the code.

@tarekgh
Copy link
Member

tarekgh commented Oct 22, 2020

I'll look at that.

@tarekgh tarekgh added enhancement Product code improvement that does NOT require public API changes/additions and removed question Answer questions and provide assistance, not an issue with source code or documentation. labels Oct 22, 2020
@jkotas jkotas added the tenet-performance Performance related issue label Oct 22, 2020
@jkotas jkotas modified the milestones: Future, 6.0.0 Oct 22, 2020
@tarekgh tarekgh added the linkable-framework Issues associated with delivering a linker friendly framework label Nov 23, 2020
@marek-safar marek-safar added size-reduction Issues impacting final app size primary for size sensitive workloads and removed linkable-framework Issues associated with delivering a linker friendly framework labels Dec 9, 2020
@vargaz vargaz self-assigned this Dec 15, 2020
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 15, 2020
vargaz added a commit to vargaz/runtime that referenced this issue Dec 15, 2020
The table was initialized in the .cctor and it contains a large amount of object references, so the generated cctor was
1K IL. Replace with a byte table and a loop.

Fixes dotnet#43732.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Globalization enhancement Product code improvement that does NOT require public API changes/additions size-reduction Issues impacting final app size primary for size sensitive workloads tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants