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

Use a frozendictionary on net-core and a readonly one on netfx #72995

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Apr 12, 2024

Running a test insertion to see if this removes the recent regressions caused by: #72965

PR1: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=9406987&view=results
PR2: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=9407001&view=results

FrozenDicts turn out to be quite expensive on NetFx if you're using StringComparer.CaseInsensitive. It causes thsi sort of blowup as the dict analyzes keys:

image

This issue is not present on netcore, where FrozenDict can do all of this work in a non-allocating fashion.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 12, 2024 17:41
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 12, 2024
@CyrusNajmabadi CyrusNajmabadi merged commit 6337bb3 into dotnet:main Apr 12, 2024
25 of 27 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the frozenDict2 branch April 12, 2024 19:21
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 12, 2024
@ToddGrun
Copy link
Contributor

Post here when you get results from the test insertions, as I'm curious whether this improved things

@CyrusNajmabadi
Copy link
Member Author

Will do.

@ToddGrun
Copy link
Contributor

Given stephen's change, this feels slightly less appealing even if the speedometer test results come back with good results

@CyrusNajmabadi
Copy link
Member Author

Note: we'd have to wait for stephen's change to make it all the way out into a release that we can pull into VS. When that happens, i'll undo this an dmove to FrozenDict uniformly.

@CyrusNajmabadi
Copy link
Member Author

Perf tests are here https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/543655

They show that there's no regression at all now that we use a RODict in netfx and a FrozenDict in core:

image

@ToddGrun
Copy link
Contributor

When that happens, i'll undo this an dmove to FrozenDict uniformly.

Might be nice to slip in a TODO comment in one of your upcoming PRs that this is temporary and a link to Stephen's PR

@CyrusNajmabadi
Copy link
Member Author

I simply will not forget. With my memory being perfect, there's no chance of anything going wrong.

@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski For review when you get back.

@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants