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

Vsp freeze #3564

Merged
merged 2 commits into from
Oct 7, 2020
Merged

Vsp freeze #3564

merged 2 commits into from
Oct 7, 2020

Conversation

SamBent
Copy link
Contributor

@SamBent SamBent commented Sep 24, 2020

Addresses #2490
This is a port of a servicing fix in .NET 4.7/4.8.

Issue: Scrolling a TreeView hangs.

Discussion:
The customer's data is highly non-uniform, in the sense that a given node's children govern subtrees whose sizes are quite different. This means the estimates VSP computes can differ wildly from the truth, and are subject to frequent (and sometimes violent) revision as more of the tree de-virtualizes. This is not necessarily bad, but it stresses the algorithms in ways that uniform trees don't. This stress uncovers two problems:

  1. VSP stores "effective offsets", intended to support correct layout after bottom-up size changes (e.g. a deep node gets larger because of new content), which shouldn't move the visible items. When VSP revises its estimates, it also changes its effective offsets. These are intended to stay in effect until the user scrolls, moving the visible items. But VSP used an indirect way to detect this (namely the local viewport offset). The non-uniform churn exposed a flaw - the viewport offset after a scroll can be the same as it was before (by coincidence, if estimates in nearby nodes change in just the wrong way). If so, the layout algorithm will choose the same node as before to be at the top of the viewport, and the "anchor" logic that checks whether the scroll ended up in the right place will ask for a remeasure, which merely repeats the same futile actions. Infinite loop.

  2. A violent enough change can cause the "anchor" logic to think that the anchor node's scroll offset is outside the extent of the full tree. This is because the former is computed bottom-up, and can include revised estimates that have not yet propagated to the latter (computed top-down). When this happens, the anchor logic simply accepts the current state, and the TreeView scrolls to the wrong place (sometimes by a lot - I've seen examples that were off by 1000s of nodes).

The fix for (1) is to keep a "scroll generation" counter, incremented each time the top-level scroll offset changes. Effective offsets remember the generation at which they were created, and are only applied during layout requests in the same generation.

The fix for (2) is to detect the bad situation and ask for a remeasure.

The customer's real app (but sadly not the sample repro they provided) suffers from a third problem.
3. When UseLayoutRounding=true, the anchor logic can fail because it uses unrounded size estimates of never-been-devirtualized items, inconsistent with the layout algorithm that rounds everything. As usual, this kind of failure leads to an infinite-remeasure hang, depends sensitively on the size and shape of the data and the history of scrolling and virtualization, and seems to be more likely in large non-uniform datasets.

The fix for (3) is to apply layout rounding to the estimated sizes.

@SamBent SamBent requested a review from a team as a code owner September 24, 2020 00:18
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Sep 24, 2020
#region Data

VirtualizationCacheLength _cacheLength;
VirtualizationCacheLengthUnit _cacheLengthUnit;
Rect _viewport;
long _scrollGeneration;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL

If you do 1000 scroll operations per second, that will happen in about 280 million years.

Maybe we can use ulong 😂

@SamBent SamBent changed the base branch from release/5.0-rc2 to release/5.0 October 7, 2020 00:29
@SamBent SamBent merged commit ec587c0 into dotnet:release/5.0 Oct 7, 2020
@SamBent SamBent deleted the VSPFreeze branch October 7, 2020 18:24
dotnet-maestro bot added a commit that referenced this pull request Oct 16, 2020
[master] Update dependencies from dotnet/winforms
- Coherency Updates:
  - Microsoft.Win32.Registry: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.CodeDom: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Configuration.ConfigurationManager: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Diagnostics.EventLog: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.DirectoryServices: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Drawing.Common: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Reflection.MetadataLoadContext: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Security.AccessControl: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Security.Cryptography.Xml: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Security.Permissions: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Security.Principal.Windows: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Windows.Extensions: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - Microsoft.NETCore.Platforms: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.IO.Packaging: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - Microsoft.NETCore.ILDAsm: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - Microsoft.NETCore.ILAsm: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - System.Resources.Extensions: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - Microsoft.NETCore.App.Internal: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - Microsoft.NETCore.App.Ref: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)
  - Microsoft.NETCore.App.Runtime.win-x64: from 6.0.0-alpha.1.20428.2 to 6.0.0-alpha.1.20515.8 (parent: Microsoft.Private.Winforms)

 - .NET Core WPF Build error on custom BaseIntermediateOutputPath #1718 (#3120)

