Repository navigation
Remember inherited lookups on the way up - #263
Conversation
A computed style looks inherited declarations up through the snapshots of its parents. A property that no ancestor declares, e.g. visibility, was looked up through every ancestor for every element, so GetInnerText and IsVisible on the render tree grew quadratically with the depth (AngleSharp#262). - Keep the nearest declaration found for a name, or that there is none, on the snapshots passed; they never change, so each one is walked once per name. The entries are copied on write and published atomically - Leave the lookup of explicitly inherited declarations as it was - Add tests for deep chains, read deepest first and twice - Add DeepTreeBenchmarks for GetInnerText and IsVisible at 100 and 500 levels
FlorianRappl
left a comment
FlorianRappl
left a comment
There was a problem hiding this comment.
Good, but not good enough as the two most important pieces did not really move:
| CssCascadeBenchmarks.RenderTree | 5,241 μs | 5,105 μs | −2.6% | same |
| CssCascadeBenchmarks.ComputedStyle | 3,954 μs | 3,983 μs | +0.7% | same |
Can we use the PR to improve those, too?
The cascade allocated a new list for the matching rules of every element and grew the explicit style one declaration at a time; on a real document this growth and the garbage it leaves dominate the render tree. - Reuse the list of matching rules from element to element; it is taken and given back atomically, so a concurrent cascade gets its own - Size the explicit style for the declarations of the matching rules
|
Profiling the two benchmarks on this branch (sampled stacks, net10.0):
Reusing the collection across calls would remove most of that, but it would have to be dropped when a sheet is added, removed or disabled, a rule changes, or a media condition stops matching (e.g. the device is resized). I see two ways:
Would you like one of these, here or in a separate PR? Or do you have an existing mechanism in mind for noticing style changes? |
|
With the reduced GC I think this makes sense for this round. Let's keep it like that! |
Types of Changes
Prerequisites
Please make sure you can check the following two boxes:
Contribution Type
What types of changes does your code introduce? Put an
xin all the boxes that apply:Description
The cache part of #262; rebuilding
GetInnerTexton the render tree follows as a separate PR.A computed style looks inherited declarations up through the snapshots of its parents. A property that no ancestor declares, e.g.
visibility, was looked up through every ancestor for every element, soIsVisibleon the render tree andGetInnerTextgrew quadratically with the depth.The change
Snapshotremembers lookups: the nearest declaration found for a name (or that there is none) is kept on the snapshots passed on the way up. Snapshots never change, so each one is walked once per name. The entries are a small array per snapshot, copied on write and published withInterlocked.CompareExchange, so concurrent readers of the same computed style stay safe without a lock.prop: inheriton every level) keep the existing walk; it is rare and needs every level.Snapshotclass inCssStyleDeclaration.cs.InheritanceChainTests: 300-level chains built with the DOM API, read deepest first and then again, forcolorinherited from two different ancestors and forvisibility: hiddenfrom one. A cache that leaks an answer past the declaring ancestor fails both.DeepTreeBenchmarks:GetInnerText, andIsVisibleover a render tree built once, at 100 and 500 levels (within what the recursive walks survive on a 1 MB stack).Benchmarks
Paired builds of the same benchmark code against
devel(d0c4329), alternating which goes first each round. BenchmarkDotNet 0.15.8, net10.0, AngleSharp 1.9.0 for both, pinned to the performance cores of an Intel Core Ultra 7 165H. Median of 6 rounds:DeepTreeBenchmarks.IsVisible, 100 levelsDeepTreeBenchmarks.IsVisible, 500 levelsDeepTreeBenchmarks.InnerText, 100 levelsDeepTreeBenchmarks.InnerText, 500 levelsInnerTextBenchmarks.GetInnerText_RealWorld_LengthInnerTextBenchmarks.GetInnerText_SelectorHeavy_LengthCssCascadeBenchmarks.RenderTreeCssCascadeBenchmarks.ComputedStyleThe deep-tree gains hold in every round (e.g. −90.9% to −91.5% for
IsVisibleat 500). The real documents stay within the noise: their rounds change sign, and one round was +15% for bothGetInnerTextcases at once, a disturbance of that round.GetInnerTextcomputes the styles again on every call, so the remembered lookups cost about 4% more allocation on the deep trees and under 1% on the real ones.🤖 Generated with Claude Code