Skip to content

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
@ghost ghost 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
@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