perf(replay): deduplicate pending mirror removal roots - #4808
Open
marandaneto wants to merge 1 commit into
Open
Conversation
Contributor
|
Contributor
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
marandaneto
marked this pull request as ready for review
September 6, 2026 07:16
marandaneto
requested review from
TueHaulund,
arnohillen,
fasyy612 and
ksvat
and removed request for
a team
September 6, 2026 07:16
Contributor
Contributor
|
Reviews (1): Last reviewed commit: "perf(replay): deduplicate pending mirror..." | Re-trigger Greptile |
Contributor
|
Size Change: +964 B (0%) Total Size: 20.5 MB 📦 View Changed
ℹ️ View Unchanged
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Moving a large subtree repeatedly within one mutation batch queues the same root for mirror cleanup multiple times. The recorder then walks the same final subtree for every queued entry before serializing additions.
This PR is stacked on #4807. Both benchmark builds already include the previous getter-cache and serialization-options improvements. This is another partial improvement for #4217, not a complete fix.
Changes
Mirror.removeNodeFromMapand emitted removal records unchanged. No live DOM values or masking decisions are cached.Performance results
Three local runs per build and shape on an Apple M4 Pro, Chromium 136.0.7103.25, approximately 50k nodes, compression enabled and no CPU throttling. Baseline and candidate runs alternated using the same benchmark source. Profiling and diagnostic counters were disabled in timing comparisons.
The repeated-move case is five round trips plus a final move. It deliberately stresses duplicate cleanup and is not a claim about typical customer behavior. Results are descriptive samples, not significance estimates. Single-subtree removal was effectively flat. Table restoration was slightly worse in these samples, at 154 / 164 ms longest task. Startup is not optimized.
A separate 10k-node diagnostic run showed 11 cleanup roots and 110,253 recursive visits before the change, versus one root and 10,023 visits afterward. Both visited the same 10,023 distinct physical nodes. The focused regression failed before the change with 11 root visits instead of one and passes afterward.
All 12 timing comparison arms passed replay, input, privacy and drop/recovery checks. The recorder artifact increased by 13 raw bytes, with no gzip size increase in this build.
Validation and risk
881c58a8ddd502fde75670890573ae40e01a757cagainst the parent branch reported no actionable findings.The incident checker flagged serializer correctness. This does not change style serialization. Cleanup ordering, stable IDs and replay state are covered by the tests above. There are no new lazy-load contracts, persisted fields, masking policies, sampling, rotation or flush behavior. No recording-volume change is expected.
Recording remains synchronous. The repeated-move workload still blocks for roughly 470 ms at 50k nodes, so this does not bound main-thread work or resolve #4217.
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
posthog-jspatch changeset🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Implemented with Pi using repository inspection, Chromium profiling, Vitest, Playwright and isolated autoreview. The change targets demonstrated duplicate cleanup only. It does not apply that rule to mutation preprocessing or ordering, where parent relationships need separate analysis.
The code formatter hook was skipped to preserve existing rrweb formatting rather than reformat entire files. Targeted formatting/lint checks and the other commit hooks passed. No hook configuration was changed. Human review is required.