From 8d2c5a810fec167c6ffe0bb5132e9e259ef558d4 Mon Sep 17 00:00:00 2001 From: Tommi Date: Mon, 19 Mar 2018 11:12:48 +0100 Subject: [PATCH] Detangling target dependencies in rtc_base_approved. The eventual goal is to allow PlatformThread to use SequencedTaskChecker, but getting to that point will require some more detangling. Here are (roughly) the steps taken in this CL: * Make constructormagic a separate target. * Move atomicops and arraysize to separate targets * Move platform_thread_types to a separate target * Move criticalsection to a separate target * Move thread_checker to separate target * Make sequenced_task_checker not depend on base_approved * Move ptr_util to a separate target * Move scoped_ptr to ptr_util * Make rtc_task_queue_api not depend on base_approved * Make sequenced_task_checker depend on rtc_task_queue_api * Move rtc::Event to its own target * Move basictypes.h to constructormagic * Move format_macros and stringize_macros into constructormagic * Rename constructormagic target to... macromagic * Move stringencode to stringutils * New target for safe_conversions * Move timeutils to a new target. * Move logging to a new target. * Move platform_thread to a new target. * Make refcount a new target (refcount, refcountedobject, refcounter). * Remove rtc_base_approved from deps of TQ * Remove a circular dependency between event tracer and platform thread. Further steps will probably be to factor TaskQueue::Current() to not be a part of the TaskQueue class itself and have it declared+implemented in a target that's lower level than TQ itself. SequencedTaskChecker can then depend on that target and avoid the TQ dependency. Once we're there, PlatformThread will be able to depend on SequencedTaskChecker. Attempted but eventually removed from this CL: * Make TQ a part of rtc_base_approved * Remove direct dependencies on sequenced_task_checker. * Profit. A few include-what-you-use updates along the way. Fix a few targets that were depending on rtc_task_queue_api Change-Id: Iee79aa2e81d978444c51b3005db9df7dc12d92a9 Bug: webrtc:8957 Reviewed-on: https://webrtc-review.googlesource.com/58480 Commit-Queue: Tommi Reviewed-by: Karl Wiberg Reviewed-by: Mirko Bonadei Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#22487} --- logging/BUILD.gn | 2 +- media/BUILD.gn | 7 +- pc/BUILD.gn | 4 +- rtc_base/BUILD.gn | 357 +++++++++++++++++++----- rtc_base/criticalsection.cc | 1 + rtc_base/criticalsection.h | 1 - rtc_base/criticalsection_unittest.cc | 1 + rtc_base/event_tracer.cc | 1 + rtc_base/format_macros.h | 2 - rtc_base/platform_thread.cc | 3 - rtc_base/sequenced_task_checker_impl.cc | 1 - rtc_base/sequenced_task_checker_impl.h | 1 + rtc_base/thread_checker_impl.cc | 2 - 13 files changed, 292 insertions(+), 91 deletions(-) diff --git a/logging/BUILD.gn b/logging/BUILD.gn index 7b83c378cd..afee1946cd 100644 --- a/logging/BUILD.gn +++ b/logging/BUILD.gn @@ -219,7 +219,7 @@ rtc_static_library("rtc_event_log_impl_base") { ":rtc_event_log_impl_output", "../rtc_base:checks", "../rtc_base:rtc_base_approved", - "../rtc_base:rtc_task_queue_api", + "../rtc_base:rtc_task_queue", "../rtc_base:safe_minmax", "../rtc_base:sequenced_task_checker", ] diff --git a/media/BUILD.gn b/media/BUILD.gn index a9f8df8e37..90478a06be 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -69,7 +69,8 @@ rtc_static_library("rtc_media_base") { deps = [ "../api:audio_options_api", "../rtc_base:checks", - "../rtc_base:rtc_task_queue_api", + "../rtc_base:rtc_base_approved", + "../rtc_base:rtc_task_queue", "../rtc_base:sanitizer", "../rtc_base:sequenced_task_checker", "../rtc_base:stringutils", @@ -450,7 +451,7 @@ if (rtc_include_tests) { "../modules/video_coding:video_coding_utility", "../p2p:rtc_p2p", "../rtc_base:checks", - "../rtc_base:rtc_task_queue_api", + "../rtc_base:rtc_task_queue", "../rtc_base:stringutils", ] sources = [ @@ -552,7 +553,7 @@ if (rtc_include_tests) { "../pc:rtc_pc", "../pc:rtc_pc_base", "../rtc_base:checks", - "../rtc_base:rtc_task_queue_api", + "../rtc_base:rtc_task_queue", "../rtc_base:stringutils", "../test:field_trial", ] diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 592f574eb8..4f69eef7c6 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -382,7 +382,7 @@ if (rtc_include_tests) { "../rtc_base:rtc_base", "../rtc_base:rtc_base_approved", "../rtc_base:rtc_base_tests_utils", - "../rtc_base:rtc_task_queue_api", + "../rtc_base:rtc_task_queue", "../test:test_support", ] @@ -516,7 +516,7 @@ if (rtc_include_tests) { "../rtc_base:rtc_base_approved", "../rtc_base:rtc_base_tests_main", "../rtc_base:rtc_base_tests_utils", - "../rtc_base:rtc_task_queue_api", + "../rtc_base:rtc_task_queue", "../system_wrappers:metrics_default", "../system_wrappers:runtime_enabled_features_default", "../test:audio_codec_mocks", diff --git a/rtc_base/BUILD.gn b/rtc_base/BUILD.gn index 50e0b2499b..fbec98b485 100644 --- a/rtc_base/BUILD.gn +++ b/rtc_base/BUILD.gn @@ -23,7 +23,6 @@ group("base") { ":rtc_base", ":rtc_base_approved", ":rtc_task_queue", - ":sequenced_task_checker", ":weak_ptr", ] if (is_android) { @@ -85,13 +84,169 @@ source_set("compile_assert_c") { rtc_source_set("rtc_base_approved") { visibility = [ "*" ] public_deps = [ + ":atomicops", + ":criticalsection", + ":logging", + ":macromagic", + ":platform_thread", + ":platform_thread_types", + ":ptr_util", + ":refcount", ":rtc_base_approved_generic", + ":rtc_event", + ":safe_conversions", + ":stringutils", + ":thread_checker", + ":timeutils", ] if (is_mac && !build_with_chromium) { public_deps += [ ":rtc_base_approved_objc" ] } } +rtc_source_set("macromagic") { + sources = [ + "arraysize.h", + "basictypes.h", + "constructormagic.h", + "format_macros.h", + "stringize_macros.h", + "thread_annotations.h", + ] +} + +rtc_source_set("platform_thread_types") { + sources = [ + "platform_thread_types.cc", + "platform_thread_types.h", + ] +} + +rtc_source_set("ptr_util") { + sources = [ + "ptr_util.h", + "scoped_ref_ptr.h", + ] +} + +rtc_source_set("refcount") { + sources = [ + "refcount.h", + "refcountedobject.h", + "refcounter.h", + ] + deps = [ + ":atomicops", + ":macromagic", + ] +} + +rtc_source_set("criticalsection") { + sources = [ + "criticalsection.cc", + "criticalsection.h", + ] + deps = [ + ":atomicops", + ":checks", + ":macromagic", + ":platform_thread_types", + "..:typedefs", + ] +} + +rtc_source_set("platform_thread") { + visibility = [ + ":rtc_base_approved", + ":rtc_base_approved_generic", + ":rtc_task_queue_libevent", + ":rtc_task_queue_win", + ":sequenced_task_checker", + ] + sources = [ + "platform_thread.cc", + "platform_thread.h", + ] + deps = [ + ":atomicops", + ":checks", + ":macromagic", + ":platform_thread_types", + ":rtc_event", + ":thread_checker", + ":timeutils", + "..:typedefs", + ] +} + +rtc_source_set("rtc_event") { + if (build_with_chromium) { + sources = [ + "../../webrtc_overrides/rtc_base/event.cc", + "../../webrtc_overrides/rtc_base/event.h", + ] + } else { + sources = [ + "event.cc", + "event.h", + ] + } + + deps = [ + ":checks", + ":macromagic", + ] +} + +rtc_source_set("logging") { + deps = [ + ":criticalsection", + ":macromagic", + ":platform_thread_types", + ":stringutils", + ":timeutils", + ] + + if (build_with_chromium) { + # Dependency on chromium's logging (in //base). + deps += [ "//base:base" ] + sources = [ + "../../webrtc_overrides/rtc_base/logging.cc", + "../../webrtc_overrides/rtc_base/logging.h", + ] + } else { + sources = [ + "logging.cc", + "logging.h", + ] + + # logging.h needs the deprecation header while downstream projects are + # removing code that depends on logging implementation details. + deps += [ ":deprecation" ] + } +} + +rtc_source_set("thread_checker") { + sources = [ + "thread_checker.h", + "thread_checker_impl.cc", + "thread_checker_impl.h", + ] + deps = [ + ":checks", + ":criticalsection", + ":macromagic", + ":platform_thread_types", + "..:typedefs", + ] +} + +rtc_source_set("atomicops") { + sources = [ + "atomicops.h", + ] +} + rtc_source_set("checks") { sources = [ "checks.cc", @@ -140,8 +295,31 @@ rtc_source_set("safe_minmax") { ] } +rtc_source_set("safe_conversions") { + sources = [ + "numerics/safe_conversions.h", + "numerics/safe_conversions_impl.h", + ] + deps = [ + ":checks", + ] +} + +rtc_source_set("timeutils") { + sources = [ + "timeutils.cc", + "timeutils.h", + ] + deps = [ + ":checks", + ":safe_conversions", + ] +} + rtc_source_set("stringutils") { sources = [ + "stringencode.cc", + "stringencode.h", "strings/string_builder.cc", "strings/string_builder.h", "stringutils.cc", @@ -170,7 +348,6 @@ rtc_source_set("deprecation") { rtc_source_set("rtc_base_approved_generic") { visibility = [ ":rtc_base_approved", - ":rtc_base_approved_objc", ":weak_ptr_unittests", ] @@ -178,19 +355,29 @@ rtc_source_set("rtc_base_approved_generic") { defines = [] libs = [] deps = [ + ":atomicops", ":checks", + ":criticalsection", + ":logging", + ":macromagic", + ":platform_thread", + ":platform_thread_types", + ":ptr_util", + ":refcount", + ":rtc_event", + ":rtc_task_queue", ":safe_compare", + ":safe_conversions", ":stringutils", + ":thread_checker", + ":timeutils", ":type_traits", "../:typedefs", ] sources = [ - "arraysize.h", - "atomicops.h", "base64.cc", "base64.h", - "basictypes.h", "bind.h", "bitbuffer.cc", "bitbuffer.h", @@ -202,18 +389,14 @@ rtc_source_set("rtc_base_approved_generic") { "bytebuffer.cc", "bytebuffer.h", "byteorder.h", - "constructormagic.h", "copyonwritebuffer.cc", "copyonwritebuffer.h", - "criticalsection.cc", - "criticalsection.h", "event_tracer.cc", "event_tracer.h", "file.cc", "file.h", "flags.cc", "flags.h", - "format_macros.h", "function_view.h", "ignore_wundef.h", "location.cc", @@ -222,18 +405,11 @@ rtc_source_set("rtc_base_approved_generic") { "numerics/histogram_percentile_counter.h", "numerics/mod_ops.h", "numerics/moving_max_counter.h", - "numerics/safe_conversions.h", - "numerics/safe_conversions_impl.h", "onetimeevent.h", "pathutils.cc", "pathutils.h", "platform_file.cc", "platform_file.h", - "platform_thread.cc", - "platform_thread.h", - "platform_thread_types.cc", - "platform_thread_types.h", - "ptr_util.h", "race_checker.cc", "race_checker.h", "random.cc", @@ -242,25 +418,12 @@ rtc_source_set("rtc_base_approved_generic") { "rate_statistics.h", "ratetracker.cc", "ratetracker.h", - "refcount.h", - "refcountedobject.h", - "refcounter.h", - "scoped_ref_ptr.h", "string_to_number.cc", "string_to_number.h", - "stringencode.cc", - "stringencode.h", - "stringize_macros.h", "swap_queue.h", "template_util.h", - "thread_annotations.h", - "thread_checker.h", - "thread_checker_impl.cc", - "thread_checker_impl.h", "timestampaligner.cc", "timestampaligner.h", - "timeutils.cc", - "timeutils.h", "trace_event.h", "zero_memory.cc", "zero_memory.h", @@ -284,27 +447,6 @@ rtc_source_set("rtc_base_approved_generic") { sources += [ "file_win.cc" ] } - if (build_with_chromium) { - # Dependency on chromium's logging (in //base). - deps += [ "//base:base" ] - sources += [ - "../../webrtc_overrides/rtc_base/event.cc", - "../../webrtc_overrides/rtc_base/event.h", - "../../webrtc_overrides/rtc_base/logging.cc", - "../../webrtc_overrides/rtc_base/logging.h", - ] - } else { - sources += [ - "event.cc", - "event.h", - "logging.cc", - "logging.h", - ] - - # logging.h needs the deprecation header while downstream projects are - # removing code that depends on logging implementation details. - deps += [ ":deprecation" ] - } if (is_component_build && is_win) { # Copy the VS runtime DLLs into the isolate so that they don't have to be # preinstalled on the target machine. The debug runtimes have a "d" at @@ -395,16 +537,14 @@ if (is_mac && !build_with_chromium) { "logging_mac.mm", ] deps = [ - ":rtc_base_approved_generic", + ":logging", ] } } rtc_source_set("rtc_task_queue") { visibility = [ "*" ] - deps = [ - ":rtc_base_approved", - ] + deps = [] public_deps = [ ":rtc_task_queue_api", ] @@ -421,47 +561,109 @@ rtc_source_set("rtc_task_queue") { # provided implementation should be used. An external implementation should # depend on rtc_task_queue_api. rtc_source_set("rtc_task_queue_api") { + # The visibility list is commented out so that we won't break external + # implementations, but left here to manually test as well as for sake of what + # targets we expect to depend on rtc_task_queue_api. + # visibility = [ + # ":rtc_task_queue", + # ":rtc_task_queue_impl", + # ":sequenced_task_checker", + # ] sources = [ "task_queue.h", ] deps = [ - ":rtc_base_approved", + ":macromagic", + ":ptr_util", ] } -rtc_source_set("rtc_task_queue_impl") { - visibility = [ "*" ] - deps = [ - ":checks", - ":rtc_base_approved", - ":rtc_task_queue_api", - ] - if (rtc_build_libevent) { - deps += [ "//base/third_party/libevent" ] - } - if (rtc_enable_libevent) { +if (rtc_enable_libevent) { + rtc_source_set("rtc_task_queue_libevent") { + visibility = [ ":rtc_task_queue_impl" ] sources = [ "task_queue_libevent.cc", "task_queue_posix.cc", "task_queue_posix.h", ] + deps = [ + ":checks", + ":criticalsection", + ":logging", + ":platform_thread", + ":ptr_util", + ":refcount", + ":rtc_task_queue_api", + ":safe_conversions", + ":timeutils", + ] + if (rtc_build_libevent) { + deps += [ "//base/third_party/libevent" ] + } + } +} + +if (is_mac || is_ios) { + rtc_source_set("rtc_task_queue_gcd") { + visibility = [ ":rtc_task_queue_impl" ] + sources = [ + "task_queue_gcd.cc", + "task_queue_posix.cc", + "task_queue_posix.h", + ] + deps = [ + ":checks", + ":logging", + ":ptr_util", + ":refcount", + ":rtc_task_queue_api", + ] + } +} + +if (is_win) { + rtc_source_set("rtc_task_queue_win") { + visibility = [ ":rtc_task_queue_impl" ] + sources = [ + "task_queue_win.cc", + ] + deps = [ + ":checks", + ":criticalsection", + ":logging", + ":macromagic", + ":platform_thread", + ":ptr_util", + ":refcount", + ":rtc_event", + ":rtc_task_queue_api", + ":safe_conversions", + ":timeutils", + ] + } +} + +rtc_source_set("rtc_task_queue_impl") { + visibility = [ "*" ] + if (rtc_enable_libevent) { + deps = [ + ":rtc_task_queue_libevent", + ] } else { if (is_mac || is_ios) { - sources = [ - "task_queue_gcd.cc", - "task_queue_posix.cc", - "task_queue_posix.h", + deps = [ + ":rtc_task_queue_gcd", ] } if (is_win) { - sources = [ - "task_queue_win.cc", + deps = [ + ":rtc_task_queue_win", ] } } } -rtc_static_library("sequenced_task_checker") { +rtc_source_set("sequenced_task_checker") { sources = [ "sequenced_task_checker.h", "sequenced_task_checker_impl.cc", @@ -469,8 +671,10 @@ rtc_static_library("sequenced_task_checker") { ] deps = [ ":checks", - ":rtc_base_approved", + ":criticalsection", + ":macromagic", ":rtc_task_queue", + ":thread_checker", ] } @@ -480,7 +684,8 @@ rtc_static_library("weak_ptr") { "weak_ptr.h", ] deps = [ - ":rtc_base_approved", + ":ptr_util", + ":refcount", ":sequenced_task_checker", ] } @@ -869,7 +1074,6 @@ rtc_source_set("rtc_base_tests_utils") { "nattypes.h", "proxyserver.cc", "proxyserver.h", - "refcount.h", "sigslottester.h", "sigslottester.h.pump", "testbase64.h", @@ -1070,6 +1274,7 @@ if (rtc_include_tests) { ":rtc_base_approved_generic", ":rtc_base_tests_main", ":rtc_base_tests_utils", + ":rtc_event", ":rtc_task_queue", ":weak_ptr", "../test:test_support", diff --git a/rtc_base/criticalsection.cc b/rtc_base/criticalsection.cc index f9168d89e8..b2bbd03e02 100644 --- a/rtc_base/criticalsection.cc +++ b/rtc_base/criticalsection.cc @@ -10,6 +10,7 @@ #include "rtc_base/criticalsection.h" +#include "rtc_base/atomicops.h" #include "rtc_base/checks.h" #include "rtc_base/platform_thread_types.h" diff --git a/rtc_base/criticalsection.h b/rtc_base/criticalsection.h index 1ef9634efe..569e1479bd 100644 --- a/rtc_base/criticalsection.h +++ b/rtc_base/criticalsection.h @@ -11,7 +11,6 @@ #ifndef RTC_BASE_CRITICALSECTION_H_ #define RTC_BASE_CRITICALSECTION_H_ -#include "rtc_base/atomicops.h" #include "rtc_base/checks.h" #include "rtc_base/constructormagic.h" #include "rtc_base/platform_thread_types.h" diff --git a/rtc_base/criticalsection_unittest.cc b/rtc_base/criticalsection_unittest.cc index f5d6957fcf..6792504353 100644 --- a/rtc_base/criticalsection_unittest.cc +++ b/rtc_base/criticalsection_unittest.cc @@ -13,6 +13,7 @@ #include #include "rtc_base/arraysize.h" +#include "rtc_base/atomicops.h" #include "rtc_base/checks.h" #include "rtc_base/criticalsection.h" #include "rtc_base/event.h" diff --git a/rtc_base/event_tracer.cc b/rtc_base/event_tracer.cc index 9cbda972e3..3461ec625a 100644 --- a/rtc_base/event_tracer.cc +++ b/rtc_base/event_tracer.cc @@ -14,6 +14,7 @@ #include #include +#include "rtc_base/atomicops.h" #include "rtc_base/checks.h" #include "rtc_base/criticalsection.h" #include "rtc_base/event.h" diff --git a/rtc_base/format_macros.h b/rtc_base/format_macros.h index d252a94cca..48127e16e0 100644 --- a/rtc_base/format_macros.h +++ b/rtc_base/format_macros.h @@ -24,8 +24,6 @@ // printf("xyz: %" PRIuS, size); // The "u" in the macro corresponds to %u, and S is for "size". -#include "typedefs.h" // NOLINT(build/include) - #if defined(WEBRTC_POSIX) #if (defined(_INTTYPES_H) || defined(_INTTYPES_H_)) && !defined(PRId64) diff --git a/rtc_base/platform_thread.cc b/rtc_base/platform_thread.cc index ca2ce1367b..79d9d53aee 100644 --- a/rtc_base/platform_thread.cc +++ b/rtc_base/platform_thread.cc @@ -13,7 +13,6 @@ #include "rtc_base/atomicops.h" #include "rtc_base/checks.h" #include "rtc_base/timeutils.h" -#include "rtc_base/trace_event.h" #if defined(WEBRTC_LINUX) #include @@ -178,8 +177,6 @@ void PlatformThread::Run() { #endif do { - TRACE_EVENT1("webrtc", "PlatformThread::Run", "name", name_.c_str()); - // The interface contract of Start/Stop is that for a successful call to // Start, there should be at least one call to the run function. So we // call the function before checking |stop_|. diff --git a/rtc_base/sequenced_task_checker_impl.cc b/rtc_base/sequenced_task_checker_impl.cc index d7f46ead7d..16069c25f7 100644 --- a/rtc_base/sequenced_task_checker_impl.cc +++ b/rtc_base/sequenced_task_checker_impl.cc @@ -14,7 +14,6 @@ #include #endif -#include "rtc_base/platform_thread.h" #include "rtc_base/sequenced_task_checker.h" #include "rtc_base/task_queue.h" diff --git a/rtc_base/sequenced_task_checker_impl.h b/rtc_base/sequenced_task_checker_impl.h index 86d5ef0213..293b1ac297 100644 --- a/rtc_base/sequenced_task_checker_impl.h +++ b/rtc_base/sequenced_task_checker_impl.h @@ -11,6 +11,7 @@ #ifndef RTC_BASE_SEQUENCED_TASK_CHECKER_IMPL_H_ #define RTC_BASE_SEQUENCED_TASK_CHECKER_IMPL_H_ +#include "rtc_base/criticalsection.h" #include "rtc_base/thread_checker.h" namespace rtc { diff --git a/rtc_base/thread_checker_impl.cc b/rtc_base/thread_checker_impl.cc index 6ec5c91d2e..850f2ac3cf 100644 --- a/rtc_base/thread_checker_impl.cc +++ b/rtc_base/thread_checker_impl.cc @@ -12,8 +12,6 @@ #include "rtc_base/thread_checker_impl.h" -#include "rtc_base/platform_thread.h" - namespace rtc { ThreadCheckerImpl::ThreadCheckerImpl() : valid_thread_(CurrentThreadRef()) {