feat(web): unify CanvasKit and Skwasm garbage collection#183867
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a unified memory management system for native objects in Flutter's web engine, which is a significant and well-executed refactoring. It creates generic UniqueRef and CountedRef classes for handling garbage collection and reference counting, and then adapts both the CanvasKit and Skwasm backends to use this new system. This unification replaces renderer-specific logic with a consistent, type-safe, and more efficient approach. The changes are extensive but logical, and the inclusion of comprehensive tests for the generic logic, as well as for each backend, is commendable. My review found the implementation to be solid, with only a minor suggestion to improve the robustness of a mock object in one of the new test files.
a97d043 to
cde2450
Compare
|
@gemini-cli /review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified memory management system for both CanvasKit and Skwasm, which is a significant and well-executed refactoring. By creating generic UniqueRef and CountedRef classes, you've replaced renderer-specific finalization logic with a consistent, type-safe, and more efficient approach. This change correctly enables reference counting for Skwasm objects like ui.Image and ui.Picture, allowing for proper cloning. The code is well-supported by a comprehensive set of new and updated tests. My review includes a few suggestions for improving consistency in test mocks and debug properties, and for reducing redundancy in the new SkwasmObjectWrapper.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified memory management system for native objects in both CanvasKit and Skwasm renderers, which is a significant and positive architectural improvement. The new generic UniqueRef and CountedRef classes, along with the Finalizer abstraction, provide a more robust, testable, and consistent way to handle garbage collection. The refactoring has been applied consistently across the codebase, and the addition of new tests for the generic memory management logic, as well as for renderer-specific implementations, is commendable. I've identified a couple of issues related to debug-only methods that could cause issues in release builds, and a few opportunities to improve debuggability and documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request refactors native memory management by introducing generic UniqueRef and CountedRef classes, which are then extended by CanvasKit-specific CkUniqueRef and CkCountedRef to handle Skia object disposal. This change replaces previous finalization registry patterns and updates numerous CanvasKit and Skwasm implementation files to use the new system, improving consistency and control over resource lifecycle. Review comments suggest further simplifying debugDisposed getters in CkImageShader and SkwasmFragmentShader by delegating to their underlying reference objects for better consistency and reduced verbosity.
mdebbar
left a comment
There was a problem hiding this comment.
Thanks for hardening and cleaning this memory management code. Very important!
|
autosubmit label was removed for flutter/flutter/183867, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
Introduce a shared memory management system in `lib/src/engine/native_memory.dart` that both CanvasKit and Skwasm renderers now utilize. This unification replaces renderer-specific finalization logic with a consistent, efficient, and type-safe approach. Key changes: - Created `NativeMemoryFinalizer`, `UniqueRef`, and `CountedRef` as generic base classes for native resource management. - Refactored CanvasKit to use `CkUniqueRef` and `CkCountedRef`, which specialize the base classes for Skia objects. - Refactored Skwasm to use the unified `UniqueRef` and `CountedRef` abstractions, enabling proper `ui.Image` and `ui.Picture` cloning via reference counting. - Improved efficiency by using tokens to detach manually disposed objects from the `DomFinalizationRegistry`. - Added a `Finalizer` interface to support mocking and verification of finalization behavior in tests. - Comprehensive test coverage across generic engine logic and renderer-specific implementations. Testing: - Generic memory management tests: `test/engine/native_memory_test.dart` - CanvasKit-specific tests: `test/canvaskit/native_memory_test.dart` - Skwasm-specific tests: `test/skwasm/native_memory_test.dart`
3c8f61e to
af3db11
Compare
) Introduce a shared memory management system in `lib/src/engine/native_memory.dart` that both CanvasKit and Skwasm renderers now utilize. This unification replaces renderer-specific finalization logic with a consistent, efficient, and type-safe approach. Key changes: - Created `NativeMemoryFinalizer`, `UniqueRef`, and `CountedRef` as generic base classes for native resource management. - Refactored CanvasKit to use `CkUniqueRef` and `CkCountedRef`, which specialize the base classes for Skia objects. - Refactored Skwasm to use the unified `UniqueRef` and `CountedRef` abstractions, enabling proper `ui.Image` and `ui.Picture` cloning via reference counting. - Improved efficiency by using tokens to detach manually disposed objects from the `DomFinalizationRegistry`. - Added a `Finalizer` interface to support mocking and verification of finalization behavior in tests. - Comprehensive test coverage across generic engine logic and renderer-specific implementations. Testing: - Generic memory management tests: `test/engine/native_memory_test.dart` - CanvasKit-specific tests: `test/canvaskit/native_memory_test.dart` - Skwasm-specific tests: `test/skwasm/native_memory_test.dart` Fixes flutter#175628 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Introduce a shared memory management system in
lib/src/engine/native_memory.dartthat both CanvasKit and Skwasm renderers now utilize. This unification replaces renderer-specific finalization logic with a consistent, efficient, and type-safe approach.Key changes:
NativeMemoryFinalizer,UniqueRef, andCountedRefas generic base classes for native resource management.CkUniqueRefandCkCountedRef, which specialize the base classes for Skia objects.UniqueRefandCountedRefabstractions, enabling properui.Imageandui.Picturecloning via reference counting.DomFinalizationRegistry.Finalizerinterface to support mocking and verification of finalization behavior in tests.Testing:
test/engine/native_memory_test.darttest/canvaskit/native_memory_test.darttest/skwasm/native_memory_test.dartFixes #175628
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.