-
Notifications
You must be signed in to change notification settings - Fork 56
fix: add caching + reduce redundant array creation in getHierarchy #1194
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
base: main
Are you sure you want to change the base?
Conversation
|
✅ No documentation updates required. |
…mplitude-TypeScript into dpgraham/optimize-get-hierarchy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes the performance of the getHierarchy and getNearestLabel functions by implementing caching mechanisms and replacing inefficient array operations with direct iteration. The changes are motivated by performance tests showing that these functions are called multiple times on the same element within the same event loop.
- Adds caching for
getHierarchyandgetNearestLabelresults that invalidates after the current event loop - Replaces
Array.fromandArray.prototype.filteroperations with direct for-loop iterations to reduce O(n) array creation overhead - Includes comprehensive performance test pages to validate the optimizations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test-server/autocapture/performance-test.html | Performance test page with deeply nested DOM structure (15 levels, 12 siblings per level) |
| test-server/autocapture/performance-test-extreme.html | Extreme performance test page with 100th descendant target and 100 siblings per row |
| packages/plugin-autocapture-browser/src/hierarchy.ts | Adds caching mechanism and optimizes sibling iteration in getElementProperties and getHierarchy |
| packages/plugin-autocapture-browser/src/helpers.ts | Adds caching mechanism for getNearestLabel function |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
✅ No documentation updates required. |
|
bugbot run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Bugbot reviewed your changes and found no bugs!
Comment @cursor review or bugbot run to trigger another review on this PR
Mercy811
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Dan for fixing it! LGTM!
| // beforeAll(() => { | ||
| // // mock getGlobalScope | ||
| // jest.spyOn(AnalyticsCore, 'getGlobalScope').mockImplementation(() => { | ||
| // const globalThis = global as any; | ||
| // console.log("RETURNING GLOBAL SCOPE"); | ||
| // return { | ||
| // MessageChannel: function () { | ||
| // return { | ||
| // port1: { | ||
| // onmessage: () => {}, | ||
| // }, | ||
| // port2: { | ||
| // postMessage: () => {}, | ||
| // }, | ||
| // }; | ||
| // }, | ||
| // ...globalThis, | ||
| // }; | ||
| // }); | ||
| // // (global as any).MessageChannel = function () { | ||
| // // return { | ||
| // // port1: { | ||
| // // onmessage: () => {}, | ||
| // // }, | ||
| // // port2: { | ||
| // // postMessage: () => {}, | ||
| // // }, | ||
| // // }; | ||
| // // }; | ||
| // }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just delete it?
| // beforeAll(() => { | |
| // // mock getGlobalScope | |
| // jest.spyOn(AnalyticsCore, 'getGlobalScope').mockImplementation(() => { | |
| // const globalThis = global as any; | |
| // console.log("RETURNING GLOBAL SCOPE"); | |
| // return { | |
| // MessageChannel: function () { | |
| // return { | |
| // port1: { | |
| // onmessage: () => {}, | |
| // }, | |
| // port2: { | |
| // postMessage: () => {}, | |
| // }, | |
| // }; | |
| // }, | |
| // ...globalThis, | |
| // }; | |
| // }); | |
| // // (global as any).MessageChannel = function () { | |
| // // return { | |
| // // port1: { | |
| // // onmessage: () => {}, | |
| // // }, | |
| // // port2: { | |
| // // postMessage: () => {}, | |
| // // }, | |
| // // }; | |
| // // }; | |
| // }); |
Summary
getHierarchyandgetNearestLabelget executed multiple times (click handler, change handler) on the same element, within the same event loop, returning the same resultArray.fromandArray.prototype.filteron both thechildrenandattributesproperties of Element. These methods create new arrays, where the creation of the arrays is O(n) both size and spacegetHierarchyandgetNearestLabelinto one function so that we traverse an element's ancestry once instead of twiceChecklist
Note
Memoizes
getHierarchyand nearest-label lookups per event loop, streamlines hierarchy property computation to avoid array creations, and adds deep DOM performance test pages.getHierarchy(element)results within the same event loop usingMessageChannel-backed cache.getNearestLabel(element)used ingetEventProperties.Array.from/filterusage with indexed iteration for siblings and attributes to avoid extra allocations.index,indexOfType) and previous-sibling tag retrieval.getGlobalScopeand add test ensuring cached hierarchy reuse.test-server/autocapture/performance-test.htmlandperformance-test-extreme.htmlto benchmark deep DOM hierarchies with metrics UI.Written by Cursor Bugbot for commit 7b21b28. Configure here.