Optimize some code path to move instead of copy data.
Test: atest DefaultVehicleHalTest
Bug: 210063973
Change-Id: Ia6a75df7098fae23797571bb59dad3696239ab87
diff --git a/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h b/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h
index 833707a..15a6278 100644
--- a/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h
+++ b/automotive/vehicle/aidl/impl/vhal/include/ConnectedClient.h
@@ -79,7 +79,7 @@
GetSetValuesClient(std::shared_ptr<PendingRequestPool> requestPool, CallbackType callback);
// Sends the results to this client.
- void sendResults(const std::vector<ResultType>& results);
+ void sendResults(std::vector<ResultType>&& results);
// Sends each result separately to this client. Each result would be sent through one callback
// invocation.
diff --git a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
index 62b2627..e3267dd 100644
--- a/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
+++ b/automotive/vehicle/aidl/impl/vhal/include/DefaultVehicleHal.h
@@ -39,13 +39,6 @@
namespace automotive {
namespace vehicle {
-// private namespace
-namespace defaultvehiclehal_impl {
-
-constexpr int INVALID_MEMORY_FD = -1;
-
-} // namespace defaultvehiclehal_impl
-
class DefaultVehicleHal final : public ::aidl::android::hardware::automotive::vehicle::BnVehicle {
public:
using CallbackType =
diff --git a/automotive/vehicle/aidl/impl/vhal/include/ParcelableUtils.h b/automotive/vehicle/aidl/impl/vhal/include/ParcelableUtils.h
index 4b7c2f3..7b2111b 100644
--- a/automotive/vehicle/aidl/impl/vhal/include/ParcelableUtils.h
+++ b/automotive/vehicle/aidl/impl/vhal/include/ParcelableUtils.h
@@ -29,6 +29,9 @@
namespace automotive {
namespace vehicle {
+// Turns the values into a stable large parcelable that could be sent via binder.
+// If values is small enough, it would be put into output.payloads, otherwise a shared memory file
+// would be created and output.sharedMemoryFd would be filled in.
template <class T1, class T2>
::ndk::ScopedAStatus vectorToStableLargeParcelable(std::vector<T1>&& values, T2* output) {
output->payloads = std::move(values);
@@ -44,6 +47,9 @@
// 'sharedMemoryFd' field.
output->payloads.clear();
output->sharedMemoryFd = std::move(*fd);
+ } else {
+ output->sharedMemoryFd = ::ndk::ScopedFileDescriptor();
+ // Do not modify payloads.
}
return ::ndk::ScopedAStatus::ok();
}
diff --git a/automotive/vehicle/aidl/impl/vhal/src/ConnectedClient.cpp b/automotive/vehicle/aidl/impl/vhal/src/ConnectedClient.cpp
index 5ccef55..098bfee 100644
--- a/automotive/vehicle/aidl/impl/vhal/src/ConnectedClient.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/src/ConnectedClient.cpp
@@ -84,9 +84,9 @@
// Send all the GetValue/SetValue results through callback in a single callback invocation.
template <class ResultType, class ResultsType>
void sendGetOrSetValueResults(std::shared_ptr<IVehicleCallback> callback,
- const std::vector<ResultType>& results) {
+ std::vector<ResultType>&& results) {
ResultsType parcelableResults;
- ScopedAStatus status = vectorToStableLargeParcelable(results, &parcelableResults);
+ ScopedAStatus status = vectorToStableLargeParcelable(std::move(results), &parcelableResults);
if (status.isOk()) {
if (ScopedAStatus callbackStatus = callCallback(callback, parcelableResults);
!callbackStatus.isOk()) {
@@ -99,7 +99,8 @@
ALOGE("failed to marshal result into large parcelable, error: "
"%s, code: %d",
status.getMessage(), statusCode);
- sendGetOrSetValueResultsSeparately<ResultType, ResultsType>(callback, results);
+ sendGetOrSetValueResultsSeparately<ResultType, ResultsType>(callback,
+ parcelableResults.payloads);
}
// The timeout callback for GetValues/SetValues.
@@ -115,7 +116,7 @@
.status = StatusCode::TRY_AGAIN,
});
}
- sendGetOrSetValueResults<ResultType, ResultsType>(callback, timeoutResults);
+ sendGetOrSetValueResults<ResultType, ResultsType>(callback, std::move(timeoutResults));
}
// The on-results callback for GetValues/SetValues.
@@ -123,7 +124,7 @@
void getOrSetValuesCallback(
const void* clientId,
std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> callback,
- std::vector<ResultType> results, std::shared_ptr<PendingRequestPool> requestPool) {
+ std::vector<ResultType>&& results, std::shared_ptr<PendingRequestPool> requestPool) {
std::unordered_set<int64_t> requestIds;
for (const auto& result : results) {
requestIds.insert(result.requestId);
@@ -145,7 +146,7 @@
}
if (!results.empty()) {
- sendGetOrSetValueResults<ResultType, ResultsType>(callback, results);
+ sendGetOrSetValueResults<ResultType, ResultsType>(callback, std::move(results));
}
}
@@ -156,9 +157,9 @@
std::shared_ptr<IVehicleCallback> callback, const SetValueResult& result);
template void sendGetOrSetValueResults<GetValueResult, GetValueResults>(
- std::shared_ptr<IVehicleCallback> callback, const std::vector<GetValueResult>& results);
+ std::shared_ptr<IVehicleCallback> callback, std::vector<GetValueResult>&& results);
template void sendGetOrSetValueResults<SetValueResult, SetValueResults>(
- std::shared_ptr<IVehicleCallback> callback, const std::vector<SetValueResult>& results);
+ std::shared_ptr<IVehicleCallback> callback, std::vector<SetValueResult>&& results);
template void sendGetOrSetValueResultsSeparately<GetValueResult, GetValueResults>(
std::shared_ptr<IVehicleCallback> callback, const std::vector<GetValueResult>& results);
@@ -175,11 +176,11 @@
template void getOrSetValuesCallback<GetValueResult, GetValueResults>(
const void* clientId,
std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> callback,
- std::vector<GetValueResult> results, std::shared_ptr<PendingRequestPool> requestPool);
+ std::vector<GetValueResult>&& results, std::shared_ptr<PendingRequestPool> requestPool);
template void getOrSetValuesCallback<SetValueResult, SetValueResults>(
const void* clientId,
std::shared_ptr<::aidl::android::hardware::automotive::vehicle::IVehicleCallback> callback,
- std::vector<SetValueResult> results, std::shared_ptr<PendingRequestPool> requestPool);
+ std::vector<SetValueResult>&& results, std::shared_ptr<PendingRequestPool> requestPool);
} // namespace
@@ -230,9 +231,8 @@
}
template <class ResultType, class ResultsType>
-void GetSetValuesClient<ResultType, ResultsType>::sendResults(
- const std::vector<ResultType>& results) {
- return sendGetOrSetValueResults<ResultType, ResultsType>(mCallback, results);
+void GetSetValuesClient<ResultType, ResultsType>::sendResults(std::vector<ResultType>&& results) {
+ return sendGetOrSetValueResults<ResultType, ResultsType>(mCallback, std::move(results));
}
template <class ResultType, class ResultsType>
@@ -283,7 +283,8 @@
// TODO(b/205189110): Use memory pool here and fill in sharedMemoryId.
VehiclePropValues vehiclePropValues;
int32_t sharedMemoryFileCount = 0;
- ScopedAStatus status = vectorToStableLargeParcelable(updatedValues, &vehiclePropValues);
+ ScopedAStatus status =
+ vectorToStableLargeParcelable(std::move(updatedValues), &vehiclePropValues);
if (!status.isOk()) {
int statusCode = status.getServiceSpecificError();
ALOGE("subscribe: failed to marshal result into large parcelable, error: "
diff --git a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
index 3e088c5..dd88979 100644
--- a/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/src/DefaultVehicleHal.cpp
@@ -387,7 +387,7 @@
if (!failedResults.empty()) {
// First send the failed results we already know back to the client.
- client->sendResults(failedResults);
+ client->sendResults(std::move(failedResults));
}
if (hardwareRequests.empty()) {
@@ -476,7 +476,7 @@
if (!failedResults.empty()) {
// First send the failed results we already know back to the client.
- client->sendResults(failedResults);
+ client->sendResults(std::move(failedResults));
}
if (hardwareRequests.empty()) {
diff --git a/automotive/vehicle/aidl/impl/vhal/test/ConnectedClientTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/ConnectedClientTest.cpp
index bd4a565..bdb0d31 100644
--- a/automotive/vehicle/aidl/impl/vhal/test/ConnectedClientTest.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/test/ConnectedClientTest.cpp
@@ -80,7 +80,8 @@
GetValuesClient client(getPool(), getCallbackClient());
- client.sendResults(results);
+ auto resultsCopy = results;
+ client.sendResults(std::move(resultsCopy));
auto maybeGetValueResults = getCallback()->nextGetValueResults();
ASSERT_TRUE(maybeGetValueResults.has_value());
@@ -160,7 +161,8 @@
SetValuesClient client(getPool(), getCallbackClient());
- client.sendResults(results);
+ auto resultsCopy = results;
+ client.sendResults(std::move(resultsCopy));
auto maybeSetValueResults = getCallback()->nextSetValueResults();
ASSERT_TRUE(maybeSetValueResults.has_value());
diff --git a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
index ff355c3..566f0bd 100644
--- a/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
+++ b/automotive/vehicle/aidl/impl/vhal/test/DefaultVehicleHalTest.cpp
@@ -371,7 +371,7 @@
return mVhal->mOnBinderDiedContexts[clientId].get();
}
- bool countOnBinderDiedContexts() {
+ size_t countOnBinderDiedContexts() {
std::scoped_lock<std::mutex> lockGuard(mVhal->mLock);
return mVhal->mOnBinderDiedContexts.size();
}
@@ -444,6 +444,7 @@
if (result.value() != nullptr) {
requests.payloads.clear();
requests.sharedMemoryFd = std::move(*result.value());
+ requests.payloads.clear();
}
return {};
}