2081931 - Defer dropping the Stylo ElementData (primary and eager pseudo-element ComputedValues) of elements whose style data is cleared through Element::ClearServoData (DOM removals, but also document teardown via Document::ClearStaleServoData, NAC unbinding, shad

Status: RESOLVED DUPLICATE of bug 2052707

(Core :: CSS Parsing and Computation, defect)

Description

[perf-ai-bot]

Defer dropping the Stylo ElementData (primary and eager pseudo-element ComputedValues) of elements whose style data is cleared through Element::ClearServoData (DOM removals, but also document teardown via Document::ClearStaleServoData, NAC unbinding, shadow-root attachment, slot reassignment, input type changes and the like), moving the freeing out of the mutation into an idle runnable. Servo_Element_ClearData now detaches the element's ElementData box via the new GeckoElement::take_data (same flag and mServoData clearing as before, so HasServoData() is false immediately and the "unbound / display:none elements have no servo data" invariant holds) and pushes it onto a process-wide, main-thread-only Vec held in an AtomicRefCell static. TElement::clear_data, used by the Stylo traversal (e.g. clear_descendant_data for display:none subtrees, possibly on worker threads), is reimplemented as drop(take_data()) and still frees synchronously. After detaching, Element::ClearServoData (the only C++ caller of Servo_Element_ClearData) dispatches an idle-priority runnable (NS_DispatchToCurrentThreadQueue with a 1000 ms timeout; immediate drop if dispatch fails) that calls the new Servo_Element_DropDeferredData, unless one is already pending. A main-thread static flag tracks the pending runnable and is cleared when it runs, so the explicit flushes below do not cause redundant idle runnables and timers. The runnable drops the whole backlog at once: it does not check the idle deadline, and if no idle period comes within 1000 ms the timer runs it anyway, even on a busy main thread. The list is capped at 20000 entries; once it is full, newly detached data is dropped synchronously, as before, until the list is drained, so reaching the cap never frees the whole backlog in the middle of a mutation. The list is also flushed at the start of ServoStyleSet::ShellDetachedFromDocument (after Document::DeletePresShell's ClearStaleServoData and before MaybeGCRuleTree, so the rule tree can collect the released nodes), in ~ServoStyleSet (before the style set's rule tree is torn down, so deferred styles do not keep its nodes alive past the style set; dropping them later would still be safe through RuleNode::drop_without_free_list), and in Servo_Shutdown. Because the list is process-wide, these flushes also drop data detached from other documents. Deferral only extends the lifetime of refcounted styles; image requests are still released through the async StyleImageRequestCleanupTask in Gecko_LoadData_Drop. The freeing work is moved, not reduced: in the supplied profiles, whole-thread Servo_Element_ClearData was 207 samples before, versus 13 (Servo_Element_ClearData) + 248 (Servo_Element_DropDeferredData) after. About 59% of the deferred drop runs from the idle runnable and ~20% from PresShell::Destroy -> ShellDetachedFromDocument, none inside SP3's measured sync/async markers.

Author-reported measurement (full SP3, sequential, interleaved, 15 pairs): 10 pairs from byte-identical cp -a snapshots, 8.958 -> 9.010 runs/min (+0.58%); 5 pairs against a re-materialized baseline libxul, 8.884 -> 8.986 (+1.15%); combined 8.933 -> 9.002 (+0.77%), paired t=3.1, patched ahead in 12/15 pairs. Per-suite (snapshot arms): Svelte-Complex-DOM +2.1%, Lit-Complex-DOM +2.5%, Preact-Complex-DOM +0.7%. These numbers were measured on the originally submitted patch, before the audit changed the pending-runnable flag and the cap policy; deferral behaves the same below the cap. The layout/style mochitests the author ran (pseudo display fixup, pseudoelement state, restyle table wrapper, min-size auto, transitions) pass.

Problem

DOM removals through RestyleManager::ContentWillBeRemoved -> ClearServoDataFromSubtree -> Element::ClearServoData -> Servo_Element_ClearData synchronously destroy detached ElementData in the baseline. This releases computed-style and rule-node references and, when their reference counts permit, destroys style structs and frees allocations during the mutation. The supplied profile summary attributes 138 of 30436 in-marker main-thread samples (0.45%) to Servo_Element_ClearData and 169 (0.56%) to ClearServoDataFromSubtree, with higher shares on some Charts and NewsSite subtests. Nulling mServoData and clearing the existing flags prevents subsequent element lookups from accessing the detached ElementData. Its owned resources remain live, however, and their eventual destruction can trigger further cleanup. Deferral therefore trades immediate mutation work for retained memory and later destruction, image-request cleanup, and rule-tree reclamation.

Performance

• Benchmark: speedometer3
• Test: sp3-full
• Component: Layout
• Local Improvement: 1.05%

Validation

• Before profile
• After profile
• Try push
• Performance comparison

Comment 1

[perf-ai-bot]

Please move this bug to the correct component.

Once the Phabricator patch is ready for review, please commandeer it.

Comment 2

[perf-ai-bot]

Attachment: WIP: Bug 2081931 - Defer dropping the Stylo ElementData (primary and eager pseudo-element ComputedValues) of elements whose style data is cleared through Element::ClearServoData (DOM removals, but also document teardown via Document::ClearStaleServoData,

Defer dropping the Stylo ElementData (primary and eager pseudo-element ComputedValues) of elements whose style data is cleared through Element::ClearServoData (DOM removals, but also document teardown via Document::ClearStaleServoData, NAC unbinding, shad

Comment 3

[Daniel Holbert [:dholbert]]

If I understand the patch correctly, the tl;dr here is just deferring some cleanup work to happen in a batch at an idle time, to improve responsiveness (with graceful handling to account for edge cases like the batch getting too big or the delay getting too long).

The "performance comparison" here looks pretty good (bunch of small wins in subtests, and nearly no reported regressions), so hopefully this is worth pursuing...

Comment 4

[Denis Palmeiro [:denispal]]

A version of this patch landed in bug 2052707.