[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
diff --git a/adb/adb.cpp b/adb/adb.cpp
index 0518e9d..08d3904 100644
--- a/adb/adb.cpp
+++ b/adb/adb.cpp
@@ -1198,9 +1198,9 @@
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 c859d75..0ad0465 100644
--- a/adb/client/adb_client.cpp
+++ b/adb/client/adb_client.cpp
@@ -37,6 +37,7 @@
#include <vector>
#include <android-base/file.h>
+#include <android-base/no_destructor.h>
#include <android-base/stringprintf.h>
#include <android-base/strings.h>
#include <android-base/thread_annotations.h>
@@ -415,18 +416,14 @@
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<FeatureSet> 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 1c6cde7..bf0be40 100644
--- a/adb/client/adb_client.h
+++ b/adb/client/adb_client.h
@@ -76,7 +76,7 @@
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 da3154e..3810cc8 100644
--- a/adb/client/adb_install.cpp
+++ b/adb/client/adb_install.cpp
@@ -57,10 +57,8 @@
}
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 ceb21d5..6a7493f 100644
--- a/adb/client/commandline.cpp
+++ b/adb/client/commandline.cpp
@@ -672,10 +672,8 @@
}
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_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 @@
// 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 @@
}
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 @@
} 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 @@
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 6816734..f2c673a 100644
--- a/adb/client/file_sync_client.cpp
+++ b/adb/client/file_sync_client.cpp
@@ -225,13 +225,14 @@
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 @@
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 @@
private:
std::deque<std::pair<std::string, std::string>> 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 cc2e0e3..963c3c1 100644
--- a/adb/transport.cpp
+++ b/adb/transport.cpp
@@ -29,7 +29,6 @@
#include <unistd.h>
#include <algorithm>
-#include <deque>
#include <list>
#include <memory>
#include <mutex>
@@ -40,6 +39,7 @@
#include <adb/crypto/x509_generator.h>
#include <adb/tls/tls_connection.h>
#include <android-base/logging.h>
+#include <android-base/no_destructor.h>
#include <android-base/parsenetaddress.h>
#include <android-base/stringprintf.h>
#include <android-base/strings.h>
@@ -1170,28 +1170,29 @@
}
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<FeatureSet> 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 @@
return FeatureSet();
}
- auto names = android::base::Split(features_string, ",");
- return FeatureSet(names.begin(), names.end());
+ return android::base::Split(features_string, ",");
+}
+
+template <class Range, class Value>
+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 4b2e000..e93c31c 100644
--- a/adb/transport.h
+++ b/adb/transport.h
@@ -30,7 +30,7 @@
#include <string>
#include <string_view>
#include <thread>
-#include <unordered_set>
+#include <vector>
#include <android-base/macros.h>
#include <android-base/thread_annotations.h>
@@ -40,7 +40,10 @@
#include "adb_unique_fd.h"
#include "types.h"
-typedef std::unordered_set<std::string> 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<std::string>;
namespace adb {
namespace tls {
diff --git a/adb/transport_test.cpp b/adb/transport_test.cpp
index 00beb3a..a9ada4a 100644
--- a/adb/transport_test.cpp
+++ b/adb/transport_test.cpp
@@ -66,7 +66,7 @@
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"));