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

Fix datagrid memory leak - selection and clear #9924

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

singhashish-wpf
Copy link
Member

@singhashish-wpf singhashish-wpf commented Oct 10, 2024

Fixes #6983 and partially fixes 6087

Description

Clear properties which were holding references related to selection and focused info, once the datagrid is cleared.

Customer Impact

Huge memory consumption.

Regression

No

Testing

Local testing with sample app for memory leak fix.
WPF CTP testing.

Risk

Low

Microsoft Reviewers: Open in CodeFlow

@singhashish-wpf singhashish-wpf requested review from a team as code owners October 10, 2024 04:58
@dotnet-policy-service dotnet-policy-service bot added the PR metadata: Label to tag PRs, to facilitate with triage label Oct 10, 2024
@singhashish-wpf
Copy link
Member Author

cc: @KirillOsenkov

KirillOsenkov
KirillOsenkov previously approved these changes Oct 10, 2024
@Symbai
Copy link
Contributor

Symbai commented Oct 10, 2024

In the original issue people reported that it also happens on ItemsControl and ListView etc. I only see a fix for Datagrid here. So its just a partial fix? If so a merge should not close the original issue.

@KirillOsenkov
Copy link
Member

It's a good idea to fix this for other controls as well, but I think it's better to do it in separate PRs, one per control. This way if we break anything we can roll back just that one PR.

@Symbai
Copy link
Contributor

Symbai commented Oct 11, 2024

It's a good idea to fix this for other controls as well, but I think it's better to do it in separate PRs, one per control. This way if we break anything we can roll back just that one PR.

I don't disagree but to my knowledge if the first post contains Fixes #number then the issue gets automatically closed once the PR has been merged. And since the original issue contains useful information including repros I think it should be left open.

@h3xds1nz
Copy link
Contributor

I don't disagree but to my knowledge if the first post contains Fixes #number then the issue gets automatically closed once the PR has been merged. And since the original issue contains useful information including repros I think it should be left open.

You know how good it feels to close an issue? Eitherway there's also #6983, so that one could be considered for DataGrid and the currently linked one renamed appropriately if the issue is in those controls.

@Symbai
Copy link
Contributor

Symbai commented Oct 11, 2024

You know how good it feels to close an issue?

Yes. But I also know how insanely frustrating it feels for users if repo owners are closing an issue which isn't (fully) fixed. You have to create a new one, posting all the information again. Repo owners asking for a repro again, etc.

Eitherway there's also #6983, so that one could be considered for DataGrid and the currently linked one renamed appropriately if the issue is in those controls.

Sounds good 👍

@h3xds1nz
Copy link
Contributor

Yes. But I also know how insanely frustrating it feels for users if repo owners are closing an issue which isn't (fully) fixed. You have to create a new one, posting all the information again. Repo owners asking for a repro again, etc.

Of course, I was joking, was most likely an oversight.

@singhashish-wpf
Copy link
Member Author

Thanks @KirillOsenkov for the suggestion and the review.
Thanks @Symbai and @h3xds1nz
Updated the description to remove the link to the other issue which contains lot more information, such that it doesn't get closed automatically with this PR.

@h3xds1nz
Copy link
Contributor

@singhashish-wpf No worries.

By the way, a very tiny nit: Most of the codebase has space between // and the actual comment text, I believe we should follow it when we can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Included in test pass PR metadata: Label to tag PRs, to facilitate with triage Status:InProgress Status:Proposed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataGrid: memory leaks after items cleared
5 participants