- Notifications
You must be signed in to change notification settings - Fork5.2k
Optimize asm size for the biggest corelib method - WebUtility..cctor#48906
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedMar 1, 2021
Tagging subscribers to this area: @dotnet/ncl Issue DetailsAccording to the jit-diff tool This PR shrinks I could use source gen here since we already use them for eventpipe stuff but I don't think it worth the complexity.
|
MichalStrehovsky commentedMar 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Splitting the data into two static ReadOnlySpans (one for keys, one for values) would probably be the most efficient, space-wise, and codegen-wise. Then just do a for loop, indexing into both spans and adding into dictionary. |
EgorBo commentedMar 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Nice idea, wonder if it's possible to hide those two spans under the hood of the source gen then and let the user to specify any type of such immutable dictionaries |
MichalStrehovsky commentedMar 1, 2021
Ah, I just realized that the static ReadOnlySpan trick won't work because these are |
stephentoub commentedMar 1, 2021
Yeah, we need#24961, if anyone wants to tackle that in the runtime and compiler. :-) |
jkotas commentedMar 1, 2021
You can have just one Span of bytes with the data and do the endianness conversion yourself. |
jkotas commentedMar 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Or if you wanted to optimize the heck out of this, you can create a perfect or nearly perfect hash function for this set and use the |
Uh oh!
There was an error while loading.Please reload this page.
| 0x73,0x6D,0x61,0x69,0x64,0x00,0x00,0x00/*ToUInt64Key("diams")*/,0x66,0x26,/*'\x2666'*/ | ||
| }; | ||
| vardictionary=newDictionary<ulong,char>(tableData.Length/(sizeof(ulong)+sizeof(char))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think Jan was suggesting not using Dictionary at all, and instead a perfect hash over a buffer.
If you use what you're doing here, looks like you can remove the byte in the 8th column as it's always zero.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Well, this code is only executed once, my goal was to decrease the native size (from17605 bytes to only277 bytes!) using the first Jan's suggestion. If you insist on the second more performant one I can change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No I think this is fine, I was secretly hoping you would do it only because it would be interesting 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
And kudos for the huge improvemnet. Are there any more of these big static dictionaries I wonder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Found another, but likely not on a hot path:
Line 274 in0e8efce
| varresult=newDictionary<string,string>(Count) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will take a look, thanks!
Uh oh!
There was an error while loading.Please reload this page.
According to the jit-diff tool
WebUtility..cctoris the largest method (with a noticeable delay to compile it) - it's triggered whenWebUtility.HtmlDecodepublic API is called.This PR shrinks
s_lookupTablecodegen from 17605 bytes to10083277 bytes.I could use source gen here since we already use them for eventpipe stuff but I don't think it worth the complexity.