From 6fc26dff7cc9f340d05971625a1e3b7fc62327b7 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Thu, 26 Mar 2020 18:19:28 -0700 Subject: [PATCH] [adb] Don't copy features set on each get() The features are already cached in a static and don't change. Let's return the cached set instead of copying it every time. Bug: 150183149 Test: manual Change-Id: Ifdca852cc3b32e09e50ea4771f7878987c46cce8 --- adb/adb.cpp | 4 +-- adb/client/adb_client.cpp | 21 +++++------- adb/client/adb_client.h | 2 +- adb/client/adb_install.cpp | 6 ++-- adb/client/commandline.cpp | 36 +++++++------------- adb/client/file_sync_client.cpp | 12 ++++--- adb/transport.cpp | 59 ++++++++++++++++++--------------- adb/transport.h | 7 ++-- adb/transport_test.cpp | 2 +- 9 files changed, 71 insertions(+), 78 deletions(-) diff --git a/adb/adb.cpp b/adb/adb.cpp index 0518e9d12..08d3904c7 100644 --- a/adb/adb.cpp +++ b/adb/adb.cpp @@ -1198,9 +1198,9 @@ HostRequestResult handle_host_request(std::string_view service, TransportType ty FeatureSet features = supported_features(); // Abuse features to report libusb status. if (should_use_libusb()) { - features.insert(kFeatureLibusb); + features.emplace_back(kFeatureLibusb); } - features.insert(kFeaturePushSync); + features.emplace_back(kFeaturePushSync); SendOkay(reply_fd, FeatureSetToString(features)); return HostRequestResult::Handled; } diff --git a/adb/client/adb_client.cpp b/adb/client/adb_client.cpp index c859d750c..0ad0465eb 100644 --- a/adb/client/adb_client.cpp +++ b/adb/client/adb_client.cpp @@ -37,6 +37,7 @@ #include #include +#include #include #include #include @@ -415,18 +416,14 @@ std::string format_host_command(const char* command) { return android::base::StringPrintf("%s:%s", prefix, command); } -bool adb_get_feature_set(FeatureSet* feature_set, std::string* error) { - static FeatureSet* features = nullptr; - if (!features) { +const FeatureSet& adb_get_feature_set() { + static const android::base::NoDestructor features([] { std::string result; - if (adb_query(format_host_command("features"), &result, error)) { - features = new FeatureSet(StringToFeatureSet(result)); + if (!adb_query(format_host_command("features"), &result, &result)) { + fprintf(stderr, "failed to get feature set: %s\n", result.c_str()); + return FeatureSet{}; } - } - if (features) { - *feature_set = *features; - return true; - } - feature_set->clear(); - return false; + return StringToFeatureSet(result); + }()); + return *features; } diff --git a/adb/client/adb_client.h b/adb/client/adb_client.h index 1c6cde77d..bf0be40b3 100644 --- a/adb/client/adb_client.h +++ b/adb/client/adb_client.h @@ -76,7 +76,7 @@ bool adb_status(borrowed_fd fd, std::string* _Nonnull error); std::string format_host_command(const char* _Nonnull command); // Get the feature set of the current preferred transport. -bool adb_get_feature_set(FeatureSet* _Nonnull feature_set, std::string* _Nonnull error); +const FeatureSet& adb_get_feature_set(); #if defined(__linux__) // Get the path of a file containing the path to the server executable, if the socket spec set via diff --git a/adb/client/adb_install.cpp b/adb/client/adb_install.cpp index da3154e4a..3810cc89f 100644 --- a/adb/client/adb_install.cpp +++ b/adb/client/adb_install.cpp @@ -57,10 +57,8 @@ enum class CmdlineOption { None, Enable, Disable }; } static bool can_use_feature(const char* feature) { - FeatureSet features; - std::string error; - if (!adb_get_feature_set(&features, &error)) { - fprintf(stderr, "error: %s\n", error.c_str()); + auto&& features = adb_get_feature_set(); + if (features.empty()) { return false; } return CanUseFeature(features, feature); diff --git a/adb/client/commandline.cpp b/adb/client/commandline.cpp index ceb21d595..6a7493f08 100644 --- a/adb/client/commandline.cpp +++ b/adb/client/commandline.cpp @@ -672,10 +672,8 @@ static int RemoteShell(bool use_shell_protocol, const std::string& type_arg, cha } static int adb_shell(int argc, const char** argv) { - FeatureSet features; - std::string error_message; - if (!adb_get_feature_set(&features, &error_message)) { - fprintf(stderr, "error: %s\n", error_message.c_str()); + auto&& features = adb_get_feature_set(); + if (features.empty()) { return 1; } @@ -779,13 +777,10 @@ static int adb_shell(int argc, const char** argv) { } static int adb_abb(int argc, const char** argv) { - FeatureSet features; - std::string error_message; - if (!adb_get_feature_set(&features, &error_message)) { - fprintf(stderr, "error: %s\n", error_message.c_str()); + auto&& features = adb_get_feature_set(); + if (features.empty()) { return 1; } - if (!CanUseFeature(features, kFeatureAbb)) { error_exit("abb is not supported by the device"); } @@ -1164,9 +1159,8 @@ int send_shell_command(const std::string& command, bool disable_shell_protocol, // Use shell protocol if it's supported and the caller doesn't explicitly // disable it. if (!disable_shell_protocol) { - FeatureSet features; - std::string error; - if (adb_get_feature_set(&features, &error)) { + auto&& features = adb_get_feature_set(); + if (!features.empty()) { use_shell_protocol = CanUseFeature(features, kFeatureShell2); } else { // Device was unreachable. @@ -1816,10 +1810,8 @@ int adb_commandline(int argc, const char** argv) { } return adb_connect_command(android::base::StringPrintf("tcpip:%d", port)); } else if (!strcmp(argv[0], "remount")) { - FeatureSet features; - std::string error; - if (!adb_get_feature_set(&features, &error)) { - fprintf(stderr, "error: %s\n", error.c_str()); + auto&& features = adb_get_feature_set(); + if (features.empty()) { return 1; } @@ -2042,10 +2034,8 @@ int adb_commandline(int argc, const char** argv) { } else if (!strcmp(argv[0], "track-jdwp")) { return adb_connect_command("track-jdwp"); } else if (!strcmp(argv[0], "track-app")) { - FeatureSet features; - std::string error; - if (!adb_get_feature_set(&features, &error)) { - fprintf(stderr, "error: %s\n", error.c_str()); + auto&& features = adb_get_feature_set(); + if (features.empty()) { return 1; } if (!CanUseFeature(features, kFeatureTrackApp)) { @@ -2074,10 +2064,8 @@ int adb_commandline(int argc, const char** argv) { return 0; } else if (!strcmp(argv[0], "features")) { // Only list the features common to both the adb client and the device. - FeatureSet features; - std::string error; - if (!adb_get_feature_set(&features, &error)) { - fprintf(stderr, "error: %s\n", error.c_str()); + auto&& features = adb_get_feature_set(); + if (features.empty()) { return 1; } diff --git a/adb/client/file_sync_client.cpp b/adb/client/file_sync_client.cpp index 681673449..f2c673a88 100644 --- a/adb/client/file_sync_client.cpp +++ b/adb/client/file_sync_client.cpp @@ -225,13 +225,14 @@ struct TransferLedger { class SyncConnection { public: - SyncConnection() : acknowledgement_buffer_(sizeof(sync_status) + SYNC_DATA_MAX) { + SyncConnection() + : acknowledgement_buffer_(sizeof(sync_status) + SYNC_DATA_MAX), + features_(adb_get_feature_set()) { acknowledgement_buffer_.resize(0); max = SYNC_DATA_MAX; // TODO: decide at runtime. - std::string error; - if (!adb_get_feature_set(&features_, &error)) { - Error("failed to get feature set: %s", error.c_str()); + if (features_.empty()) { + Error("failed to get feature set"); } else { have_stat_v2_ = CanUseFeature(features_, kFeatureStat2); have_ls_v2_ = CanUseFeature(features_, kFeatureLs2); @@ -239,6 +240,7 @@ class SyncConnection { have_sendrecv_v2_brotli_ = CanUseFeature(features_, kFeatureSendRecv2Brotli); have_sendrecv_v2_lz4_ = CanUseFeature(features_, kFeatureSendRecv2LZ4); have_sendrecv_v2_dry_run_send_ = CanUseFeature(features_, kFeatureSendRecv2DryRunSend); + std::string error; fd.reset(adb_connect("sync:", &error)); if (fd < 0) { Error("connect failed: %s", error.c_str()); @@ -919,7 +921,7 @@ class SyncConnection { private: std::deque> deferred_acknowledgements_; Block acknowledgement_buffer_; - FeatureSet features_; + const FeatureSet& features_; bool have_stat_v2_; bool have_ls_v2_; bool have_sendrecv_v2_; diff --git a/adb/transport.cpp b/adb/transport.cpp index cc2e0e3b1..963c3c194 100644 --- a/adb/transport.cpp +++ b/adb/transport.cpp @@ -29,7 +29,6 @@ #include #include -#include #include #include #include @@ -40,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -1170,28 +1170,29 @@ size_t atransport::get_max_payload() const { } const FeatureSet& supported_features() { - // Local static allocation to avoid global non-POD variables. - static const FeatureSet* features = new FeatureSet{ - kFeatureShell2, - kFeatureCmd, - kFeatureStat2, - kFeatureLs2, - kFeatureFixedPushMkdir, - kFeatureApex, - kFeatureAbb, - kFeatureFixedPushSymlinkTimestamp, - kFeatureAbbExec, - kFeatureRemountShell, - kFeatureTrackApp, - kFeatureSendRecv2, - kFeatureSendRecv2Brotli, - kFeatureSendRecv2LZ4, - kFeatureSendRecv2DryRunSend, - // Increment ADB_SERVER_VERSION when adding a feature that adbd needs - // to know about. Otherwise, the client can be stuck running an old - // version of the server even after upgrading their copy of adb. - // (http://b/24370690) - }; + static const android::base::NoDestructor features([] { + return FeatureSet{ + kFeatureShell2, + kFeatureCmd, + kFeatureStat2, + kFeatureLs2, + kFeatureFixedPushMkdir, + kFeatureApex, + kFeatureAbb, + kFeatureFixedPushSymlinkTimestamp, + kFeatureAbbExec, + kFeatureRemountShell, + kFeatureTrackApp, + kFeatureSendRecv2, + kFeatureSendRecv2Brotli, + kFeatureSendRecv2LZ4, + kFeatureSendRecv2DryRunSend, + // Increment ADB_SERVER_VERSION when adding a feature that adbd needs + // to know about. Otherwise, the client can be stuck running an old + // version of the server even after upgrading their copy of adb. + // (http://b/24370690) + }; + }()); return *features; } @@ -1205,16 +1206,20 @@ FeatureSet StringToFeatureSet(const std::string& features_string) { return FeatureSet(); } - auto names = android::base::Split(features_string, ","); - return FeatureSet(names.begin(), names.end()); + return android::base::Split(features_string, ","); +} + +template +static bool contains(const Range& r, const Value& v) { + return std::find(std::begin(r), std::end(r), v) != std::end(r); } bool CanUseFeature(const FeatureSet& feature_set, const std::string& feature) { - return feature_set.count(feature) > 0 && supported_features().count(feature) > 0; + return contains(feature_set, feature) && contains(supported_features(), feature); } bool atransport::has_feature(const std::string& feature) const { - return features_.count(feature) > 0; + return contains(features_, feature); } void atransport::SetFeatures(const std::string& features_string) { diff --git a/adb/transport.h b/adb/transport.h index 4b2e00043..e93c31cbc 100644 --- a/adb/transport.h +++ b/adb/transport.h @@ -30,7 +30,7 @@ #include #include #include -#include +#include #include #include @@ -40,7 +40,10 @@ #include "adb_unique_fd.h" #include "types.h" -typedef std::unordered_set FeatureSet; +// Even though the feature set is used as a set, we only have a dozen or two +// of available features at any moment. Vector works much better in terms of +// both memory usage and performance for these sizes. +using FeatureSet = std::vector; namespace adb { namespace tls { diff --git a/adb/transport_test.cpp b/adb/transport_test.cpp index 00beb3a2b..a9ada4abf 100644 --- a/adb/transport_test.cpp +++ b/adb/transport_test.cpp @@ -66,7 +66,7 @@ TEST_F(TransportTest, SetFeatures) { ASSERT_TRUE(t.has_feature("bar")); t.SetFeatures(FeatureSetToString(FeatureSet{"foo", "bar", "foo"})); - ASSERT_EQ(2U, t.features().size()); + ASSERT_LE(2U, t.features().size()); ASSERT_TRUE(t.has_feature("foo")); ASSERT_TRUE(t.has_feature("bar"));