feat: recorder performance improvements #313
Conversation
|
Warning Rate limit exceeded@amhsirak has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request introduces changes focused on performance monitoring and optimization across various components. Modifications include updates to the Changes
Sequence DiagramsequenceDiagram
participant Frontend as Frontend Performance Monitor
participant Backend as Backend Performance Monitor
participant Browser as Remote Browser
participant Canvas as Canvas Component
Frontend->>Frontend: Track FPS, Memory
Backend->>Backend: Monitor Screenshot Times
Browser->>Browser: Optimize Screenshots
Canvas->>Canvas: Manage Animation Frames
Canvas->>Canvas: Debounce Events
Possibly related PRs
Suggested Labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
amhsirak
added
the
Status: Work In Progess
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
perf/performance.ts (2)
1-24: Consider bounding the data arrays to prevent unbounded growth.
TheFrontendPerformanceMonitorconstructor starts collecting FPS, memory usage, render times, and event latencies without a retention limit. Over long sessions, these arrays could grow large and consume excessive memory.constructor() { this.metrics = { - fps: [], - memoryUsage: [], - renderTime: [], - eventLatency: [], + fps: [], // Possibly store up to N entries + memoryUsage: [], // Possibly store up to N entries + renderTime: [], // Possibly store up to N entries + eventLatency: [], // Possibly store up to N entries }; ... }
96-118: Limit memory usage array in theBackendPerformanceMonitorfor longevity.
Like the frontend monitor, the backend monitor stores memory usage in an unbounded array. Over extended operation periods, memory usage for storing data points could become problematic.src/components/atoms/canvas.tsx (1)
236-236: Wrap variable declarations in their own block within the switch-case.
The static analysis tool flags declaration within thecase 'wheel':block. Wrapping the declaration in{ ... }ensures proper scoping and prevents potential collisions with other cases.case 'wheel': + { const wheelEvent = event as WheelEvent; debouncer.current.add(() => { socket.emit('input:wheel', { deltaX: Math.round(wheelEvent.deltaX), deltaY: Math.round(wheelEvent.deltaY) }); setLastAction('scroll'); }); break; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 236-236: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
server/src/browser-management/classes/RemoteBrowser.ts (2)
108-113: Initialize memory management earlier if needed.
You define theperformanceMonitorand callstartPerformanceReportingin the constructor, but callinitializeMemoryManagementonly within a separate method. Consider callinginitializeMemoryManagement()in the constructor if you need memory checks immediately.
137-152: Evaluate the interval for memory cleanup thoroughly.
The garbage collection and memory usage threshold checks trigger every 60 seconds (gcInterval). Depending on usage, you may need a more or less frequent interval, or adaptive logic based on load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
package.json(3 hunks)perf/performance.ts(1 hunks)server/src/browser-management/classes/RemoteBrowser.ts(7 hunks)src/components/atoms/canvas.tsx(2 hunks)src/components/organisms/BrowserWindow.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- src/components/organisms/BrowserWindow.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/atoms/canvas.tsx
[error] 236-236: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (8)
perf/performance.ts (3)
26-54: Ensure browser compatibility for performance memory usage.
Using(performance as any).memoryis non-standard and might not be supported in all browsers. Consider feature detecting or gracefully handling environments without this property.
70-79: Validate memory usage arrays before accessing the last metric.
When callingthis.metrics.memoryUsage[this.metrics.memoryUsage.length - 1], consider the scenario wherememoryUsagemight be empty to avoid undefined references.
145-157: Gauge potential memory trend tolerance.
Currently, the threshold for a memory trend to qualify as increasing or decreasing is 1MB. You may want to calibrate this threshold based on real usage or system capacity, or expose it as a configurable parameter.src/components/atoms/canvas.tsx (2)
156-164: Confirm minimal overhead of multiple class instantiations.
You instantiateRAFScheduler,EventDebouncer,MeasurementCache, andFrontendPerformanceMonitorinuseRef, which is good to avoid re-creation on rerender. Verify that this approach aligns with your intended usage pattern, especially regarding any teardown logic inuseEffectcleanup.
189-202: Potential performance optimization for repeated bounding client rect calls.
You are caching only the canvas’s bounding rect inMeasurementCache, which is beneficial. If you expand usage for other DOM elements, confirm the LRU logic is correct for your needs.server/src/browser-management/classes/RemoteBrowser.ts (2)
Line range hint
406-419: Ensure concurrency safety when acknowledging screencast frames.
The code acknowledges the frame viaPage.screencastFrameAckafter asetTimeout. Confirm there's no scenario where the client stops responding or resets the session while pending acknowledgments are still outstanding.
648-682: Validate correct usage of throttle with your FPS target.
The throttle is set to1000 / SCREENCAST_CONFIG.targetFPSms. If 30FPS is desired, this is about 33.3ms between calls. Consider verifying that these intervals match your real-time needs without overloading the system.package.json (1)
47-47: Dependencies look good for intended usage.
Lodash is used for throttling, and sharp is for image manipulation. Ensure you maintain these dependencies at secure and stable versions.Also applies to: 69-69, 101-101
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
server/src/browser-management/classes/RemoteBrowser.ts (6)
24-28: Consider environment-specific memory thresholds.The fixed heap size threshold of 2GB might be too high for some environments. Consider making this configurable based on the environment or available system memory.
const MEMORY_CONFIG = { gcInterval: 60000, // 1 minute - maxHeapSize: 2048 * 1024 * 1024, // 2GB - heapUsageThreshold: 0.85 // 85% + maxHeapSize: process.env.MAX_HEAP_SIZE || 2048 * 1024 * 1024, // 2GB default + heapUsageThreshold: process.env.HEAP_USAGE_THRESHOLD || 0.85 // 85% default };
30-44: Adjust screenshot queue size based on FPS.With a targetFPS of 30 and maxQueueSize of 2, the queue might overflow during performance spikes. Consider increasing the queue size or making it proportional to the FPS.
const SCREENCAST_CONFIG: { format: "jpeg" | "png"; maxWidth: number; maxHeight: number; targetFPS: number; compressionQuality: number; maxQueueSize: number; } = { format: 'jpeg', maxWidth: 900, maxHeight: 400, targetFPS: 30, compressionQuality: 0.8, - maxQueueSize: 2 + maxQueueSize: 5 // Increased to handle ~167ms of frames at 30 FPS };
134-149: Enhance memory management with metrics and error handling.The memory management implementation could be improved with better error handling and metrics logging.
private initializeMemoryManagement(): void { setInterval(() => { const memoryUsage = process.memoryUsage(); const heapUsageRatio = memoryUsage.heapUsed / MEMORY_CONFIG.maxHeapSize; + + // Log memory metrics + logger.debug('Memory usage metrics:', { + heapUsed: memoryUsage.heapUsed, + heapTotal: memoryUsage.heapTotal, + rss: memoryUsage.rss, + heapUsageRatio + }); if (heapUsageRatio > MEMORY_CONFIG.heapUsageThreshold) { logger.warn('High memory usage detected, triggering cleanup'); this.performMemoryCleanup(); } }, MEMORY_CONFIG.gcInterval); } private async performMemoryCleanup(): Promise<void> { this.screenshotQueue = []; this.isProcessingScreenshot = false; if (global.gc) { + try { global.gc(); + logger.debug('Manual garbage collection completed'); + } catch (error) { + logger.error('Manual garbage collection failed:', error); + } }Also applies to: 151-171
434-452: Optimize sharp instance usage and error handling.Consider caching the sharp instance and improving error handling with specific error types.
+ private sharpInstance: sharp.Sharp | null = null; + private async optimizeScreenshot(screenshot: Buffer): Promise<Buffer> { try { - return await sharp(screenshot) + // Reuse sharp instance + if (!this.sharpInstance) { + this.sharpInstance = sharp() + .jpeg({ + quality: Math.round(SCREENCAST_CONFIG.compressionQuality * 100), + progressive: true + }) + .resize({ + width: SCREENCAST_CONFIG.maxWidth, + height: SCREENCAST_CONFIG.maxHeight, + fit: 'inside', + withoutEnlargement: true + }); + } + return await this.sharpInstance + .clone() + .toBuffer(); - } catch (error) { + } catch (error: unknown) { + if (error instanceof Error) { + logger.error('Screenshot optimization failed:', error.message); + } return screenshot; } }
644-677: Optimize screenshot emission and queue processing.The current implementation could be improved in terms of throttling precision and queue efficiency.
- private emitScreenshot = throttle(async (payload: Buffer): Promise<void> => { + private emitScreenshot = throttle( + async (payload: Buffer): Promise<void> => { if (this.isProcessingScreenshot) { if (this.screenshotQueue.length < SCREENCAST_CONFIG.maxQueueSize) { this.screenshotQueue.push(payload); } return; } this.isProcessingScreenshot = true; + const startTime = performance.now(); try { await this.performanceMonitor.measureEmitPerformance(async () => { const optimizedScreenshot = await this.optimizeScreenshot(payload); const base64Data = optimizedScreenshot.toString('base64'); const dataWithMimeType = `data:image/jpeg;base64,${base64Data}`; await new Promise<void>((resolve) => { this.socket.emit('screencast', dataWithMimeType, () => resolve()); }); }); } catch (error) { logger.error('Screenshot emission failed:', error); } finally { this.isProcessingScreenshot = false; + const processingTime = performance.now() - startTime; + logger.debug('Screenshot processing time:', processingTime); // Process next screenshot in queue if any if (this.screenshotQueue.length > 0) { const nextScreenshot = this.screenshotQueue.shift(); if (nextScreenshot) { - this.emitScreenshot(nextScreenshot); + // Use setTimeout to prevent stack overflow + setTimeout(() => this.emitScreenshot(nextScreenshot), 0); } } } - }, 1000 / SCREENCAST_CONFIG.targetFPS); + }, + 1000 / SCREENCAST_CONFIG.targetFPS, + { leading: true, trailing: true } + );
Line range hint
1-678: Overall implementation feedback.The performance improvements are well-implemented with good attention to:
- Memory management
- Screenshot optimization
- Frame rate control
- Queue management
However, consider the following architectural improvements:
- Make configuration values environment-specific
- Implement proper cleanup for all intervals
- Add comprehensive error handling
- Add detailed performance metrics logging
| private startPerformanceReporting() { | ||
| setInterval(() => { | ||
| const report = this.performanceMonitor.getPerformanceReport(); | ||
|
|
||
| console.log('Backend Performance Report:', report); | ||
|
|
||
| }, 5000); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve performance reporting implementation.
The current implementation has two issues:
- The interval is never cleared, potentially causing memory leaks
- Uses console.log instead of the logger utility
+ private performanceReportingInterval: NodeJS.Timeout | null = null;
+
private startPerformanceReporting() {
- setInterval(() => {
+ this.performanceReportingInterval = setInterval(() => {
const report = this.performanceMonitor.getPerformanceReport();
- console.log('Backend Performance Report:', report);
+ logger.info('Backend Performance Report:', report);
}, 5000);
}
+
+ private stopPerformanceReporting() {
+ if (this.performanceReportingInterval) {
+ clearInterval(this.performanceReportingInterval);
+ this.performanceReportingInterval = null;
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
amhsirak
added
Type: Enhancement
No description provided.