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

[Impeller] Add RAII wrappers for VMA objects. #43626

Merged
merged 2 commits into from
Jul 13, 2023
Merged

[Impeller] Add RAII wrappers for VMA objects. #43626

merged 2 commits into from
Jul 13, 2023

Conversation

chinmaygarde
Copy link
Member

Uses fml::UniqueObject<T>.

@chinmaygarde chinmaygarde self-assigned this Jul 13, 2023
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@chinmaygarde
Copy link
Member Author

Refactor to address the nit in #43564 (comment).

chinmaygarde
chinmaygarde commented Jul 13, 2023
View reviewed changes
impeller/renderer/backend/vulkan/allocator_vk.cc
return;
}
TRACE_EVENT0("impeller", "DestroyDeviceTexture");
image_view.reset();
Copy link
Member Author

Choose a reason for hiding this comment

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

The image view is already a unique object. It had to be reset manually earlier because the image was explicitly collected. With the RAII wrapper, there is no need as the ivar destruction order holds.

jonahwilliams
jonahwilliams reviewed Jul 13, 2023
View reviewed changes
impeller/renderer/backend/vulkan/allocator_vk.h
fml::RefPtr<vulkan::VulkanProcTable> vk_;
VmaAllocator allocator_ = {};
VmaPool staging_buffer_pools_[kPoolCount] = {};
UniqueAllocatorVMA allocator_;
Copy link
Contributor

Choose a reason for hiding this comment

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

So, to state the obvious as I'm still a C++ noob:

The RAII wrappers handle destroying the native resources, and we always go though destructors in reverse order. So staging_buffer_pools_ being declared after allocator_ ensures that we destroy the pools with the allocator before destroying the allocator itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right.

