Skip to content

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

Merged
tarekgh merged 6 commits into
dotnet:masterfrom
vargaz:casing-cctor
Dec 17, 2020
Merged

Optimize the initialization of the OrdinalCasing::s_sCasingTable table.#46061
tarekgh merged 6 commits into
dotnet:masterfrom
vargaz:casing-cctor

Conversation

@vargaz

@vargaz vargaz commented Dec 15, 2020

Copy link
Copy Markdown
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 was
1K IL. Replace with a byte table and a loop.

Fixes dotnet#43732.
@ghost

ghost commented Dec 15, 2020

Copy link
Copy Markdown

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

vargaz commented Dec 15, 2020

Copy link
Copy Markdown
Contributor Author

Updated based on comments.

@tarekgh

tarekgh commented Dec 15, 2020

Copy link
Copy Markdown
Member

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

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

Copy link
Copy Markdown
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?

@tarekgh tarekgh left a comment

Copy link
Copy Markdown
Member

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

vargaz commented Dec 17, 2020

Copy link
Copy Markdown
Contributor Author

Will try, just have trouble running the benchmarks.

@vargaz

vargaz commented Dec 17, 2020

Copy link
Copy Markdown
Contributor Author

Which public api ends up in this code path ?

@marek-safar

Copy link
Copy Markdown
Contributor

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

@tarekgh
tarekgh merged commit ca2aecb into dotnet:master Dec 17, 2020
@vargaz
vargaz deleted the casing-cctor branch December 17, 2020 23:44
@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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Large memory dependency in OrdinalCasing::ToUpper

8 participants