Fixed unsharing of last shared row in DataGridViewRowCollection.

[kirsan31]

Proposed changes

With current implementation if currently unsharing row is the last one instance (of this shared row) in DataGridViewRowCollection we will clone it and therefore current instance will be lost. The only exception is if it is the very last row in the collection. It's more correct in this situation to always just update the index and return the current row.

Customer Impact

• In some situation old behavior can mislead ppl and theoretically lead to bugs. For example lets see at DataGridViewCell_Displayed_GetWithSharedDataGridViewWithHandle_ReturnsExpected test:

public void DataGridViewCell_Displayed_GetWithSharedDataGridViewWithHandle_ReturnsExpected(bool gridVisible, bool rowHeadersVisible, bool columnHeadersVisible, bool columnVisible)
{
    using var control = new DataGridView
    {
        Visible = gridVisible,
        RowHeadersVisible = rowHeadersVisible,
        ColumnHeadersVisible = columnHeadersVisible
    };
    using var cellTemplate = new SubDataGridViewCell();
    using var column = new DataGridViewColumn
    {
        CellTemplate = cellTemplate,
        Visible = columnVisible
    };
    control.Columns.Add(column);
    // We create default row with State = DataGridViewElementStates.Visible;
    // After that we have only one such shared row in the collection.
    // And there 2 rows in the collection overall (+ 1 new_row because of AllowUserToAddRows == true).
    control.Rows.Add();
    // Store it. Note that if we put this line after Assert.NotEqual(IntPtr.Zero, control.Handle),
    // with previews implementation (prior #6957) test will fail.
    DataGridViewRow row = control.Rows.SharedRow(0);
    // While calling control.Handle we will create a new handle.
    // During this process DataGridView call MakeFirstDisplayedCellCurrentCell().
    // Where if we have cell to display (columnVisible) we will set it as CurrentCell.
    // Which in turn will lead to unsharing of the raw in any case.
    // MakeFirstDisplayedCellCurrentCell() -> SetAndSelectCurrentCellAddress() -> SetCurrentCellAddressCore()
    //   -> OnCurrentCellChanged() -> CurrentCell.get() -> Rows[index]
    // See https://github.com/dotnet/winforms/issues/6930#issuecomment-1090213559.
    // Note that the cell is Displayed (Displayed == true) only if the owning row IS NOT shared and Displayed and owning column is Displayed.
    // So we have these options:
    // 1. columnVisible == false - our row remain shared and therefore cell will not be Displayed;
    // 2. gridVisible == false - our row and column are not Displayed, so and the cell too.
    // 3. (gridVisible && columnVisible) == true - our row became unshared and Displayed.
    //    But because this is the only one such row in the collection and Count of all rows = 2,
    //    with previous implementation (prior #6957) we will clone it and LOST.
    Assert.NotEqual(IntPtr.Zero, control.Handle);
    int invalidatedCallCount = 0;
    control.Invalidated += (sender, e) => invalidatedCallCount++;
    int styleChangedCallCount = 0;
    control.StyleChanged += (sender, e) => styleChangedCallCount++;
    int createdCallCount = 0;
    control.HandleCreated += (sender, e) => createdCallCount++;

    DataGridViewCell cell = row.Cells[0];
    // Here 3 our ways with previous implementation (prior #6957):
    // 1. All correct - we have shared and therefore not Displayed row.
    // 2. All correct - we have not Displayed row.
    // 3. control.Rows.SharedRow(0) unshared and Displayed BUT our stored row (LOST) still shared and therefore not Displayed.
    //    So test will pass of course.
    // Test must check: Assert.Equal(gridVisible && columnVisible, cell.Displayed);
    // And with new implementation it will work no mater where you put row = control.Rows.SharedRow(0);
    Assert.False(cell.Displayed);
    Assert.True(control.IsHandleCreated);
    Assert.Equal(0, invalidatedCallCount);
    Assert.Equal(0, styleChangedCallCount);
    Assert.Equal(0, createdCallCount);
}