* Use more robust relative path calculation in markup code generation

 - Do not reflow lines after measure

 - disconnect HostVisual on its own thread

 - Fix three scrolling hangs

 - remove useless Assert

 - handle re-entrant request to close ToolTip

 - Move StackTrace to error branch

 - FixedPage SOM bugs

 - Don't add null item peers

 - Use correct lock object for predefined packages

 - repair bad copy/paste

 - fix build break

 - remove AppContext switch

 - DataGrid.Copy: fail silently if clipboard is locked

 - update_intellisense_artifacts

 - Remove or replace Policheck violations in code comments (#3606)

 - Merge pull request #3542 from dotnet/rc2-askmode-BaseIntermediateOutputPath

Ask-Mode: [release/5.0-rc2] Custom intermediate output paths shouldn't break markup compilation

 - Merge pull request #3564 from SamBent/VSPFreeze

Vsp freeze

 - Merge pull request #3565 from SamBent/AutomationPeerNull

Avoid null item AutomationPeers

 - Merge pull request #3566 from SamBent/TextReflow

Avoid reflow of shaped text

 - Merge pull request #3567 from SamBent/HostVisualThreading

HostVisual threading

 - Merge pull request #3568 from SamBent/PopupReentrancy

Reentrancy when closing ToolTip

 - Merge pull request #3570 from SamBent/ClearTypeAntiAliasing

ClearType anti-aliasing

 - Merge pull request #3574 from SamBent/NoisyStackTrace

Move StackTrace to error branch

 - Merge pull request #3575 from SamBent/FixedPageOverlap

FixedPage SOM bugs

 - Merge pull request #3576 from SamBent/LockedClipboard

DataGrid.Copy: fail silently if clipboard is locked

 - Merge pull request #3577 from SamBent/PredefinedPackageSynch

App resource threading issue

 - Revert "Merge pull request #3542 from dotnet/rc2-askmode-BaseIntermediateOutputPath"

This reverts commit 3d4e91c, reversing
changes made to ab840c4.

 - Merge branch 'release/5.0' of https://github.com/dotnet/wpf into release/5.0

 - update intellisense version

 - missed a version

 - 1228498 [ wpf ][ PoliCheck ] - Defect : Term "nuked"

 - Merge pull request #3637 from dotnet/revertPath

[release/5.0] Revert path change to markup compiler

 - Merge pull request #3642 from dotnet/policheck2

1228498 [ wpf ][ PoliCheck ] - Defect : Term "nuked"

 - Update arcade

 - Merge remote-tracking branch 'upstream/release/5.0' into darc-master-f9327a78-0358-4132-8625-923a71a6b121

 - Update wpf-int dependencies

 - Replace null comparisons on non-nullable types that now cause compilation errors, due to the .NET SDK update.

 - PR feedback

 - PR feedback

 - Merge pull request #3640 from AdamYoblick/release/5.0

Update intellisense version

 - Merge remote-tracking branch 'upstream/release/5.0' into darc-master-f9327a78-0358-4132-8625-923a71a6b121

 - Merge branch 'darc-master-f9327a78-0358-4132-8625-923a71a6b121' of https://github.com/dotnet/wpf into darc-master-f9327a78-0358-4132-8625-923a71a6b121

 - Suppress obsolete API warnings. These must be fixed for 6.0.0.

 - Merge pull request #3659 from dotnet/fixdarcupdate

Temporarily suppress obsolete API errors to get WPF master building

 - Merge branch 'master' into darc-master-f9327a78-0358-4132-8625-923a71a6b121
@ghost ghost locked as resolved and limited conversation to collaborators Apr 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants