Skip to content
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

[web] remove obsolete object caches; simplify native object management #40894

Merged
merged 2 commits into from
Apr 4, 2023

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Apr 3, 2023

(this is attempt 3; details below)

Remove obsolete object caches and introduce a simpler way to manage native objects:

  • Remove the unused SynchronousSkiaObjectCache.
  • Introduce new library native_memory.dart that's smaller and simpler than skia_object_cache.dart.
  • Introduce two types of native object references:
    • UniqueRef a reference with a unique Dart object owner.
    • CountedRef a ref-counted reference with multiple Dart object owners.
  • All native references use GC (via FinalizationRegistry) as a back-up.
  • The new library removes everything related to object resurrection that was needed only in browsers that didn't support FinalizationRegistry. All browsers support it now.
  • Remove the ad hoc SkParagraph cache that predates the introduction of Paragraph.dispose.
  • Rewrite CkParagraph in terms of UniqueRef.
  • Rewrite CkImage in terms of CountedRef; delete SkiaObjectBox.

This PR does not migrate all objects from the old skia_object_cache.dart to native_memory.dart. That would be too big of a change. The migration can be done in multiple smaller PRs.

This also removes a few unnecessary relayouts observed in flutter/flutter#120921, but not all of them (more details in flutter/flutter#120921 (comment))

About attempt 3

More about attempt 2 here.

In this attempt 3 I'm replacing the factory with a top-level function.

Benchmarks

Now that this landed in flutter/flutter I have some benchmark numbers from the devicelab. The text_out_of_picture_bounds benchmark dropped by 3-4x (lower is better):

Screenshot 2023-04-04 at 6 13 06 PM

The repro provided in flutter/flutter#123204 dropped from 110ms/frame to 10ms/frame.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label Apr 3, 2023
@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #40894 at sha da3318d

@mdebbar
Copy link
Contributor

mdebbar commented Apr 4, 2023

The flutter-gold check is stuck due to a bug where Skia Gold gets confused when you force-push to a PR. In order to work around it, please push a new commit (not force-push).

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #40894 at sha 2cd7e56

Copy link
Contributor

@CaseyHillers CaseyHillers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the Google Testing side

@yjbanov yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 4, 2023
@yjbanov yjbanov merged commit 3e5f27d into flutter:main Apr 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 4, 2023
CaseyHillers added a commit that referenced this pull request Apr 5, 2023
yjbanov pushed a commit that referenced this pull request Apr 5, 2023
…anagement" (#40937)

Reverts #40894

This is breaking Google Testing. See b/277004090
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants