From 3dea629a9e950b48474f2ba81edc54dfa7259ef0 Mon Sep 17 00:00:00 2001 From: Zhi Dou Date: Wed, 25 Oct 2023 21:51:31 +0000 Subject: [PATCH] aconfig: cache flag value in generated java code This change add cache in generated jave code to improve the performance. The cache is a DeviceConfig.Properties. One Properties contains all flag values from the given namesapce. The Properties for a given namespace is initialized as null, and the first call for any flags in that Properties will trigger the call to fetch all the values. After the first call, the flag values are stored in the Properties, so the following calls will directly get the value from the Properties instead from the backup storage. Test: atest aconfig.test.java Bug: 307511846 Change-Id: Ic43626101c28099199b6faa419cf1945bd53b15d --- .../src/android/provider/DeviceConfig.java | 10 ++ tools/aconfig/src/codegen_java.rs | 166 +++++++++++++----- .../FakeFeatureFlagsImpl.java.template | 4 +- .../templates/FeatureFlags.java.template | 2 +- .../templates/FeatureFlagsImpl.java.template | 65 ++++--- tools/aconfig/templates/Flags.java.template | 4 +- 6 files changed, 172 insertions(+), 79 deletions(-) diff --git a/tools/aconfig/fake_device_config/src/android/provider/DeviceConfig.java b/tools/aconfig/fake_device_config/src/android/provider/DeviceConfig.java index 50b6289e4e..dbb07ac983 100644 --- a/tools/aconfig/fake_device_config/src/android/provider/DeviceConfig.java +++ b/tools/aconfig/fake_device_config/src/android/provider/DeviceConfig.java @@ -26,4 +26,14 @@ public class DeviceConfig { public static boolean getBoolean(String ns, String name, boolean def) { return false; } + + public static Properties getProperties(String namespace, String... names) { + return new Properties(); + } + + public static class Properties { + public boolean getBoolean(String name, boolean def) { + return false; + } + } } diff --git a/tools/aconfig/src/codegen_java.rs b/tools/aconfig/src/codegen_java.rs index 43c2ecfc36..05ee0d759f 100644 --- a/tools/aconfig/src/codegen_java.rs +++ b/tools/aconfig/src/codegen_java.rs @@ -16,6 +16,7 @@ use anyhow::Result; use serde::Serialize; +use std::collections::BTreeSet; use std::path::PathBuf; use tinytemplate::TinyTemplate; @@ -31,12 +32,19 @@ pub fn generate_java_code<'a, I>( where I: Iterator, { - let class_elements: Vec = - parsed_flags_iter.map(|pf| create_class_element(package, pf)).collect(); - let is_read_write = class_elements.iter().any(|elem| elem.is_read_write); + let flag_elements: Vec = + parsed_flags_iter.map(|pf| create_flag_element(package, pf)).collect(); + let properties_set: BTreeSet = + flag_elements.iter().map(|fe| format_property_name(&fe.device_config_namespace)).collect(); + let is_read_write = flag_elements.iter().any(|elem| elem.is_read_write); let is_test_mode = codegen_mode == CodegenMode::Test; - let context = - Context { class_elements, is_test_mode, is_read_write, package_name: package.to_string() }; + let context = Context { + flag_elements, + is_test_mode, + is_read_write, + properties_set, + package_name: package.to_string(), + }; let mut template = TinyTemplate::new(); template.add_template("Flags.java", include_str!("../templates/Flags.java.template"))?; template.add_template( @@ -66,49 +74,62 @@ where #[derive(Serialize)] struct Context { - pub class_elements: Vec, + pub flag_elements: Vec, pub is_test_mode: bool, pub is_read_write: bool, + pub properties_set: BTreeSet, pub package_name: String, } #[derive(Serialize)] -struct ClassElement { +struct FlagElement { pub default_value: bool, pub device_config_namespace: String, pub device_config_flag: String, pub flag_name_constant_suffix: String, pub is_read_write: bool, pub method_name: String, + pub properties: String, } -fn create_class_element(package: &str, pf: &ProtoParsedFlag) -> ClassElement { +fn create_flag_element(package: &str, pf: &ProtoParsedFlag) -> FlagElement { let device_config_flag = codegen::create_device_config_ident(package, pf.name()) .expect("values checked at flag parse time"); - ClassElement { + FlagElement { default_value: pf.state() == ProtoFlagState::ENABLED, device_config_namespace: pf.namespace().to_string(), device_config_flag, flag_name_constant_suffix: pf.name().to_ascii_uppercase(), is_read_write: pf.permission() == ProtoFlagPermission::READ_WRITE, method_name: format_java_method_name(pf.name()), + properties: format_property_name(pf.namespace()), } } fn format_java_method_name(flag_name: &str) -> String { - flag_name - .split('_') - .filter(|&word| !word.is_empty()) - .enumerate() - .map(|(index, word)| { - if index == 0 { - word.to_ascii_lowercase() - } else { - word[0..1].to_ascii_uppercase() + &word[1..].to_ascii_lowercase() - } - }) - .collect::>() - .join("") + let splits: Vec<&str> = flag_name.split('_').filter(|&word| !word.is_empty()).collect(); + if splits.len() == 1 { + let name = splits[0]; + name[0..1].to_ascii_lowercase() + &name[1..] + } else { + splits + .iter() + .enumerate() + .map(|(index, word)| { + if index == 0 { + word.to_ascii_lowercase() + } else { + word[0..1].to_ascii_uppercase() + &word[1..].to_ascii_lowercase() + } + }) + .collect::>() + .join("") + } +} + +fn format_property_name(property_name: &str) -> String { + let name = format_java_method_name(property_name); + format!("mProperties{}{}", &name[0..1].to_ascii_uppercase(), &name[1..]) } #[cfg(test)] @@ -265,8 +286,10 @@ mod tests { // TODO(b/303773055): Remove the annotation after access issue is resolved. import android.compat.annotation.UnsupportedAppUsage; import android.provider.DeviceConfig; + import android.provider.DeviceConfig.Properties; /** @hide */ public final class FeatureFlagsImpl implements FeatureFlags { + private Properties mPropertiesAconfigTest; @Override @UnsupportedAppUsage public boolean disabledRo() { @@ -275,11 +298,18 @@ mod tests { @Override @UnsupportedAppUsage public boolean disabledRw() { - return getValue( - "aconfig_test", - "com.android.aconfig.test.disabled_rw", - false - ); + if (mPropertiesAconfigTest == null) { + mPropertiesAconfigTest = + getProperties( + "aconfig_test", + "com.android.aconfig.test.disabled_rw" + ); + } + return mPropertiesAconfigTest + .getBoolean( + "com.android.aconfig.test.disabled_rw", + false + ); } @Override @UnsupportedAppUsage @@ -294,32 +324,36 @@ mod tests { @Override @UnsupportedAppUsage public boolean enabledRw() { - return getValue( - "aconfig_test", - "com.android.aconfig.test.enabled_rw", - true - ); - } - private boolean getValue(String nameSpace, - String flagName, boolean defaultValue) { - boolean value = defaultValue; - try { - value = DeviceConfig.getBoolean( - nameSpace, - flagName, - defaultValue + if (mPropertiesAconfigTest == null) { + mPropertiesAconfigTest = + getProperties( + "aconfig_test", + "com.android.aconfig.test.enabled_rw" + ); + } + return mPropertiesAconfigTest + .getBoolean( + "com.android.aconfig.test.enabled_rw", + true ); + } + private Properties getProperties( + String namespace, + String flagName) { + Properties properties = null; + try { + properties = DeviceConfig.getProperties(namespace); } catch (NullPointerException e) { throw new RuntimeException( - "Cannot read value of flag " + flagName + " from DeviceConfig. " + - "It could be that the code using flag executed " + - "before SettingsProvider initialization. " + - "Please use fixed read-only flag by adding " + - "is_fixed_read_only: true in flag declaration.", + "Cannot read value of flag " + flagName + " from DeviceConfig. " + + "It could be that the code using flag executed " + + "before SettingsProvider initialization. " + + "Please use fixed read-only flag by adding " + + "is_fixed_read_only: true in flag declaration.", e ); } - return value; + return properties; } } "#; @@ -441,9 +475,45 @@ mod tests { #[test] fn test_format_java_method_name() { - let input = "____some_snake___name____"; let expected = "someSnakeName"; + let input = "____some_snake___name____"; + let formatted_name = format_java_method_name(input); + assert_eq!(expected, formatted_name); + + let input = "someSnakeName"; + let formatted_name = format_java_method_name(input); + assert_eq!(expected, formatted_name); + + let input = "SomeSnakeName"; + let formatted_name = format_java_method_name(input); + assert_eq!(expected, formatted_name); + + let input = "SomeSnakeName_"; + let formatted_name = format_java_method_name(input); + assert_eq!(expected, formatted_name); + + let input = "_SomeSnakeName"; let formatted_name = format_java_method_name(input); assert_eq!(expected, formatted_name); } + + #[test] + fn test_format_property_name() { + let expected = "mPropertiesSomeSnakeName"; + let input = "____some_snake___name____"; + let formatted_name = format_property_name(input); + assert_eq!(expected, formatted_name); + + let input = "someSnakeName"; + let formatted_name = format_property_name(input); + assert_eq!(expected, formatted_name); + + let input = "SomeSnakeName"; + let formatted_name = format_property_name(input); + assert_eq!(expected, formatted_name); + + let input = "SomeSnakeName_"; + let formatted_name = format_property_name(input); + assert_eq!(expected, formatted_name); + } } diff --git a/tools/aconfig/templates/FakeFeatureFlagsImpl.java.template b/tools/aconfig/templates/FakeFeatureFlagsImpl.java.template index 72a896f997..933d6a77cb 100644 --- a/tools/aconfig/templates/FakeFeatureFlagsImpl.java.template +++ b/tools/aconfig/templates/FakeFeatureFlagsImpl.java.template @@ -11,7 +11,7 @@ public class FakeFeatureFlagsImpl implements FeatureFlags \{ resetAll(); } -{{ for item in class_elements}} +{{ for item in flag_elements}} @Override @UnsupportedAppUsage public boolean {item.method_name}() \{ @@ -41,7 +41,7 @@ public class FakeFeatureFlagsImpl implements FeatureFlags \{ private Map mFlagMap = new HashMap<>( Map.ofEntries( - {{-for item in class_elements}} + {{-for item in flag_elements}} Map.entry(Flags.FLAG_{item.flag_name_constant_suffix}, false) {{ -if not @last }},{{ endif }} {{ -endfor }} diff --git a/tools/aconfig/templates/FeatureFlags.java.template b/tools/aconfig/templates/FeatureFlags.java.template index 02305e69fe..da850aec4f 100644 --- a/tools/aconfig/templates/FeatureFlags.java.template +++ b/tools/aconfig/templates/FeatureFlags.java.template @@ -4,7 +4,7 @@ import android.compat.annotation.UnsupportedAppUsage; /** @hide */ public interface FeatureFlags \{ -{{ for item in class_elements}} +{{ for item in flag_elements }} {{ -if not item.is_read_write }} {{ -if item.default_value }} @com.android.aconfig.annotations.AssumeTrueForR8 diff --git a/tools/aconfig/templates/FeatureFlagsImpl.java.template b/tools/aconfig/templates/FeatureFlagsImpl.java.template index 1620dfe85d..ff089dfdc8 100644 --- a/tools/aconfig/templates/FeatureFlagsImpl.java.template +++ b/tools/aconfig/templates/FeatureFlagsImpl.java.template @@ -4,45 +4,58 @@ import android.compat.annotation.UnsupportedAppUsage; {{ if not is_test_mode }} {{ if is_read_write- }} import android.provider.DeviceConfig; +import android.provider.DeviceConfig.Properties; {{ endif }} /** @hide */ public final class FeatureFlagsImpl implements FeatureFlags \{ -{{ for item in class_elements}} +{{ if is_read_write- }} +{{ for properties in properties_set }} + private Properties {properties}; +{{ endfor }} +{{ endif- }} + +{{ for flag in flag_elements }} @Override @UnsupportedAppUsage - public boolean {item.method_name}() \{ - {{ -if item.is_read_write }} - return getValue( - "{item.device_config_namespace}", - "{item.device_config_flag}", - {item.default_value} - ); + public boolean {flag.method_name}() \{ + {{ -if flag.is_read_write }} + if ({flag.properties} == null) \{ + {flag.properties} = + getProperties( + "{flag.device_config_namespace}", + "{flag.device_config_flag}" + ); + } + return {flag.properties} + .getBoolean( + "{flag.device_config_flag}", + {flag.default_value} + ); {{ else }} - return {item.default_value}; + return {flag.default_value}; {{ endif- }} } {{ endfor }} -{{ if is_read_write- }} - private boolean getValue(String nameSpace, - String flagName, boolean defaultValue) \{ - boolean value = defaultValue; + +{{ -if is_read_write }} + private Properties getProperties( + String namespace, + String flagName) \{ + Properties properties = null; try \{ - value = DeviceConfig.getBoolean( - nameSpace, - flagName, - defaultValue - ); + properties = DeviceConfig.getProperties(namespace); } catch (NullPointerException e) \{ throw new RuntimeException( - "Cannot read value of flag " + flagName + " from DeviceConfig. " + - "It could be that the code using flag executed " + - "before SettingsProvider initialization. " + - "Please use fixed read-only flag by adding " + - "is_fixed_read_only: true in flag declaration.", + "Cannot read value of flag " + flagName + " from DeviceConfig. " + + "It could be that the code using flag executed " + + "before SettingsProvider initialization. " + + "Please use fixed read-only flag by adding " + + "is_fixed_read_only: true in flag declaration.", e ); } - return value; + + return properties; } {{ endif- }} } @@ -50,10 +63,10 @@ public final class FeatureFlagsImpl implements FeatureFlags \{ {#- Generate only stub if in test mode #} /** @hide */ public final class FeatureFlagsImpl implements FeatureFlags \{ -{{ for item in class_elements}} +{{ for flag in flag_elements }} @Override @UnsupportedAppUsage - public boolean {item.method_name}() \{ + public boolean {flag.method_name}() \{ throw new UnsupportedOperationException( "Method is not implemented."); } diff --git a/tools/aconfig/templates/Flags.java.template b/tools/aconfig/templates/Flags.java.template index 66c4c5a114..cf6604cfa3 100644 --- a/tools/aconfig/templates/Flags.java.template +++ b/tools/aconfig/templates/Flags.java.template @@ -5,11 +5,11 @@ import android.compat.annotation.UnsupportedAppUsage; /** @hide */ public final class Flags \{ -{{- for item in class_elements}} +{{- for item in flag_elements}} /** @hide */ public static final String FLAG_{item.flag_name_constant_suffix} = "{item.device_config_flag}"; {{- endfor }} -{{ for item in class_elements}} +{{ for item in flag_elements}} {{ -if not item.is_read_write }} {{ -if item.default_value }} @com.android.aconfig.annotations.AssumeTrueForR8