• ~We will not lost any rows and they will not end in Freachable queue #6859~ (Not relevant after https://github.com/dotnet/winforms/pull/6978). Also we will not create a new row in this case. Performance improvement.
• With new implementation we will gain some performance penalty when unsharing rows (see comments in code).

Regression?

• No

Risk

• Low in current implementation (see comments in code).

Test methodology

Existing tests (with corrections) and manual testing.

Microsoft Reviewers: Open in CodeFlow

[RussKie]

Is this a fix to an open issue? If so - could you please link it. If not - could you please provide a sample that demonstrates the issue.

[kirsan31]

Is this a fix to an open issue? If so - could you please link it. If not - could you please provide a sample that demonstrates the issue.

I was looking for leaks while working on the https://github.com/dotnet/winforms/issues/6859 and found this problem. After fixing it I ran into the fact that two tests do not work. Actually, the fact that they worked before and indicates a problem :) I will try to update 1 post...

[kirsan31]

I've updated the 1 post with more detailed explanations.

[RussKie]

• I can put these comments to source if needed.

Thank you for the comments! They do help. And if you could add them to the code - it'd be great! I'll help with the wording once they're in.

[RussKie]

// So we have 2 ways:
    // 1. gridVisible && columnVisible == false; our row remain shared and therefore invisible.
    // 2. gridVisible && columnVisible == true; our row became unshared and therefore visible. But because this is the only one such row in the collection and Count of all rows = 2,
    // with previous implementation we will clone it and LOST.

These sounds like good candidates for individual tests. That is, add a test to verify that a row gets shared and another to verify the opposite.

// Here 2 our ways:
    // 1. All correct - we have shared and therefore invisible row.
    // 2. control.Rows.SharedRow(0) unshared and therefore visible BUT our stored row (LOST) still shared and therefore invisible :( So test will pass of course.
    // Test must check: Assert.Equal(gridVisible && columnVisible, cell.Displayed); And with new implementation it will work no mater where you put row = control.Rows.SharedRow(0);

Same here.

[kirsan31]

I will definitely look into it (and everything else), the only thing I don't know how soon, it's a complete mess with the time for now :(

[RussKie]

Totally understand. No rush, take your time. Thank you.

[KlausLoeffelmann]

@kirsan31: Is this something - from your assessment - people may have encountered before and implemented their own workaround? If that is the case, how likely is it that after this fix their workaround will no longer work? (Sorry for those basic questions - only ramping up with this).

[kirsan31]

@KlausLoeffelmann

@kirsan31: Is this something - from your assessment - people may have encountered before and implemented their own workaround? If that is the case, how likely is it that after this fix their workaround will no longer work? (Sorry for those basic questions - only ramping up with this).

The only workaround for this case that come to my mind is - refresh cached (and may be lost) shared raw with call to SharedRow(index). So after the fix everything seems to be working. But I might be missing something, of course...

[kirsan31]

Sorry for long delay. I once again studied the code, regarding the implementation through the reference counter. And I'm afraid that it will be very problematic (at least for me) and error prone :( Then I started writing a benchmark (BenchmarkDotNet) to test the performance drop when using IndexOf. But ran into a problem - can't use BenchmarkDotNet with newly build WinForms dll 🤷‍♂️ Any suggestions?

[RussKie]

I guess if @adamsitnik doesn't have an answer, the only way to run the benchmark would be with this step.

[kirsan31]

I guess if @adamsitnik doesn't have an answer, the only way to run the benchmark would be with this step.

Аnd run benchmarks separately? This will be not very accurate, but I will try...

[adamsitnik]

Any suggestions?

I currently don't have a fix for https://github.com/dotnet/BenchmarkDotNet/issues/1197#issuecomment-1173126572, but you can use the InProcessToolchain to run the benchmarks in the same process (as long as they don't have side effects like memory leaks you should be OK).

[kirsan31]

Ok, I've tested this change (thanks to @adamsitnik advice): Modified 6.0.7 branch: https://github.com/kirsan31/winforms/tree/DGVRowUnsharingBench Benchmark (need to be run from NewRel configuration): https://github.com/kirsan31/DataGridViewRowUnsharingBench Job

• Original - without modifications.
• New - testing change.

Method:

• EnumAll_1Uniq - All rows are shared with one shared row.
• EnumAll_2Uniq - First halve of rows are shared with one shared row and second halve with another shared row.
• EnumAll_HalfUniq - Every 2 rows are shared with one individual shared row.
• EnumAll_AllUniq - Every row is individual shared row. Results:

In my opinion, the drop in performance in some cases is acceptable. Also, as noted in 1 post, in some cases we get an increase in performance and a decrease in memory used.

[kirsan31]

@RussKie what about this and this your comments? I was going to implement them after a performance check...

[KlausLoeffelmann]

Does this change affect virtual mode in any way?

[kirsan31]

Does this change affect virtual mode in any way?

Yes of course, because we can call DataGridViewRow this[int index] after filling DataGridView with data in virtual mode.

[KlausLoeffelmann]

What I mean is: Have you thought of any additional cases to take into account when the DGV is virtual mode. If my question doesn't make sense, then please disregard. Just wanted to make sure we covered all important scenarios.

[kirsan31]

What I mean is: Have you thought of any additional cases to take into account when the DGV is virtual mode. If my question doesn't make sense, then please disregard. Just wanted to make sure we covered all important scenarios.

Theoretically unsharring rows through DataGridViewRow this[int index] should work the same for all modes, but I will doublecheck.

Also I almost finished with comments and new checks in sources, will update later...

[KlausLoeffelmann]

but I will doublecheck.

Thanks a lot for that!

[kirsan31]

Theoretically unsharring rows through DataGridViewRow this[int index] should work the same for all modes, but I will doublecheck.

I've looked at the code again, in my understanding, there is not (and should not be) any difference in processing of DataGridViewRow DataGridViewRowCollection[int index] for any DGV mode.

Comments and one new check added (1 post was edited too).

P.s. if we will fix #6930 (explanation here). Then these tests (and probably some others) will also need to be redo again.

[RussKie]

Totally understand. No rush, take your time. Thank you.

[kirsan31]

@RussKie I want to clarify, all the work has already been done, we are waiting for a review from @KlausLoeffelmann here...

[KlausLoeffelmann]

Just one last ask for the moment: If the rows are generated through Data Binding, will that have any breaking changes? Do you have tests for that?

[kirsan31]

@KlausLoeffelmann

Just one last ask for the moment: If the rows are generated through Data Binding, will that have any breaking changes? Do you have tests for that?

Oh, it was so long time ago... As I remember answer must be the same:

I've looked at the code again, in my understanding, there is not (and should not be) any difference in processing of DataGridViewRow DataGridViewRowCollection[int index] for any DGV mode.

But of course to be sure I have to raise (remember) my knowledge on this topic and doublecheck...

[KlausLoeffelmann]

Thanks for the quick answer! I know, it's been a while, and I was really busy with... 😉

Let's just make sure to think of everything. I really want this, but I also don't want to have any breaking changes we have not thought of!

[KlausLoeffelmann]

But of course to be sure I have to raise (remember) my knowledge on this topic and doublecheck...

Did you get the chance to check for this?

@Olina-Zhang: Do we have any DataGridView-binding-related bigger manual test scenarios we could apply for this with a set of private binaries resulting from this branch?

Also, @kirsan31, if you get the chance, could you resolve the merge-conflicts?

[kirsan31]

@KlausLoeffelmann

Did you get the chance to check for this?

Sorry not yet. And I don't know when I can, as I said, it will take extra time to get to the bottom of the problem again :(

Also, @kirsan31, if you get the chance, could you resolve the merge-conflicts?

Do you think it is advisable to do this without answering the previous question?

[KlausLoeffelmann]

Maybe for testing-now purposes, but please don't feel rushed! But no, this is not-at-all pressuring, please take your time! And as always: Really appreciate your thoroughness, here!

Will add the waiting-author-feedback label, though.

[Olina-Zhang]

@Olina-Zhang: Do we have any DataGridView-binding-related bigger manual test scenarios we could apply for this with a set of private binaries resulting from this branch?

We have some .Net framework scenarios about DataGridView-binding, and can follow them in .Net Core for testing.