jonahwilliams
jonahwilliams approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 13, 2023
@auto-submit auto-submit bot merged commit 356f0a2 into flutter:main Jul 13, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 13, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023
@engine-flutter-autoroll
356f0a2fc [Impeller] Add RAII wrappers for VMA objects. (flutter/engi…
61fee0c 
…ne#43626)
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Jul 13, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions) (#…
ee4208f 
…130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 [email protected] Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 [email protected] Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 [email protected] Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 [email protected] Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 [email protected] Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 [email protected] [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 [email protected] Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 [email protected] [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 [email protected] Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 [email protected] Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 [email protected] Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 [email protected] Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 [email protected] Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 [email protected] [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 [email protected] [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 [email protected] Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 [email protected] move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 [email protected] Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 [email protected] Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 [email protected] Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 [email protected] Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 [email protected] [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 [email protected] Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 [email protected] Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 [email protected] Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 [email protected] Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 [email protected] Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 [email protected] [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

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 [email protected],[email protected] 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
...
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 13, 2023
@engine-flutter-autoroll @LouiseHsu
Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions) (f…
d6f8d61 
…lutter#130496)

Roll Flutter Engine from 16e2ab7e986c to b4080361f2f9 (28 revisions)

flutter/engine@16e2ab7...b408036

2023-07-13 [email protected] Roll Skia from e5ec341bc3ca to 56b68ce6196c (1 revision) (flutter/engine#43633)
2023-07-13 [email protected] Roll Dart SDK from f499e91e8cb2 to ade4dae923f3 (1 revision) (flutter/engine#43632)
2023-07-13 [email protected] Roll Skia from 811b046c673b to e5ec341bc3ca (1 revision) (flutter/engine#43631)
2023-07-13 [email protected] Make GOMA state automatic by default (flutter/engine#43584)
2023-07-13 [email protected] Roll Skia from c8da0c657c4e to 811b046c673b (3 revisions) (flutter/engine#43630)
2023-07-13 [email protected] [Impeller] Remove unactionable error logs and use structure chains for instance creation. (flutter/engine#43629)
2023-07-13 [email protected] Roll Skia from 7f391ea9164e to c8da0c657c4e (1 revision) (flutter/engine#43628)
2023-07-13 [email protected] [Impeller] Add RAII wrappers for VMA objects. (flutter/engine#43626)
2023-07-13 [email protected] Roll Fuchsia Linux SDK from 1STsUj0X5YgpiSNEb... to xBJq6PsO5ebblODMe... (flutter/engine#43627)
2023-07-13 [email protected] Roll Skia from 6ed93436d57c to 7f391ea9164e (1 revision) (flutter/engine#43625)
2023-07-13 [email protected] Roll Dart SDK from 8f8f281ccdc6 to f499e91e8cb2 (3 revisions) (flutter/engine#43623)
2023-07-13 [email protected] Roll Fuchsia Mac SDK from 0fvk838jTDNQ_l43k... to 3C7P0w8ySmtqpyi3S... (flutter/engine#43622)
2023-07-13 [email protected] Roll Skia from bedc92598644 to 6ed93436d57c (3 revisions) (flutter/engine#43621)
2023-07-13 [email protected] [Impeller] Add support to embedder for Impeller on GL (via Angle on Windows). (flutter/engine#43388)
2023-07-12 [email protected] [Impeller] Allocate buffers out of a pool on the raster thread. (flutter/engine#43564)
2023-07-12 [email protected] Roll Skia from 4e989b1564ee to bedc92598644 (1 revision) (flutter/engine#43617)
2023-07-12 [email protected] move rtree and canvas_spy sources to Fuchsia sub-directory (flutter/engine#43615)
2023-07-12 [email protected] Roll Skia from 68e3c0b3eea7 to 4e989b1564ee (6 revisions) (flutter/engine#43614)
2023-07-12 [email protected] Document (and assert) that channel names can't contains nulls (flutter/engine#43593)
2023-07-12 [email protected] Reland "Refactor JSNumber.toDart and Object.toJS" (flutter/engine#43363)
2023-07-12 [email protected] Add a flag to `ParagraphBuilder` for rounding hack migration (flutter/engine#43118)
2023-07-12 [email protected] [Impeller] Fixes for asymmetric stencil descriptors (flutter/engine#43535)
2023-07-12 [email protected] Roll Skia from adeeb8d50f7c to 68e3c0b3eea7 (1 revision) (flutter/engine#43609)
2023-07-12 [email protected] Add comment to use of 3x3 mapRect in TransformLayer (flutter/engine#43608)
2023-07-12 [email protected] Roll Skia from a251a36ea519 to adeeb8d50f7c (1 revision) (flutter/engine#43606)
2023-07-12 [email protected] Roll Dart SDK from b95f6531c726 to 8f8f281ccdc6 (2 revisions) (flutter/engine#43607)
2023-07-12 [email protected] Roll Skia from ebc149cff431 to a251a36ea519 (2 revisions) (flutter/engine#43604)
2023-07-12 [email protected] [Impeller] Use new SkParagraph APIs for stroked text. (flutter/engine#41735)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from 1STsUj0X5Ygp to xBJq6PsO5ebb
  fuchsia/sdk/core/mac-amd64 from 0fvk838jTDNQ to 3C7P0w8ySmtq

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 [email protected],[email protected] 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
...
kjlubick pushed a commit to kjlubick/engine that referenced this pull request Jul 14, 2023
@chinmaygarde @kjlubick
[Impeller] Add RAII wrappers for VMA objects. (flutter#43626)
7df3a04 
Uses `fml::UniqueObject<T>`.
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this pull request Jul 20, 2023
@chinmaygarde @harryterkelsen
[Impeller] Add RAII wrappers for VMA objects. (flutter#43626)
05a17c2 
Uses `fml::UniqueObject<T>`.
@chinmaygarde chinmaygarde deleted the raiivma branch August 22, 2024 19:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@chinmaygarde chinmaygarde

Labels

autosubmit Merge PR when tree becomes green via auto submit App e: impeller

Projects

No open projects
Impeller
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chinmaygarde @jonahwilliams