From 30e4b12d3ba49501e3d7e0ef4406cfe34919f70d Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 8 Nov 2024 13:44:46 -0800 Subject: [PATCH 1/4] iOS: Use fml::CFRef in place of Scoped `fml::CFRef` implements the bulk of the operations implemented in the one-off `Scoped` class. It doesn't implement the `handle()` method that allows direct writing into the internal storage of the wrapper, but that method is effectively an escape hatch for all the safety guarantees provided by the wrapper, so it seems safer to avoid adding it. Issue: https://github.com/flutter/flutter/issues/137801 --- fml/platform/darwin/cf_utils.h | 17 +-- fml/platform/darwin/cf_utils_unittests.mm | 30 +++++ impeller/golden_tests/metal_screenshot.h | 3 +- .../framework/Source/profiler_metrics_ios.mm | 108 ++++++------------ 4 files changed, 79 insertions(+), 79 deletions(-) diff --git a/fml/platform/darwin/cf_utils.h b/fml/platform/darwin/cf_utils.h index 8a1773d3d4875..71bbb2bb8d35f 100644 --- a/fml/platform/darwin/cf_utils.h +++ b/fml/platform/darwin/cf_utils.h @@ -14,6 +14,7 @@ namespace fml { /// Default retain and release implementations for CFRef. template struct CFRefTraits { + static constexpr T kNullValue = nullptr; static void Retain(T instance) { CFRetain(instance); } static void Release(T instance) { CFRelease(instance); } }; @@ -26,7 +27,7 @@ template class CFRef { public: /// Creates a new null CFRef. - CFRef() : instance_(nullptr) {} + CFRef() : instance_(CFRefTraits::kNullValue) {} /// Takes over ownership of `instance`, which is expected to be already /// retained. @@ -43,7 +44,7 @@ class CFRef { /// Move ctor: Takes over ownership of the CoreFoundation object owned /// by `other`. The object owned by `other` is set to null. CFRef(CFRef&& other) : instance_(other.instance_) { - other.instance_ = nullptr; + other.instance_ = CFRefTraits::kNullValue; } /// Takes over ownership of the CoreFoundation object owned by `other`. @@ -57,14 +58,14 @@ class CFRef { if (instance_) { CFRefTraits::Release(instance_); } - instance_ = nullptr; + instance_ = CFRefTraits::kNullValue; } /// Takes over ownership of `instance`, null by default. The object is /// expected to be already retained if non-null. /// /// Releases the previous object, if non-null. - void Reset(T instance = nullptr) { + void Reset(T instance = CFRefTraits::kNullValue) { if (instance_) { CFRefTraits::Release(instance_); } @@ -73,7 +74,7 @@ class CFRef { /// Retains a shared copy of `instance`. The previous object is released if /// non-null. Has no effect if `instance` is the currently-held object. - void Retain(T instance = nullptr) { + void Retain(T instance = CFRefTraits::kNullValue) { if (instance_ == instance) { return; } @@ -88,7 +89,7 @@ class CFRef { /// with the object. [[nodiscard]] T Release() { auto instance = instance_; - instance_ = nullptr; + instance_ = CFRefTraits::kNullValue; return instance; } @@ -108,7 +109,9 @@ class CFRef { operator T() const { return instance_; } /// Returns true if the underlying CoreFoundation object is non-null. - explicit operator bool() const { return instance_ != nullptr; } + explicit operator bool() const { + return instance_ != CFRefTraits::kNullValue; + } private: T instance_; diff --git a/fml/platform/darwin/cf_utils_unittests.mm b/fml/platform/darwin/cf_utils_unittests.mm index 2128d72554d63..ee8fe546d9550 100644 --- a/fml/platform/darwin/cf_utils_unittests.mm +++ b/fml/platform/darwin/cf_utils_unittests.mm @@ -7,6 +7,21 @@ #include "flutter/testing/testing.h" namespace fml { + +// Support variables for CFTest.SupportsCustomRetainRelease. +namespace { +static bool retain_called = false; +static bool release_called = false; +} // namespace + +// Template specialization for CFTest.SupportsCustomRetainRelease. +template <> +struct CFRefTraits { + static constexpr int64_t kNullValue = 0; + static void Retain(int64_t instance) { retain_called = true; } + static void Release(int64_t instance) { release_called = true; } +}; + namespace testing { TEST(CFTest, CanCreateRefs) { @@ -83,5 +98,20 @@ EXPECT_EQ(ref_count_before + 1u, ref_count_after); } +TEST(CFTest, SupportsCustomRetainRelease) { + CFRef ref(1); + ASSERT_EQ(ref.Get(), 1); + ASSERT_FALSE(retain_called); + ASSERT_FALSE(release_called); + ref.Reset(); + ASSERT_EQ(ref.Get(), 0); + ASSERT_TRUE(release_called); + ref.Retain(2); + ASSERT_EQ(ref.Get(), 2); + ASSERT_TRUE(retain_called); + retain_called = false; + release_called = false; +} + } // namespace testing } // namespace fml diff --git a/impeller/golden_tests/metal_screenshot.h b/impeller/golden_tests/metal_screenshot.h index 3e0cbb213522d..739c7691d238e 100644 --- a/impeller/golden_tests/metal_screenshot.h +++ b/impeller/golden_tests/metal_screenshot.h @@ -15,9 +15,10 @@ namespace fml { -/// Default retain and release implementations for CGImageRef. +/// fml::CFRef retain and release implementations for CGImageRef. template <> struct CFRefTraits { + static constexpr CGImageRef kNullValue = nullptr; static void Retain(CGImageRef instance) { CGImageRetain(instance); } static void Release(CGImageRef instance) { CGImageRelease(instance); } }; diff --git a/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm b/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm index ad97326d1116b..89f8eabbfbc26 100644 --- a/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm +++ b/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm @@ -6,6 +6,7 @@ #import +#import "flutter/fml/platform/darwin/cf_utils.h" #import "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h" #import "flutter/shell/platform/darwin/ios/framework/Source/IOKit.h" @@ -34,75 +35,37 @@ } // namespace -namespace flutter { -namespace { - -#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG || \ - FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_PROFILE - -template -T ClearValue() { - return nullptr; -} +namespace fml { +/// fml::CFRef retain and release implementations for io_object_t and related types. template <> -io_object_t ClearValue() { - return 0; -} - -template -/// Generic RAII wrapper like unique_ptr but gives access to its handle. -class Scoped { - public: - typedef void (*Deleter)(T); - explicit Scoped(Deleter deleter) : object_(ClearValue()), deleter_(deleter) {} - Scoped(T object, Deleter deleter) : object_(object), deleter_(deleter) {} - ~Scoped() { - if (object_) { - deleter_(object_); - } - } - T* handle() { - if (object_) { - deleter_(object_); - object_ = ClearValue(); - } - return &object_; - } - T get() { return object_; } - void reset(T new_value) { - if (object_) { - deleter_(object_); - } - object_ = new_value; - } - - private: - FML_DISALLOW_COPY_ASSIGN_AND_MOVE(Scoped); - T object_; - Deleter deleter_; +struct CFRefTraits { + static constexpr io_object_t kNullValue = 0; + static void Retain(io_object_t instance) { IOObjectRetain(instance); } + static void Release(io_object_t instance) { IOObjectRelease(instance); } }; -void DeleteCF(CFMutableDictionaryRef value) { - CFRelease(value); -} +} // namespace fml -void DeleteIO(io_object_t value) { - IOObjectRelease(value); -} +namespace flutter { +namespace { + +#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG || \ + FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_PROFILE std::optional FindGpuUsageInfo(io_iterator_t iterator) { - for (Scoped regEntry(IOIteratorNext(iterator), DeleteIO); regEntry.get(); - regEntry.reset(IOIteratorNext(iterator))) { - Scoped serviceDictionary(DeleteCF); - if (IORegistryEntryCreateCFProperties(regEntry.get(), serviceDictionary.handle(), - kCFAllocatorDefault, kNilOptions) != kIOReturnSuccess) { + for (fml::CFRef regEntry(IOIteratorNext(iterator)); regEntry.Get(); + regEntry.Reset(IOIteratorNext(iterator))) { + CFMutableDictionaryRef cfServiceDictionary; + if (IORegistryEntryCreateCFProperties(regEntry.Get(), &cfServiceDictionary, kCFAllocatorDefault, + kNilOptions) != kIOReturnSuccess) { continue; } - - NSDictionary* dictionary = - ((__bridge NSDictionary*)serviceDictionary.get())[@"PerformanceStatistics"]; - NSNumber* utilization = dictionary[@"Device Utilization %"]; + // Transfer ownership to ARC-managed pointer. + NSDictionary* serviceDictionary = (__bridge_transfer NSDictionary*)cfServiceDictionary; + cfServiceDictionary = nullptr; + NSDictionary* performanceStatistics = serviceDictionary[@"PerformanceStatistics"]; + NSNumber* utilization = performanceStatistics[@"Device Utilization %"]; if (utilization) { return (GpuUsageInfo){.percent_usage = [utilization doubleValue]}; } @@ -111,24 +74,27 @@ void DeleteIO(io_object_t value) { } [[maybe_unused]] std::optional FindSimulatorGpuUsageInfo() { - Scoped iterator(DeleteIO); + io_iterator_t io_iterator; if (IOServiceGetMatchingServices(kIOMasterPortDefault, IOServiceNameMatching("IntelAccelerator"), - iterator.handle()) == kIOReturnSuccess) { - return FindGpuUsageInfo(iterator.get()); + &io_iterator) == kIOReturnSuccess) { + fml::CFRef iterator(io_iterator); + return FindGpuUsageInfo(iterator.Get()); } return std::nullopt; } [[maybe_unused]] std::optional FindDeviceGpuUsageInfo() { - Scoped iterator(DeleteIO); + io_iterator_t io_iterator; if (IOServiceGetMatchingServices(kIOMasterPortDefault, IOServiceNameMatching("sgx"), - iterator.handle()) == kIOReturnSuccess) { - for (Scoped regEntry(IOIteratorNext(iterator.get()), DeleteIO); - regEntry.get(); regEntry.reset(IOIteratorNext(iterator.get()))) { - Scoped innerIterator(DeleteIO); - if (IORegistryEntryGetChildIterator(regEntry.get(), kIOServicePlane, - innerIterator.handle()) == kIOReturnSuccess) { - std::optional result = FindGpuUsageInfo(innerIterator.get()); + &io_iterator) == kIOReturnSuccess) { + fml::CFRef iterator(io_iterator); + for (fml::CFRef regEntry(IOIteratorNext(iterator.Get())); regEntry.Get(); + regEntry.Reset(IOIteratorNext(iterator.Get()))) { + io_iterator_t io_inner_iterator; + if (IORegistryEntryGetChildIterator(regEntry.Get(), kIOServicePlane, &io_inner_iterator) == + kIOReturnSuccess) { + fml::CFRef inner_iterator(io_inner_iterator); + std::optional result = FindGpuUsageInfo(inner_iterator.Get()); if (result.has_value()) { return result; } From 2c0188c38284a3ab7d1a65dc6ded2010af19f859 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 8 Nov 2024 14:31:08 -0800 Subject: [PATCH 2/4] Formatting and naming --- .../framework/Source/profiler_metrics_ios.mm | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm b/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm index 89f8eabbfbc26..50add1d75e386 100644 --- a/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm +++ b/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm @@ -54,17 +54,17 @@ FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_PROFILE std::optional FindGpuUsageInfo(io_iterator_t iterator) { - for (fml::CFRef regEntry(IOIteratorNext(iterator)); regEntry.Get(); - regEntry.Reset(IOIteratorNext(iterator))) { - CFMutableDictionaryRef cfServiceDictionary; - if (IORegistryEntryCreateCFProperties(regEntry.Get(), &cfServiceDictionary, kCFAllocatorDefault, - kNilOptions) != kIOReturnSuccess) { + for (fml::CFRef reg_entry(IOIteratorNext(iterator)); reg_entry.Get(); + reg_entry.Reset(IOIteratorNext(iterator))) { + CFMutableDictionaryRef cf_service_dictionary; + if (IORegistryEntryCreateCFProperties(reg_entry.Get(), &cf_service_dictionary, + kCFAllocatorDefault, kNilOptions) != kIOReturnSuccess) { continue; } // Transfer ownership to ARC-managed pointer. - NSDictionary* serviceDictionary = (__bridge_transfer NSDictionary*)cfServiceDictionary; - cfServiceDictionary = nullptr; - NSDictionary* performanceStatistics = serviceDictionary[@"PerformanceStatistics"]; + NSDictionary* service_dictionary = (__bridge_transfer NSDictionary*)cf_service_dictionary; + cf_service_dictionary = nullptr; + NSDictionary* performanceStatistics = service_dictionary[@"PerformanceStatistics"]; NSNumber* utilization = performanceStatistics[@"Device Utilization %"]; if (utilization) { return (GpuUsageInfo){.percent_usage = [utilization doubleValue]}; @@ -88,10 +88,10 @@ if (IOServiceGetMatchingServices(kIOMasterPortDefault, IOServiceNameMatching("sgx"), &io_iterator) == kIOReturnSuccess) { fml::CFRef iterator(io_iterator); - for (fml::CFRef regEntry(IOIteratorNext(iterator.Get())); regEntry.Get(); - regEntry.Reset(IOIteratorNext(iterator.Get()))) { + for (fml::CFRef reg_entry(IOIteratorNext(iterator.Get())); reg_entry.Get(); + reg_entry.Reset(IOIteratorNext(iterator.Get()))) { io_iterator_t io_inner_iterator; - if (IORegistryEntryGetChildIterator(regEntry.Get(), kIOServicePlane, &io_inner_iterator) == + if (IORegistryEntryGetChildIterator(reg_entry.Get(), kIOServicePlane, &io_inner_iterator) == kIOReturnSuccess) { fml::CFRef inner_iterator(io_inner_iterator); std::optional result = FindGpuUsageInfo(inner_iterator.Get()); From 93839987a50cedacd6ec3338b656b9c41d3933aa Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 8 Nov 2024 14:39:02 -0800 Subject: [PATCH 3/4] Cleanup --- fml/platform/darwin/cf_utils_unittests.mm | 25 +++++++++++------------ 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/fml/platform/darwin/cf_utils_unittests.mm b/fml/platform/darwin/cf_utils_unittests.mm index ee8fe546d9550..dba51e1c2a694 100644 --- a/fml/platform/darwin/cf_utils_unittests.mm +++ b/fml/platform/darwin/cf_utils_unittests.mm @@ -8,19 +8,18 @@ namespace fml { -// Support variables for CFTest.SupportsCustomRetainRelease. -namespace { -static bool retain_called = false; -static bool release_called = false; -} // namespace - -// Template specialization for CFTest.SupportsCustomRetainRelease. +// Template specialization used in CFTest.SupportsCustomRetainRelease. template <> struct CFRefTraits { + static bool retain_called; + static bool release_called; + static constexpr int64_t kNullValue = 0; static void Retain(int64_t instance) { retain_called = true; } static void Release(int64_t instance) { release_called = true; } }; +bool CFRefTraits::retain_called = false; +bool CFRefTraits::release_called = false; namespace testing { @@ -101,16 +100,16 @@ TEST(CFTest, SupportsCustomRetainRelease) { CFRef ref(1); ASSERT_EQ(ref.Get(), 1); - ASSERT_FALSE(retain_called); - ASSERT_FALSE(release_called); + ASSERT_FALSE(CFRefTraits::retain_called); + ASSERT_FALSE(CFRefTraits::release_called); ref.Reset(); ASSERT_EQ(ref.Get(), 0); - ASSERT_TRUE(release_called); + ASSERT_TRUE(CFRefTraits::release_called); ref.Retain(2); ASSERT_EQ(ref.Get(), 2); - ASSERT_TRUE(retain_called); - retain_called = false; - release_called = false; + ASSERT_TRUE(CFRefTraits::retain_called); + CFRefTraits::retain_called = false; + CFRefTraits::release_called = false; } } // namespace testing From 056103e44afc60d5735a77b19b855583ef2f7620 Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Fri, 8 Nov 2024 15:06:10 -0800 Subject: [PATCH 4/4] guards --- .../darwin/ios/framework/Source/profiler_metrics_ios.mm | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm b/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm index 50add1d75e386..0a4ba99a0dd1d 100644 --- a/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm +++ b/shell/platform/darwin/ios/framework/Source/profiler_metrics_ios.mm @@ -35,6 +35,9 @@ } // namespace +#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG || \ + FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_PROFILE + namespace fml { /// fml::CFRef retain and release implementations for io_object_t and related types. @@ -47,6 +50,8 @@ } // namespace fml +#endif + namespace flutter { namespace {