perf: eliminate redundant copies, fix memory leaks, use platform abstraction#50
Conversation
…raction Capture pipeline: - Skip RGBX→RGB conversion in xlib capture (RGBX handled natively by QImage) - Pass pre-computed raw bytes to pil_to_qimage to avoid double tobytes() - Add RGBX format support to pil_to_qimage via Format_RGBX8888 - Close superseded PIL images during frame deduplication (memory leak fix) Platform layer: - Singleton cache for factory functions (avoid redundant instance creation) - get_window_list() now uses wmctrl cache (consistent with get_eve_windows) - _activate_window uses platform abstraction instead of raw subprocess Intel parser: - Replace O(n) regex scan over 200+ ship names with set intersection - Tokenize message once, use fast set lookup for single-word ships Tests: - Add setup_method to factory tests to clear singleton cache for isolation https://claude.ai/code/session_01QuCbaZcM64kfksMbKkSKfK
Summary of ChangesHello @AreteDriver, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's performance and memory efficiency by optimizing image processing within the capture pipeline and refining the Intel parser's ship extraction logic. It also improves code maintainability and robustness by introducing a singleton cache for platform-specific components and abstracting window activation operations, alongside addressing potential memory leaks. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant performance improvements, memory leak fixes, and a major refactoring to a platform abstraction layer. The changes are well-executed and align with the goals described. The use of singleton caching for platform-specific managers and optimizations in the image processing pipeline are particularly effective.
My review includes a few suggestions:
- A critical fix for a test that will fail due to an API change in the capture logic.
- A recommendation to reduce code duplication in the new platform factory functions for better maintainability.
- A couple of minor performance tweaks in the intel parser and image conversion utility.
Overall, this is a high-quality contribution that substantially improves the application's performance and structure. Addressing the identified issues will make it even better.
| img = Image.frombytes("RGBX", (geom.width, geom.height), raw.data, "raw", "BGRX") | ||
| return img.convert("RGB") | ||
| # Return RGBX directly — avoids a full-image copy from .convert("RGB") | ||
| return Image.frombytes("RGBX", (geom.width, geom.height), raw.data, "raw", "BGRX") |
There was a problem hiding this comment.
This change to return an 'RGBX' image directly is a great performance optimization. However, it seems the corresponding test TestXlibCapture.test_capture_xlib_success in tests/test_platform.py was not updated. The test still asserts result.mode == 'RGB' at line 1423, which will now fail. Please update the test to assert for 'RGBX'.
| words = set(re.findall(r"[a-z]+", message.lower())) | ||
| # Single-word ships via fast set intersection | ||
| found = list(words & self.SHIP_TYPES) | ||
| # Multi-word ship names (e.g. "force auxiliary") need substring check | ||
| msg_lower = message.lower() |
There was a problem hiding this comment.
This is a great performance improvement. To make it even more efficient, you can avoid calling .lower() twice on the message.
| words = set(re.findall(r"[a-z]+", message.lower())) | |
| # Single-word ships via fast set intersection | |
| found = list(words & self.SHIP_TYPES) | |
| # Multi-word ship names (e.g. "force auxiliary") need substring check | |
| msg_lower = message.lower() | |
| msg_lower = message.lower() | |
| words = set(re.findall(r"[a-z]+", msg_lower)) | |
| # Single-word ships via fast set intersection | |
| found = list(words & self.SHIP_TYPES) | |
| # Multi-word ship names (e.g. "force auxiliary") need substring check |
| def get_window_manager() -> WindowManager: | ||
| """Create a platform-appropriate WindowManager instance. | ||
| """Get the platform-appropriate WindowManager singleton. | ||
|
|
||
| Returns: | ||
| WindowManager implementation for current platform | ||
|
|
||
| Raises: | ||
| RuntimeError: If platform is not supported | ||
| """ | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import WindowManagerWindows | ||
| key = "window_manager" | ||
| if key not in _singleton_cache: | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import WindowManagerWindows | ||
|
|
||
| return WindowManagerWindows() | ||
| elif platform == "linux": | ||
| from .linux import WindowManagerLinux | ||
| _singleton_cache[key] = WindowManagerWindows() | ||
| elif platform == "linux": | ||
| from .linux import WindowManagerLinux | ||
|
|
||
| return WindowManagerLinux() | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| _singleton_cache[key] = WindowManagerLinux() | ||
| else: | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| return _singleton_cache[key] # type: ignore[return-value] | ||
|
|
||
|
|
||
| def get_window_capture(max_workers: int = 4) -> WindowCapture: | ||
| """Create a platform-appropriate WindowCapture instance. | ||
| """Get the platform-appropriate WindowCapture singleton. | ||
|
|
||
| Args: | ||
| max_workers: Number of capture worker threads | ||
| max_workers: Number of capture worker threads (only used on first call) | ||
|
|
||
| Returns: | ||
| WindowCapture implementation for current platform | ||
|
|
||
| Raises: | ||
| RuntimeError: If platform is not supported | ||
| """ | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import WindowCaptureWindows | ||
| key = "window_capture" | ||
| if key not in _singleton_cache: | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import WindowCaptureWindows | ||
|
|
||
| return WindowCaptureWindows(max_workers=max_workers) | ||
| elif platform == "linux": | ||
| from .linux import WindowCaptureLinux | ||
| _singleton_cache[key] = WindowCaptureWindows(max_workers=max_workers) | ||
| elif platform == "linux": | ||
| from .linux import WindowCaptureLinux | ||
|
|
||
| return WindowCaptureLinux(max_workers=max_workers) | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| _singleton_cache[key] = WindowCaptureLinux(max_workers=max_workers) | ||
| else: | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| return _singleton_cache[key] # type: ignore[return-value] | ||
|
|
||
|
|
||
| def get_screen_manager() -> ScreenManager: | ||
| """Create a platform-appropriate ScreenManager instance. | ||
| """Get the platform-appropriate ScreenManager singleton. | ||
|
|
||
| Returns: | ||
| ScreenManager implementation for current platform | ||
|
|
||
| Raises: | ||
| RuntimeError: If platform is not supported | ||
| """ | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import ScreenManagerWindows | ||
| key = "screen_manager" | ||
| if key not in _singleton_cache: | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import ScreenManagerWindows | ||
|
|
||
| return ScreenManagerWindows() | ||
| elif platform == "linux": | ||
| from .linux import ScreenManagerLinux | ||
| _singleton_cache[key] = ScreenManagerWindows() | ||
| elif platform == "linux": | ||
| from .linux import ScreenManagerLinux | ||
|
|
||
| return ScreenManagerLinux() | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| _singleton_cache[key] = ScreenManagerLinux() | ||
| else: | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| return _singleton_cache[key] # type: ignore[return-value] | ||
|
|
||
|
|
||
| def get_eve_path_resolver() -> EVEPathResolver: | ||
| """Create a platform-appropriate EVEPathResolver instance. | ||
| """Get the platform-appropriate EVEPathResolver singleton. | ||
|
|
||
| Returns: | ||
| EVEPathResolver implementation for current platform | ||
|
|
||
| Raises: | ||
| RuntimeError: If platform is not supported | ||
| """ | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import EVEPathResolverWindows | ||
| key = "eve_path_resolver" | ||
| if key not in _singleton_cache: | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import EVEPathResolverWindows | ||
|
|
||
| return EVEPathResolverWindows() | ||
| elif platform == "linux": | ||
| from .linux import EVEPathResolverLinux | ||
| _singleton_cache[key] = EVEPathResolverWindows() | ||
| elif platform == "linux": | ||
| from .linux import EVEPathResolverLinux | ||
|
|
||
| return EVEPathResolverLinux() | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| _singleton_cache[key] = EVEPathResolverLinux() | ||
| else: | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| return _singleton_cache[key] # type: ignore[return-value] | ||
|
|
||
|
|
||
| def get_hotkey_helper() -> HotkeyHelper: | ||
| """Create a platform-appropriate HotkeyHelper instance. | ||
| """Get the platform-appropriate HotkeyHelper singleton. | ||
|
|
||
| Returns: | ||
| HotkeyHelper implementation for current platform | ||
|
|
||
| Raises: | ||
| RuntimeError: If platform is not supported | ||
| """ | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import HotkeyHelperWindows | ||
|
|
||
| return HotkeyHelperWindows() | ||
| elif platform == "linux": | ||
| from .linux import HotkeyHelperLinux | ||
|
|
||
| return HotkeyHelperLinux() | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| key = "hotkey_helper" | ||
| if key not in _singleton_cache: | ||
| platform = get_platform_name() | ||
| if platform == "windows": | ||
| from .windows import HotkeyHelperWindows | ||
|
|
||
| _singleton_cache[key] = HotkeyHelperWindows() | ||
| elif platform == "linux": | ||
| from .linux import HotkeyHelperLinux | ||
|
|
||
| _singleton_cache[key] = HotkeyHelperLinux() | ||
| else: | ||
| raise RuntimeError(f"Unsupported platform: {platform}") | ||
| return _singleton_cache[key] # type: ignore[return-value] |
There was a problem hiding this comment.
The introduction of a singleton cache is a good performance improvement. However, the implementation is very repetitive across all the factory functions (get_window_manager, get_window_capture, etc.). To improve maintainability and reduce code duplication, consider abstracting the caching and platform-switching logic into a generic helper function or a decorator. This would make the code much more DRY (Don't Repeat Yourself).
src/argus_overview/ui/main_tab.py
Outdated
| _FORMAT_MAP = { | ||
| "RGB": (3, QImage.Format.Format_RGB888), | ||
| "RGBX": (4, QImage.Format.Format_RGBX8888), | ||
| "RGBA": (4, QImage.Format.Format_RGBA8888), | ||
| "L": (1, QImage.Format.Format_Grayscale8), | ||
| } |
There was a problem hiding this comment.
This refactoring is much cleaner. For a minor performance improvement, consider defining _FORMAT_MAP as a module-level constant outside of the pil_to_qimage function. This will prevent it from being recreated on every function call.
…layer - Update _activate_window tests to use capture_system mock instead of subprocess.run (7 tests fixed in test_main_window_v21.py) - Fix test_get_window_list to patch _get_wmctrl_window_list instead of run_x11_subprocess (test_platform.py) - Fix test_capture_xlib_success to assert RGBX mode (matches new xlib capture that skips RGB conversion) - Fix test_get_window_list_exception to patch correct target - Move _FORMAT_MAP from pil_to_qimage() body to module-level constant (avoids dict recreation on every frame) - Add RGBX and raw_data kwarg tests for pil_to_qimage - Run ruff format on files with formatting violations Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Capture pipeline:
Platform layer:
Intel parser:
Tests:
https://claude.ai/code/session_01QuCbaZcM64kfksMbKkSKfK