Check all art-related system properties. am: dff7de431e am: 67b1370d67
Original change: https://googleplex-android-review.googlesource.com/c/platform/art/+/18395311
Change-Id: I1e6835ea800e85f3e9cdac1c0caa23f09a4a8a34
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
diff --git a/odrefresh/odr_common.cc b/odrefresh/odr_common.cc
index a83e18e..94674bc 100644
--- a/odrefresh/odr_common.cc
+++ b/odrefresh/odr_common.cc
@@ -16,6 +16,9 @@
#include "odr_common.h"
+#include <sys/system_properties.h>
+
+#include <functional>
#include <initializer_list>
#include <regex>
#include <sstream>
@@ -80,5 +83,21 @@
return sdk_version >= 32;
}
+void SystemPropertyForeach(std::function<void(const char* name, const char* value)> action) {
+ __system_property_foreach(
+ [](const prop_info* pi, void* cookie) {
+ __system_property_read_callback(
+ pi,
+ [](void* cookie, const char* name, const char* value, unsigned) {
+ auto action =
+ reinterpret_cast<std::function<void(const char* name, const char* value)>*>(
+ cookie);
+ (*action)(name, value);
+ },
+ cookie);
+ },
+ &action);
+}
+
} // namespace odrefresh
} // namespace art
diff --git a/odrefresh/odr_common.h b/odrefresh/odr_common.h
index 63f35f9..1257ab7 100644
--- a/odrefresh/odr_common.h
+++ b/odrefresh/odr_common.h
@@ -44,6 +44,9 @@
// `ro.build.version.sdk`, which represents the SDK version.
bool ShouldDisableRefresh(const std::string& sdk_version_str);
+// Passes the name and the value for each system property to the provided callback.
+void SystemPropertyForeach(std::function<void(const char* name, const char* value)> action);
+
} // namespace odrefresh
} // namespace art
diff --git a/odrefresh/odr_config.h b/odrefresh/odr_config.h
index c3f3c80..0475466 100644
--- a/odrefresh/odr_config.h
+++ b/odrefresh/odr_config.h
@@ -17,6 +17,7 @@
#ifndef ART_ODREFRESH_ODR_CONFIG_H_
#define ART_ODREFRESH_ODR_CONFIG_H_
+#include <algorithm>
#include <optional>
#include <string>
#include <unordered_map>
@@ -24,6 +25,7 @@
#include "android-base/file.h"
#include "android-base/no_destructor.h"
+#include "android-base/strings.h"
#include "arch/instruction_set.h"
#include "base/file_utils.h"
#include "base/globals.h"
@@ -34,13 +36,23 @@
namespace art {
namespace odrefresh {
+// The prefixes of system properties that odrefresh keeps track of. Odrefresh will recompile
+// everything if any property matching a prefix changes.
+constexpr const char* kCheckedSystemPropertyPrefixes[]{"dalvik.vm.", "ro.dalvik.vm."};
+
struct SystemPropertyConfig {
const char* name;
const char* default_value;
};
-// The system properties that odrefresh keeps track of. Odrefresh will recompile everything if any
-// property changes.
+// The system properties that odrefresh keeps track of, in addition to the ones matching the
+// prefixes in `kCheckedSystemPropertyPrefixes`. Odrefresh will recompile everything if any property
+// changes.
+// All phenotype flags under the `runtime_native_boot` namespace that affects the compiler's
+// behavior must be explicitly listed below. We cannot use a prefix to match all phenotype flags
+// because a default value is required for each flag. Changing the flag value from empty to the
+// default value should not trigger re-compilation. This is to comply with the phenotype flag
+// requirement (go/platform-experiments-flags#pre-requisites).
const android::base::NoDestructor<std::vector<SystemPropertyConfig>> kSystemProperties{
{SystemPropertyConfig{.name = "persist.device_config.runtime_native_boot.enable_uffd_gc",
.default_value = "false"}}};
diff --git a/odrefresh/odrefresh.cc b/odrefresh/odrefresh.cc
index 70cf48e..f829bb8 100644
--- a/odrefresh/odrefresh.cc
+++ b/odrefresh/odrefresh.cc
@@ -843,6 +843,13 @@
}
WARN_UNUSED bool OnDeviceRefresh::CheckSystemPropertiesAreDefault() const {
+ // We don't have to check properties that match `kCheckedSystemPropertyPrefixes` here because none
+ // of them is persistent. This only applies when `cache-info.xml` does not exist. When
+ // `cache-info.xml` exists, we call `CheckSystemPropertiesHaveNotChanged` instead.
+ DCHECK(std::none_of(std::begin(kCheckedSystemPropertyPrefixes),
+ std::end(kCheckedSystemPropertyPrefixes),
+ [](const char* prefix) { return StartsWith(prefix, "persist."); }));
+
const std::unordered_map<std::string, std::string>& system_properties =
config_.GetSystemProperties();
@@ -863,6 +870,8 @@
WARN_UNUSED bool OnDeviceRefresh::CheckSystemPropertiesHaveNotChanged(
const art_apex::CacheInfo& cache_info) const {
std::unordered_map<std::string, std::string> cached_system_properties;
+ std::unordered_set<std::string> checked_properties;
+
const art_apex::KeyValuePairList* list = cache_info.getFirstSystemProperties();
if (list == nullptr) {
// This should never happen. We have already checked the ART module version, and the cache
@@ -873,28 +882,24 @@
for (const art_apex::KeyValuePair& pair : list->getItem()) {
cached_system_properties[pair.getK()] = pair.getV();
+ checked_properties.insert(pair.getK());
}
const std::unordered_map<std::string, std::string>& system_properties =
config_.GetSystemProperties();
- for (const SystemPropertyConfig& system_property_config : *kSystemProperties.get()) {
- auto property = system_properties.find(system_property_config.name);
- DCHECK(property != system_properties.end());
+ for (const auto& [key, value] : system_properties) {
+ checked_properties.insert(key);
+ }
- auto cached_property = cached_system_properties.find(system_property_config.name);
- if (cached_property == cached_system_properties.end()) {
- // This should never happen. We have already checked the ART module version, and the cache
- // info is generated by the latest version of the ART module if it exists.
- LOG(ERROR) << "Missing system property from cache-info (" << system_property_config.name
- << ")";
- return false;
- }
+ for (const std::string& name : checked_properties) {
+ auto property_it = system_properties.find(name);
+ std::string property = property_it != system_properties.end() ? property_it->second : "";
+ std::string cached_property = cached_system_properties[name];
- if (property->second != cached_property->second) {
- LOG(INFO) << "System property " << system_property_config.name
- << " value changed (before: " << cached_property->second
- << ", now: " << property->second << ").";
+ if (property != cached_property) {
+ LOG(INFO) << "System property " << name << " value changed (before: \"" << cached_property
+ << "\", now: \"" << property << "\").";
return false;
}
}
diff --git a/odrefresh/odrefresh.h b/odrefresh/odrefresh.h
index 9dd4009..e48567f 100644
--- a/odrefresh/odrefresh.h
+++ b/odrefresh/odrefresh.h
@@ -141,11 +141,12 @@
/*out*/ std::vector<std::string>* checked_artifacts = nullptr) const;
// Returns true if all of the system properties listed in `kSystemProperties` are set to the
- // default values.
+ // default values. This function is usually called when cache-info.xml does not exist (i.e.,
+ // compilation has not been done before).
WARN_UNUSED bool CheckSystemPropertiesAreDefault() const;
// Returns true if none of the system properties listed in `kSystemProperties` has changed since
- // the last compilation.
+ // the last compilation. This function is usually called when cache-info.xml exists.
WARN_UNUSED bool CheckSystemPropertiesHaveNotChanged(
const com::android::art::CacheInfo& cache_info) const;
diff --git a/odrefresh/odrefresh_main.cc b/odrefresh/odrefresh_main.cc
index f8cc0b1..58ef28f 100644
--- a/odrefresh/odrefresh_main.cc
+++ b/odrefresh/odrefresh_main.cc
@@ -36,8 +36,10 @@
namespace {
using ::android::base::GetProperty;
+using ::android::base::StartsWith;
using ::art::odrefresh::CompilationOptions;
using ::art::odrefresh::ExitCode;
+using ::art::odrefresh::kCheckedSystemPropertyPrefixes;
using ::art::odrefresh::kSystemProperties;
using ::art::odrefresh::OdrCompilationLog;
using ::art::odrefresh::OdrConfig;
@@ -47,6 +49,7 @@
using ::art::odrefresh::ShouldDisablePartialCompilation;
using ::art::odrefresh::ShouldDisableRefresh;
using ::art::odrefresh::SystemPropertyConfig;
+using ::art::odrefresh::SystemPropertyForeach;
using ::art::odrefresh::ZygoteKind;
void UsageMsgV(const char* fmt, va_list ap) {
@@ -187,6 +190,16 @@
}
void GetSystemProperties(std::unordered_map<std::string, std::string>* system_properties) {
+ SystemPropertyForeach([&](const char* name, const char* value) {
+ if (strlen(value) == 0) {
+ return;
+ }
+ for (const char* prefix : kCheckedSystemPropertyPrefixes) {
+ if (StartsWith(name, prefix)) {
+ (*system_properties)[name] = value;
+ }
+ }
+ });
for (const SystemPropertyConfig& system_property_config : *kSystemProperties.get()) {
(*system_properties)[system_property_config.name] =
GetProperty(system_property_config.name, system_property_config.default_value);
diff --git a/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java b/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java
index de497c5..731ea38 100644
--- a/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java
+++ b/test/odsign/test-src/com/android/tests/odsign/OdrefreshHostTest.java
@@ -201,6 +201,60 @@
}
@Test
+ public void verifySystemPropertyMismatchTriggersCompilation() throws Exception {
+ // Change a system property from empty to a value.
+ getDevice().setProperty("dalvik.vm.foo", "1");
+ long timeMs = mTestUtils.getCurrentTimeMs();
+ getDevice().executeShellV2Command(ODREFRESH_COMMAND);
+
+ // Artifacts should be re-compiled.
+ assertArtifactsModifiedAfter(getZygoteArtifacts(), timeMs);
+ assertArtifactsModifiedAfter(getSystemServerArtifacts(), timeMs);
+
+ // Run again with the same value.
+ timeMs = mTestUtils.getCurrentTimeMs();
+ getDevice().executeShellV2Command(ODREFRESH_COMMAND);
+
+ // Artifacts should not be re-compiled.
+ assertArtifactsNotModifiedAfter(getZygoteArtifacts(), timeMs);
+ assertArtifactsNotModifiedAfter(getSystemServerArtifacts(), timeMs);
+
+ // Change the system property to another value.
+ getDevice().setProperty("dalvik.vm.foo", "2");
+ timeMs = mTestUtils.getCurrentTimeMs();
+ getDevice().executeShellV2Command(ODREFRESH_COMMAND);
+
+ // Artifacts should be re-compiled.
+ assertArtifactsModifiedAfter(getZygoteArtifacts(), timeMs);
+ assertArtifactsModifiedAfter(getSystemServerArtifacts(), timeMs);
+
+ // Run again with the same value.
+ timeMs = mTestUtils.getCurrentTimeMs();
+ getDevice().executeShellV2Command(ODREFRESH_COMMAND);
+
+ // Artifacts should not be re-compiled.
+ assertArtifactsNotModifiedAfter(getZygoteArtifacts(), timeMs);
+ assertArtifactsNotModifiedAfter(getSystemServerArtifacts(), timeMs);
+
+ // Change the system property to empty.
+ getDevice().setProperty("dalvik.vm.foo", "");
+ timeMs = mTestUtils.getCurrentTimeMs();
+ getDevice().executeShellV2Command(ODREFRESH_COMMAND);
+
+ // Artifacts should be re-compiled.
+ assertArtifactsModifiedAfter(getZygoteArtifacts(), timeMs);
+ assertArtifactsModifiedAfter(getSystemServerArtifacts(), timeMs);
+
+ // Run again with the same value.
+ timeMs = mTestUtils.getCurrentTimeMs();
+ getDevice().executeShellV2Command(ODREFRESH_COMMAND);
+
+ // Artifacts should not be re-compiled.
+ assertArtifactsNotModifiedAfter(getZygoteArtifacts(), timeMs);
+ assertArtifactsNotModifiedAfter(getSystemServerArtifacts(), timeMs);
+ }
+
+ @Test
public void verifyNoCompilationWhenCacheIsGood() throws Exception {
mTestUtils.removeCompilationLogToAvoidBackoff();
long timeMs = mTestUtils.getCurrentTimeMs();