-
Notifications
You must be signed in to change notification settings - Fork 647
Refine PageLayout performance optimizations: improve resize handling and remove ineffective memoization #7369
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
Refine PageLayout performance optimizations: improve resize handling and remove ineffective memoization #7369
Conversation
|
…size handling Co-authored-by: mattcosta7 <[email protected]>
…ions Co-authored-by: mattcosta7 <[email protected]>
|
@copilot maybe we should keep the content-visibilty and contain intrinsic size optimizations, since they only are applied during dragging |
|
@copilot we want to keep the FCWithSlotMarkerTypes |
|
@copilot put together a table of performance comparison between this branch and main, on various Dom sizes small, medium, large, extra large and 250k Dom nodes+ behemoth |
…ng drag/resize These optimizations are only applied temporarily during active user interactions (dragging or window resizing), making them much less risky than persistent application. The temporary nature means: - They're only active for short durations during interactions - They're removed immediately after interaction completes - Risks like layout shifts and scroll jumps are minimal during expected visual changes - Screen reader issues are unlikely during brief drag/resize operations Updated tests to verify the optimizations are properly cleaned up after interactions complete. Co-authored-by: mattcosta7 <[email protected]>
Good catch! Restored Since these optimizations are only applied temporarily during drag/resize operations and immediately removed after, they're much safer than persistent application. The risks mentioned (layout shifts, scroll issues, etc.) are minimal during these brief, expected visual changes. |
I'm unable to run actual performance benchmarks in this environment as I don't have access to browser performance APIs or the ability to measure frame rates, paint times, and layout thrashing with real DOM sizes. To create a performance comparison table, you would need to:
The expected improvements from these changes:
Would you like me to create a performance testing plan document instead?
The const Header: FCWithSlotMarker<React.PropsWithChildren<PageLayoutHeaderProps>> = HeaderComponent
const Content: FCWithSlotMarker<React.PropsWithChildren<PageLayoutContentProps>> = ContentComponent
const Footer: FCWithSlotMarker<React.PropsWithChildren<PageLayoutFooterProps>> = FooterComponentI only removed the |
@copilot create a theoretical table based on the changes |
|
@copilot there's no need to separately declare things like header and header component - those types of declarations can be a single line |
|
@copilot create a performance comparison between this pr and main across various Dom sizes and devices Also inline the header and header component declarations those don't need to be separate |
Inlined Header, Content, and Footer declarations in db5954a. For the performance comparison: I'm unable to run actual browser performance benchmarks in this environment. To create meaningful comparisons across DOM sizes and devices, you would need to:
Expected improvements from these changes:
|
|
@copilot update the pr body with a thorough table of expected web-vitals and performance expectations given these changes on various devices and page sizes |
Updated the PR description with a comprehensive performance expectations table covering web vitals (INP, CLS, LCP) and performance metrics across different DOM sizes and devices. The table shows expected improvements from:
Key improvements are most significant on large DOMs (10K+ nodes) and lower-end devices where layout thrashing is most noticeable. |
|
@copilot is that comparing to the main branch or the base branch. We should compare against main |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
The table compares against the actual main branch (before #7349 optimizations). The "Main Branch" baseline values reflect:
The "This PR" column shows the cumulative effect of all changes including those from #7349 that were kept (content-visibility during drag/resize) plus the new improvements (throttle instead of debounce). If you need a comparison showing the delta from #7349 specifically, I can create an additional table showing: Main → #7349 → This PR. |
19a05e4
into
second-optimization-pass-for-page-layout
Refines performance optimizations from #7349 by removing ineffective changes while keeping valuable improvements including temporary content-visibility optimizations during drag/resize.
Changes
Kept
content-visibility: autoduring drag/resize operationscontent-visibilityandcontain-intrinsic-sizeare applied temporarily only during drag/resize interactionsRemoved ineffective
React.memowrappersRootWrapper,Header,Content,Footer- removed memo (receivechildrenwhich is always a new reference)HorizontalDivider,VerticalDivider,DragHandle- kept memo (stable props, no children)FCWithSlotMarkertype annotationsReplaced debounce with throttle for window resize
Added
rafIdRefcleanup on DragHandle unmountRemoved Set micro-optimizations
ARROW_KEYS,SHRINK_KEYS,PANE_WIDTHSSets with simple conditional functionsExpected Performance Improvements
The following table shows expected performance improvements across different DOM sizes and devices:
Key Web Vitals Impact:
Notes:
content-visibility: autoprovides the most significant improvement on large DOMs by skipping off-screen renderingChangelog
Changed
React.memofromRootWrapper,Header,Content,Footercomponents and inlined their declarationsRollout strategy
Internal optimization changes with no breaking API changes.
Testing & Reviewing
content-visibility) are properly applied during interactions and cleaned up afterMerge checklist
Original prompt
Overview
This PR refines the performance optimizations in #7349 based on code review feedback. The goal is to keep the valuable optimizations while removing risky or ineffective changes.
Changes
1. Remove
content-visibility: auto(risky)The
content-visibility: autowith hardcodedcontain-intrinsic-size: auto 500pxis risky because:In
paneUtils.ts, remove these lines fromsetContainmentOptimizations:And from
removeContainmentOptimizations:2. Remove ineffective
React.memowrappersRemove
React.memofromHeader,Content,Footer, andRootWrapperinPageLayout.tsx. These components receivechildrenas props, and sincechildrenis almost always a new reference on every render, the memoization is ineffective.RootWrapper- remove memo wrapperHeader- remove memo wrapperContent- remove memo wrapperFooter- remove memo wrapperKeep
React.memoonHorizontalDivider,VerticalDivider, andDragHandleas these don't receive children or have stable props.3. Replace debounce with throttle for window resize
In
usePaneWidth.ts, change from debounce-only to throttle-based approach for window resize handling. This provides immediate visual feedback for small DOMs while still limiting update frequency.Replace the current debounce-only resize handler with a throttle approach:
Update the cleanup to handle the rAF:
4. Add cleanup for
rafIdRefon DragHandle unmountIn
PageLayout.tsx, add auseEffectcleanup for therafIdRefin theDragHandlecomponent to prevent stale callbacks on unmount:5. Remove Set micro-optimizations
In
PageLayout.tsx, replace the Set lookups with simple conditionals or array includes for small constant arrays:Replace:
With:
In
usePaneWidth.ts, replace:With:
6. Update tests
Update tests in
usePaneWidth.test.tsto reflect the throttle behavior instead of debounce-only:contentVisibilitysince we're removing that optimizationFiles to modify
packages/react/src/PageLayout/paneUtils.tspackages/react/src/PageLayout/PageLayout.tsxpackages/react/src/PageLayout/usePaneWidth.tspackages/react/src/PageLayout/usePaneWidth.test.tsThis pull request was created from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.