Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Document and slightly refactor ResourceManagerVK & friends. #45474

Merged
merged 14 commits into from
Sep 7, 2023

Conversation

matanlurey
Copy link
Contributor

@chinmaygarde specifically, let me know if I violated any correctness you were looking for, or if you would prefer I don't make 1 or more of the changes I made here.

@gaaclarke would love your input on C++ style specifically, or anything that stands out in general.


Summary

I plan to use the resource strategy in flutter/flutter#133198, but wanted to understand the code better first, so this is my contribution to make it a bit easier to understand (hopefully! push back if not!) and contribute tests.

  1. Renamed Reset(ResourceType) to Swap() for consistency with the std smart pointers.
  2. Moved non-trivial work out of the constructor into ::Create (style guide).
  3. Added some FML_DCHECKs to private APIs to enforce correctness.
  4. Made classes final and methods private where they were effectively that (style guide).
  5. Added tests to make sure I understood the contracts.

@matanlurey matanlurey self-assigned this Sep 6, 2023
@matanlurey
Copy link
Contributor Author

@chinmaygarde QQ:

if (auto manager = resource_manager_.lock()) {
  manager->Reclaim(std::move(resource_));
}

... What if another thread calls Reclaim() without locking? Should that also lock?

@matanlurey
Copy link
Contributor Author

I'm still working on the tests, they were more aspirational than functional :)

Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

LGTM. I'd just add a test that clears out a resource to capture coverage for most of the code.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

LGTM pending Aarons comments. Thanks!

@chinmaygarde chinmaygarde changed the title Document and slightly refactor ResourceManagerVK & friends [Impeller] Document and slightly refactor ResourceManagerVK & friends. Sep 6, 2023
@matanlurey
Copy link
Contributor Author

LGTM pending Aarons comments. Thanks!

@chinmaygarde Could you respond to #45474 (comment)?

@chinmaygarde
Copy link
Member

What if another thread calls Reclaim() without locking? Should that also lock?

Weak pointers can't be used that way. You need to convert the weak to a shared pointer using a lock. So, if you can call reclaim, you need a shared pointer (using a lock or otherwise).

@matanlurey matanlurey requested a review from gaaclarke September 7, 2023 16:28
@matanlurey matanlurey requested a review from gaaclarke September 7, 2023 16:54
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

It's not quite right yet. Sorry, it's best to make it flakeproof however unlikely the flake.

@matanlurey matanlurey requested a review from gaaclarke September 7, 2023 17:57
Copy link
Member

@gaaclarke gaaclarke left a comment

Choose a reason for hiding this comment

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

lgtm!

@matanlurey matanlurey added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 7, 2023
@auto-submit auto-submit bot merged commit 8d07c29 into flutter:main Sep 7, 2023
@matanlurey matanlurey deleted the impeller-resource-manager-vk branch September 7, 2023 18:33
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 7, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 7, 2023
…134240)

flutter/engine@f0b718e...8d07c29

2023-09-07 matanlurey@users.noreply.github.com [Impeller] Document and slightly refactor `ResourceManagerVK` & friends. (flutter/engine#45474)
2023-09-07 uysalere@gmail.com [fuchsia] Restructure Flatland vsync loop (flutter/engine#45531)
2023-09-07 skia-flutter-autoroll@skia.org Roll Skia from 16df0c27bc0e to c3d6534b0ac3 (3 revisions) (flutter/engine#45543)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
matanlurey added a commit that referenced this pull request Sep 12, 2023
Fixes flutter/flutter#134482.

I wish I knew how to create the thread in `::Create()`, but given the
constructor is private I feel less bad about reverting part of
#45474. Added a test that
consistently fails before this PR and passes after.

(Unblocks flutter/flutter#133198)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants