Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Optimize the initialization of the OrdinalCasing::s_sCasingTable table.#46061

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
tarekgh merged 6 commits intodotnet:masterfromvargaz:casing-cctor
Dec 17, 2020

Conversation

@vargaz
Copy link
Contributor

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#43732.

The table was initialized in the .cctor and it contains a large amount of object references, so the generated cctor was1K IL. Replace with a byte table and a loop.Fixesdotnet#43732.
@ghost
Copy link

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

Issue Details

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#43732.

Author:vargaz
Assignees:-
Labels:

area-System.Globalization

Milestone:-

@vargaz
Copy link
ContributorAuthor

Updated based on comments.

@tarekgh
Copy link
Member

@vargaz could you please have some perf benchmark numbers before and after your changes? just to ensure there is no perf effect?

for(inti=0; i<s_casingTableInit.Length*8;++i)
{
// The bits are in reverse order
byteval=(byte)(s_casingTableInit[i/8]>>(7-(i%8)));
Copy link
Member

Choose a reason for hiding this comment

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

make 8 a local const?

Copy link
Member

@tarekghtarekgh left a comment

Choose a reason for hiding this comment

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

@vargaz thanks for getting this fix.

I am approving it but would be good if you can include some benchmark numbers before and after the change. I am not expecting any perf regression but I want to ensure nothing will surprise us.

@vargaz
Copy link
ContributorAuthor

Will try, just have trouble running the benchmarks.

@vargaz
Copy link
ContributorAuthor

Which public api ends up in this code path ?

@marek-safar
Copy link
Contributor

I don't think there is much to be measured, you changed a code which is called once.

@tarekghtarekgh merged commitca2aecb intodotnet:masterDec 17, 2020
@vargazvargaz deleted the casing-cctor branchDecember 17, 2020 23:44
@ghostghost locked asresolvedand limited conversation to collaboratorsJan 17, 2021
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@GrabYourPitchforksGrabYourPitchforksGrabYourPitchforks approved these changes

@safernsafernsafern left review comments

@tarekghtarekghtarekgh approved these changes

@marek-safarmarek-safarAwaiting requested review from marek-safar

+2 more reviewers

@am11am11am11 left review comments

@TornhoofTornhoofTornhoof left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Large memory dependency in OrdinalCasing::ToUpper

8 participants

@vargaz@tarekgh@marek-safar@GrabYourPitchforks@am11@Tornhoof@safern@Dotnet-GitSync-Bot
[8]ページ先頭
©2009-2025 Movatter.jp