From 86ee89f73e4f4799b3ebcc0b5c65837c9601fe6d Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 20 Apr 2021 16:58:01 +0200 Subject: [PATCH] Simplify reference counting implementation of PendingTaskSafetyFlag. On a 32bit system, this reduces the allocation size of the flag down from 12 bytes to 8, and removes the need for a vtable (the extra 4 bytes are the vtable pointer). The downside is that this change makes the binary layout of the flag, less compatible with RefCountedObject<> based reference counting objects and thus we don't immediately get the benefits of identical COMDAT folding and subsequently there's a slight binary size increase. With wider use, the binary size benefits will come. Bug: none Change-Id: I04129771790a3258d6accaf0ab1258b7a798a55e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215681 Reviewed-by: Mirko Bonadei Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#33793} --- api/BUILD.gn | 6 ++- api/ref_counted_base.h | 48 ++++++++++++++++++- api/stats/rtc_stats_report.h | 13 +++-- .../full_screen_window_detector.h | 3 +- .../desktop_capture/linux/shared_x_display.h | 11 +++-- .../mac/desktop_configuration_monitor.h | 8 ++-- pc/BUILD.gn | 1 + pc/connection_context.cc | 3 +- pc/connection_context.h | 8 ++-- rtc_base/BUILD.gn | 1 + rtc_base/rtc_certificate.cc | 7 ++- rtc_base/rtc_certificate.h | 9 ++-- rtc_base/task_utils/BUILD.gn | 2 +- .../task_utils/pending_task_safety_flag.cc | 8 ++-- .../task_utils/pending_task_safety_flag.h | 9 ++-- .../src/jni/audio_device/opensles_common.cc | 2 - .../src/jni/audio_device/opensles_common.h | 5 +- .../src/jni/pc/add_ice_candidate_observer.h | 5 +- stats/rtc_stats_report.cc | 5 +- 19 files changed, 105 insertions(+), 49 deletions(-) diff --git a/api/BUILD.gn b/api/BUILD.gn index 4f729d5c77..c8e29526cb 100644 --- a/api/BUILD.gn +++ b/api/BUILD.gn @@ -561,6 +561,7 @@ rtc_source_set("rtc_stats_api") { deps = [ ":scoped_refptr", + "../api:refcountedbase", "../rtc_base:checks", "../rtc_base:rtc_base_approved", "../rtc_base/system:rtc_export", @@ -682,7 +683,10 @@ rtc_source_set("array_view") { rtc_source_set("refcountedbase") { visibility = [ "*" ] sources = [ "ref_counted_base.h" ] - deps = [ "../rtc_base:rtc_base_approved" ] + deps = [ + "../rtc_base:macromagic", + "../rtc_base:refcount", + ] } rtc_library("ice_transport_factory") { diff --git a/api/ref_counted_base.h b/api/ref_counted_base.h index a1761db851..238765be89 100644 --- a/api/ref_counted_base.h +++ b/api/ref_counted_base.h @@ -10,8 +10,9 @@ #ifndef API_REF_COUNTED_BASE_H_ #define API_REF_COUNTED_BASE_H_ +#include + #include "rtc_base/constructor_magic.h" -#include "rtc_base/ref_count.h" #include "rtc_base/ref_counter.h" namespace rtc { @@ -38,6 +39,51 @@ class RefCountedBase { RTC_DISALLOW_COPY_AND_ASSIGN(RefCountedBase); }; +// Template based version of `RefCountedBase` for simple implementations that do +// not need (or want) destruction via virtual destructor or the overhead of a +// vtable. +// +// To use: +// struct MyInt : public rtc::RefCountedNonVirtual { +// int foo_ = 0; +// }; +// +// rtc::scoped_refptr my_int(new MyInt()); +// +// sizeof(MyInt) on a 32 bit system would then be 8, int + refcount and no +// vtable generated. +template +class RefCountedNonVirtual { + public: + RefCountedNonVirtual() = default; + + void AddRef() const { ref_count_.IncRef(); } + RefCountReleaseStatus Release() const { + // If you run into this assert, T has virtual methods. There are two + // options: + // 1) The class doesn't actually need virtual methods, the type is complete + // so the virtual attribute(s) can be removed. + // 2) The virtual methods are a part of the design of the class. In this + // case you can consider using `RefCountedBase` instead or alternatively + // use `rtc::RefCountedObject`. + static_assert(!std::is_polymorphic::value, + "T has virtual methods. RefCountedBase is a better fit."); + const auto status = ref_count_.DecRef(); + if (status == RefCountReleaseStatus::kDroppedLastRef) { + delete static_cast(this); + } + return status; + } + + protected: + ~RefCountedNonVirtual() = default; + + private: + mutable webrtc::webrtc_impl::RefCounter ref_count_{0}; + + RTC_DISALLOW_COPY_AND_ASSIGN(RefCountedNonVirtual); +}; + } // namespace rtc #endif // API_REF_COUNTED_BASE_H_ diff --git a/api/stats/rtc_stats_report.h b/api/stats/rtc_stats_report.h index 94bd813b07..0fe5ce91f9 100644 --- a/api/stats/rtc_stats_report.h +++ b/api/stats/rtc_stats_report.h @@ -19,9 +19,11 @@ #include #include +#include "api/ref_counted_base.h" #include "api/scoped_refptr.h" #include "api/stats/rtc_stats.h" -#include "rtc_base/ref_count.h" +// TODO(tommi): Remove this include after fixing iwyu issue in chromium. +// See: third_party/blink/renderer/platform/peerconnection/rtc_stats.cc #include "rtc_base/ref_counted_object.h" #include "rtc_base/system/rtc_export.h" @@ -29,7 +31,8 @@ namespace webrtc { // A collection of stats. // This is accessible as a map from |RTCStats::id| to |RTCStats|. -class RTC_EXPORT RTCStatsReport : public rtc::RefCountInterface { +class RTC_EXPORT RTCStatsReport final + : public rtc::RefCountedNonVirtual { public: typedef std::map> StatsMap; @@ -107,11 +110,11 @@ class RTC_EXPORT RTCStatsReport : public rtc::RefCountInterface { // listing all of its stats objects. std::string ToJson() const; - friend class rtc::RefCountedObject; + protected: + friend class rtc::RefCountedNonVirtual; + ~RTCStatsReport() = default; private: - ~RTCStatsReport() override; - int64_t timestamp_us_; StatsMap stats_; }; diff --git a/modules/desktop_capture/full_screen_window_detector.h b/modules/desktop_capture/full_screen_window_detector.h index 46fb607b7d..ca30d95de4 100644 --- a/modules/desktop_capture/full_screen_window_detector.h +++ b/modules/desktop_capture/full_screen_window_detector.h @@ -32,7 +32,8 @@ namespace webrtc { // window using criteria provided by application specific // FullScreenApplicationHandler. -class FullScreenWindowDetector : public rtc::RefCountedBase { +class FullScreenWindowDetector + : public rtc::RefCountedNonVirtual { public: using ApplicationHandlerFactory = std::function( diff --git a/modules/desktop_capture/linux/shared_x_display.h b/modules/desktop_capture/linux/shared_x_display.h index 64c498c134..dd52e456ca 100644 --- a/modules/desktop_capture/linux/shared_x_display.h +++ b/modules/desktop_capture/linux/shared_x_display.h @@ -28,7 +28,8 @@ typedef union _XEvent XEvent; namespace webrtc { // A ref-counted object to store XDisplay connection. -class RTC_EXPORT SharedXDisplay : public rtc::RefCountedBase { +class RTC_EXPORT SharedXDisplay + : public rtc::RefCountedNonVirtual { public: class XEventHandler { public: @@ -38,9 +39,6 @@ class RTC_EXPORT SharedXDisplay : public rtc::RefCountedBase { virtual bool HandleXEvent(const XEvent& event) = 0; }; - // Takes ownership of |display|. - explicit SharedXDisplay(Display* display); - // Creates a new X11 Display for the |display_name|. NULL is returned if X11 // connection failed. Equivalent to CreateDefault() when |display_name| is // empty. @@ -65,8 +63,11 @@ class RTC_EXPORT SharedXDisplay : public rtc::RefCountedBase { void IgnoreXServerGrabs(); + ~SharedXDisplay(); + protected: - ~SharedXDisplay() override; + // Takes ownership of |display|. + explicit SharedXDisplay(Display* display); private: typedef std::map > EventHandlersMap; diff --git a/modules/desktop_capture/mac/desktop_configuration_monitor.h b/modules/desktop_capture/mac/desktop_configuration_monitor.h index 46a66d1d4c..aa0ebfbacc 100644 --- a/modules/desktop_capture/mac/desktop_configuration_monitor.h +++ b/modules/desktop_capture/mac/desktop_configuration_monitor.h @@ -25,15 +25,15 @@ namespace webrtc { // The class provides functions to synchronize capturing and display // reconfiguring across threads, and the up-to-date MacDesktopConfiguration. -class DesktopConfigurationMonitor : public rtc::RefCountedBase { +class DesktopConfigurationMonitor final + : public rtc::RefCountedNonVirtual { public: DesktopConfigurationMonitor(); + ~DesktopConfigurationMonitor(); + // Returns the current desktop configuration. MacDesktopConfiguration desktop_configuration(); - protected: - ~DesktopConfigurationMonitor() override; - private: static void DisplaysReconfiguredCallback(CGDirectDisplayID display, CGDisplayChangeSummaryFlags flags, diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 2a18414981..106ee55025 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -329,6 +329,7 @@ rtc_library("connection_context") { "../api:callfactory_api", "../api:libjingle_peerconnection_api", "../api:media_stream_interface", + "../api:refcountedbase", "../api:scoped_refptr", "../api:sequence_checker", "../api/neteq:neteq_api", diff --git a/pc/connection_context.cc b/pc/connection_context.cc index 2904714c87..8d6ee636f3 100644 --- a/pc/connection_context.cc +++ b/pc/connection_context.cc @@ -17,7 +17,6 @@ #include "api/transport/field_trial_based_config.h" #include "media/sctp/sctp_transport_factory.h" #include "rtc_base/helpers.h" -#include "rtc_base/ref_counted_object.h" #include "rtc_base/task_utils/to_queued_task.h" #include "rtc_base/time_utils.h" @@ -76,7 +75,7 @@ std::unique_ptr MaybeCreateSctpFactory( // Static rtc::scoped_refptr ConnectionContext::Create( PeerConnectionFactoryDependencies* dependencies) { - return new rtc::RefCountedObject(dependencies); + return new ConnectionContext(dependencies); } ConnectionContext::ConnectionContext( diff --git a/pc/connection_context.h b/pc/connection_context.h index 0c69c17a5b..8fad13c10c 100644 --- a/pc/connection_context.h +++ b/pc/connection_context.h @@ -17,6 +17,7 @@ #include "api/call/call_factory_interface.h" #include "api/media_stream_interface.h" #include "api/peer_connection_interface.h" +#include "api/ref_counted_base.h" #include "api/scoped_refptr.h" #include "api/sequence_checker.h" #include "api/transport/sctp_transport_factory_interface.h" @@ -27,7 +28,6 @@ #include "rtc_base/checks.h" #include "rtc_base/network.h" #include "rtc_base/network_monitor_factory.h" -#include "rtc_base/ref_count.h" #include "rtc_base/rtc_certificate_generator.h" #include "rtc_base/thread.h" #include "rtc_base/thread_annotations.h" @@ -47,7 +47,8 @@ class RtcEventLog; // interferes with the operation of other PeerConnections. // // This class must be created and destroyed on the signaling thread. -class ConnectionContext : public rtc::RefCountInterface { +class ConnectionContext final + : public rtc::RefCountedNonVirtual { public: // Creates a ConnectionContext. May return null if initialization fails. // The Dependencies class allows simple management of all new dependencies @@ -92,7 +93,8 @@ class ConnectionContext : public rtc::RefCountInterface { protected: explicit ConnectionContext(PeerConnectionFactoryDependencies* dependencies); - virtual ~ConnectionContext(); + friend class rtc::RefCountedNonVirtual; + ~ConnectionContext(); private: // The following three variables are used to communicate between the diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 0fdf534dd5..168c40943d 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -916,6 +916,7 @@ rtc_library("rtc_base") { ":threading", "../api:array_view", "../api:function_view", + "../api:refcountedbase", "../api:scoped_refptr", "../api:sequence_checker", "../api/numerics", diff --git a/rtc_base/rtc_certificate.cc b/rtc_base/rtc_certificate.cc index 04ae99685d..937defc6c2 100644 --- a/rtc_base/rtc_certificate.cc +++ b/rtc_base/rtc_certificate.cc @@ -13,7 +13,6 @@ #include #include "rtc_base/checks.h" -#include "rtc_base/ref_counted_object.h" #include "rtc_base/ssl_certificate.h" #include "rtc_base/ssl_identity.h" #include "rtc_base/time_utils.h" @@ -22,14 +21,14 @@ namespace rtc { scoped_refptr RTCCertificate::Create( std::unique_ptr identity) { - return new RefCountedObject(identity.release()); + return new RTCCertificate(identity.release()); } RTCCertificate::RTCCertificate(SSLIdentity* identity) : identity_(identity) { RTC_DCHECK(identity_); } -RTCCertificate::~RTCCertificate() {} +RTCCertificate::~RTCCertificate() = default; uint64_t RTCCertificate::Expires() const { int64_t expires = GetSSLCertificate().CertificateExpirationTime(); @@ -67,7 +66,7 @@ scoped_refptr RTCCertificate::FromPEM( SSLIdentity::CreateFromPEMStrings(pem.private_key(), pem.certificate())); if (!identity) return nullptr; - return new RefCountedObject(identity.release()); + return new RTCCertificate(identity.release()); } bool RTCCertificate::operator==(const RTCCertificate& certificate) const { diff --git a/rtc_base/rtc_certificate.h b/rtc_base/rtc_certificate.h index 45e51b5b1b..ce9aa47512 100644 --- a/rtc_base/rtc_certificate.h +++ b/rtc_base/rtc_certificate.h @@ -16,8 +16,8 @@ #include #include +#include "api/ref_counted_base.h" #include "api/scoped_refptr.h" -#include "rtc_base/ref_count.h" #include "rtc_base/system/rtc_export.h" namespace rtc { @@ -49,7 +49,8 @@ class RTCCertificatePEM { // A thin abstraction layer between "lower level crypto stuff" like // SSLCertificate and WebRTC usage. Takes ownership of some lower level objects, // reference counting protects these from premature destruction. -class RTC_EXPORT RTCCertificate : public RefCountInterface { +class RTC_EXPORT RTCCertificate final + : public RefCountedNonVirtual { public: // Takes ownership of |identity|. static scoped_refptr Create( @@ -82,7 +83,9 @@ class RTC_EXPORT RTCCertificate : public RefCountInterface { protected: explicit RTCCertificate(SSLIdentity* identity); - ~RTCCertificate() override; + + friend class RefCountedNonVirtual; + ~RTCCertificate(); private: // The SSLIdentity is the owner of the SSLCertificate. To protect our diff --git a/rtc_base/task_utils/BUILD.gn b/rtc_base/task_utils/BUILD.gn index 39e4ba1100..ca9a14a324 100644 --- a/rtc_base/task_utils/BUILD.gn +++ b/rtc_base/task_utils/BUILD.gn @@ -33,7 +33,7 @@ rtc_library("pending_task_safety_flag") { ] deps = [ "..:checks", - "..:refcount", + "../../api:refcountedbase", "../../api:scoped_refptr", "../../api:sequence_checker", "../system:no_unique_address", diff --git a/rtc_base/task_utils/pending_task_safety_flag.cc b/rtc_base/task_utils/pending_task_safety_flag.cc index b83d714916..57b3f6ce88 100644 --- a/rtc_base/task_utils/pending_task_safety_flag.cc +++ b/rtc_base/task_utils/pending_task_safety_flag.cc @@ -10,19 +10,17 @@ #include "rtc_base/task_utils/pending_task_safety_flag.h" -#include "rtc_base/ref_counted_object.h" - namespace webrtc { // static rtc::scoped_refptr PendingTaskSafetyFlag::Create() { - return new rtc::RefCountedObject(true); + return new PendingTaskSafetyFlag(true); } rtc::scoped_refptr PendingTaskSafetyFlag::CreateDetached() { rtc::scoped_refptr safety_flag( - new rtc::RefCountedObject(true)); + new PendingTaskSafetyFlag(true)); safety_flag->main_sequence_.Detach(); return safety_flag; } @@ -30,7 +28,7 @@ PendingTaskSafetyFlag::CreateDetached() { rtc::scoped_refptr PendingTaskSafetyFlag::CreateDetachedInactive() { rtc::scoped_refptr safety_flag( - new rtc::RefCountedObject(false)); + new PendingTaskSafetyFlag(false)); safety_flag->main_sequence_.Detach(); return safety_flag; } diff --git a/rtc_base/task_utils/pending_task_safety_flag.h b/rtc_base/task_utils/pending_task_safety_flag.h index 4864b5de3b..fc1b5bd878 100644 --- a/rtc_base/task_utils/pending_task_safety_flag.h +++ b/rtc_base/task_utils/pending_task_safety_flag.h @@ -11,10 +11,10 @@ #ifndef RTC_BASE_TASK_UTILS_PENDING_TASK_SAFETY_FLAG_H_ #define RTC_BASE_TASK_UTILS_PENDING_TASK_SAFETY_FLAG_H_ +#include "api/ref_counted_base.h" #include "api/scoped_refptr.h" #include "api/sequence_checker.h" #include "rtc_base/checks.h" -#include "rtc_base/ref_count.h" #include "rtc_base/system/no_unique_address.h" namespace webrtc { @@ -55,7 +55,8 @@ namespace webrtc { // my_task_queue_->PostTask(ToQueuedTask(pending_task_safety_flag_, // [this]() { MyMethod(); })); // -class PendingTaskSafetyFlag : public rtc::RefCountInterface { +class PendingTaskSafetyFlag final + : public rtc::RefCountedNonVirtual { public: static rtc::scoped_refptr Create(); @@ -113,7 +114,7 @@ class PendingTaskSafetyFlag : public rtc::RefCountInterface { // This should be used by the class that wants tasks dropped after destruction. // The requirement is that the instance has to be constructed and destructed on // the same thread as the potentially dropped tasks would be running on. -class ScopedTaskSafety { +class ScopedTaskSafety final { public: ScopedTaskSafety() = default; ~ScopedTaskSafety() { flag_->SetNotAlive(); } @@ -128,7 +129,7 @@ class ScopedTaskSafety { // Like ScopedTaskSafety, but allows construction on a different thread than // where the flag will be used. -class ScopedTaskSafetyDetached { +class ScopedTaskSafetyDetached final { public: ScopedTaskSafetyDetached() = default; ~ScopedTaskSafetyDetached() { flag_->SetNotAlive(); } diff --git a/sdk/android/src/jni/audio_device/opensles_common.cc b/sdk/android/src/jni/audio_device/opensles_common.cc index 04c3ae9f7a..0f35b2712a 100644 --- a/sdk/android/src/jni/audio_device/opensles_common.cc +++ b/sdk/android/src/jni/audio_device/opensles_common.cc @@ -106,8 +106,6 @@ OpenSLEngineManager::OpenSLEngineManager() { thread_checker_.Detach(); } -OpenSLEngineManager::~OpenSLEngineManager() = default; - SLObjectItf OpenSLEngineManager::GetOpenSLEngine() { RTC_LOG(INFO) << "GetOpenSLEngine"; RTC_DCHECK(thread_checker_.IsCurrent()); diff --git a/sdk/android/src/jni/audio_device/opensles_common.h b/sdk/android/src/jni/audio_device/opensles_common.h index d812b920ff..9dd1e0f7d7 100644 --- a/sdk/android/src/jni/audio_device/opensles_common.h +++ b/sdk/android/src/jni/audio_device/opensles_common.h @@ -68,10 +68,11 @@ typedef ScopedSLObject ScopedSLObjectItf; // Subsequent calls returns the already created engine. // Note: This class must be used single threaded and this is enforced by a // thread checker. -class OpenSLEngineManager : public rtc::RefCountedBase { +class OpenSLEngineManager + : public rtc::RefCountedNonVirtual { public: OpenSLEngineManager(); - ~OpenSLEngineManager() override; + ~OpenSLEngineManager() = default; SLObjectItf GetOpenSLEngine(); private: diff --git a/sdk/android/src/jni/pc/add_ice_candidate_observer.h b/sdk/android/src/jni/pc/add_ice_candidate_observer.h index ed72de9df6..1128385389 100644 --- a/sdk/android/src/jni/pc/add_ice_candidate_observer.h +++ b/sdk/android/src/jni/pc/add_ice_candidate_observer.h @@ -20,10 +20,11 @@ namespace webrtc { namespace jni { -class AddIceCandidateObserverJni final : public rtc::RefCountedBase { +class AddIceCandidateObserverJni final + : public rtc::RefCountedNonVirtual { public: AddIceCandidateObserverJni(JNIEnv* env, const JavaRef& j_observer); - ~AddIceCandidateObserverJni() override = default; + ~AddIceCandidateObserverJni() = default; void OnComplete(RTCError error); diff --git a/stats/rtc_stats_report.cc b/stats/rtc_stats_report.cc index d29d819fc3..4fbd82508e 100644 --- a/stats/rtc_stats_report.cc +++ b/stats/rtc_stats_report.cc @@ -56,15 +56,12 @@ bool RTCStatsReport::ConstIterator::operator!=( rtc::scoped_refptr RTCStatsReport::Create( int64_t timestamp_us) { - return rtc::scoped_refptr( - new rtc::RefCountedObject(timestamp_us)); + return rtc::scoped_refptr(new RTCStatsReport(timestamp_us)); } RTCStatsReport::RTCStatsReport(int64_t timestamp_us) : timestamp_us_(timestamp_us) {} -RTCStatsReport::~RTCStatsReport() {} - rtc::scoped_refptr RTCStatsReport::Copy() const { rtc::scoped_refptr copy = Create(timestamp_us_); for (auto it = stats_.begin(); it != stats_.end(); ++it) {