From b562e7e7ace88ee7574334192f0c6f29dedaa245 Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Wed, 17 Nov 2021 13:07:11 +0000 Subject: [PATCH] Tidy ups from prior code reviews Switch to new(ish) Set.of() for construction of an immutable Set from an array of elements. Update ServerFlags to be more explicit about lookup behavior. Bug: 197624972 Test: Build only Change-Id: Ib86ffabf6b2ab47d89bda2cadb345eb9fdd42dbf --- .../server/timedetector/ServerFlags.java | 34 +++++++++++--- .../timedetector/ServiceConfigAccessor.java | 11 ++--- .../ServiceConfigAccessorImpl.java | 44 +++++++++---------- 3 files changed, 51 insertions(+), 38 deletions(-) diff --git a/services/core/java/com/android/server/timedetector/ServerFlags.java b/services/core/java/com/android/server/timedetector/ServerFlags.java index 65907f1801bb3..417177fa903ef 100644 --- a/services/core/java/com/android/server/timedetector/ServerFlags.java +++ b/services/core/java/com/android/server/timedetector/ServerFlags.java @@ -35,6 +35,7 @@ import java.lang.annotation.Target; import java.time.DateTimeException; import java.time.Duration; import java.time.Instant; +import java.util.HashSet; import java.util.Map; import java.util.Objects; import java.util.Optional; @@ -179,8 +180,13 @@ public final class ServerFlags { public static final @DeviceConfigKey String KEY_ENHANCED_METRICS_COLLECTION_ENABLED = "enhanced_metrics_collection_enabled"; + /** + * The registered listeners and the keys to trigger on. The value is explicitly a HashSet to + * ensure O(1) lookup performance when working out whether a listener should trigger. + */ @GuardedBy("mListeners") - private final ArrayMap> mListeners = new ArrayMap<>(); + private final ArrayMap> mListeners = + new ArrayMap<>(); private static final Object SLOCK = new Object(); @@ -207,18 +213,29 @@ public final class ServerFlags { private void handlePropertiesChanged(@NonNull DeviceConfig.Properties properties) { synchronized (mListeners) { - for (Map.Entry> listenerEntry + for (Map.Entry> listenerEntry : mListeners.entrySet()) { - if (intersects(listenerEntry.getValue(), properties.getKeyset())) { + // It's unclear which set of the following two Sets is going to be larger in the + // average case: monitoredKeys will be a subset of the set of possible keys, but + // only changed keys are reported. Because we guarantee the type / lookup behavior + // of the monitoredKeys by making that a HashSet, that is used as the haystack Set, + // while the changed keys is treated as the needles Iterable. At the time of + // writing, properties.getKeyset() actually returns a HashSet, so iteration isn't + // super efficient and the use of HashSet for monitoredKeys may be redundant, but + // neither set will be enormous. + HashSet monitoredKeys = listenerEntry.getValue(); + Iterable modifiedKeys = properties.getKeyset(); + if (containsAny(monitoredKeys, modifiedKeys)) { listenerEntry.getKey().onChange(); } } } } - private static boolean intersects(@NonNull Set one, @NonNull Set two) { - for (String toFind : one) { - if (two.contains(toFind)) { + private static boolean containsAny( + @NonNull Set haystack, @NonNull Iterable needles) { + for (String needle : needles) { + if (haystack.contains(needle)) { return true; } } @@ -237,8 +254,11 @@ public final class ServerFlags { Objects.requireNonNull(listener); Objects.requireNonNull(keys); + // Make a defensive copy and use a well-defined Set implementation to provide predictable + // performance on the lookup. + HashSet keysCopy = new HashSet<>(keys); synchronized (mListeners) { - mListeners.put(listener, keys); + mListeners.put(listener, keysCopy); } } diff --git a/services/core/java/com/android/server/timedetector/ServiceConfigAccessor.java b/services/core/java/com/android/server/timedetector/ServiceConfigAccessor.java index 7f7d01c29710d..5f1400097a3a2 100644 --- a/services/core/java/com/android/server/timedetector/ServiceConfigAccessor.java +++ b/services/core/java/com/android/server/timedetector/ServiceConfigAccessor.java @@ -25,7 +25,6 @@ import android.annotation.Nullable; import android.content.Context; import android.os.Build; import android.os.SystemProperties; -import android.util.ArraySet; import com.android.internal.R; import com.android.internal.annotations.GuardedBy; @@ -35,7 +34,6 @@ import com.android.server.timezonedetector.ConfigurationChangeListener; import java.time.Instant; import java.util.Arrays; -import java.util.Collections; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -65,11 +63,10 @@ final class ServiceConfigAccessor { Long.max(android.os.Environment.getRootDirectory().lastModified(), Build.TIME)); /** Device config keys that affect the {@link TimeDetectorService}. */ - private static final Set SERVER_FLAGS_KEYS_TO_WATCH = Collections.unmodifiableSet( - new ArraySet<>(new String[] { - KEY_TIME_DETECTOR_LOWER_BOUND_MILLIS_OVERRIDE, - KEY_TIME_DETECTOR_ORIGIN_PRIORITIES_OVERRIDE, - })); + private static final Set SERVER_FLAGS_KEYS_TO_WATCH = Set.of( + KEY_TIME_DETECTOR_LOWER_BOUND_MILLIS_OVERRIDE, + KEY_TIME_DETECTOR_ORIGIN_PRIORITIES_OVERRIDE + ); private static final Object SLOCK = new Object(); diff --git a/services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java b/services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java index b452d90b9ec79..ae52912d4821a 100644 --- a/services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java +++ b/services/core/java/com/android/server/timezonedetector/ServiceConfigAccessorImpl.java @@ -36,7 +36,6 @@ import android.location.LocationManager; import android.os.UserHandle; import android.os.UserManager; import android.provider.Settings; -import android.util.ArraySet; import com.android.internal.R; import com.android.internal.annotations.GuardedBy; @@ -45,7 +44,6 @@ import com.android.server.timedetector.ServerFlags; import java.time.Duration; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.Optional; @@ -59,33 +57,31 @@ public final class ServiceConfigAccessorImpl implements ServiceConfigAccessor { /** * Device config keys that can affect the content of {@link ConfigurationInternal}. */ - private static final Set CONFIGURATION_INTERNAL_SERVER_FLAGS_KEYS_TO_WATCH = - Collections.unmodifiableSet(new ArraySet<>(new String[] { - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_FEATURE_SUPPORTED, - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_RUN_IN_BACKGROUND_ENABLED, - ServerFlags.KEY_ENHANCED_METRICS_COLLECTION_ENABLED, - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_DEFAULT, - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_OVERRIDE, - ServerFlags.KEY_TIME_ZONE_DETECTOR_TELEPHONY_FALLBACK_SUPPORTED, - })); + private static final Set CONFIGURATION_INTERNAL_SERVER_FLAGS_KEYS_TO_WATCH = Set.of( + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_FEATURE_SUPPORTED, + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_RUN_IN_BACKGROUND_ENABLED, + ServerFlags.KEY_ENHANCED_METRICS_COLLECTION_ENABLED, + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_DEFAULT, + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_OVERRIDE, + ServerFlags.KEY_TIME_ZONE_DETECTOR_TELEPHONY_FALLBACK_SUPPORTED + ); /** * Device config keys that can affect {@link * com.android.server.timezonedetector.location.LocationTimeZoneManagerService} behavior. */ - private static final Set LOCATION_TIME_ZONE_MANAGER_SERVER_FLAGS_KEYS_TO_WATCH = - Collections.unmodifiableSet(new ArraySet<>(new String[] { - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_FEATURE_SUPPORTED, - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_RUN_IN_BACKGROUND_ENABLED, - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_DEFAULT, - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_OVERRIDE, - ServerFlags.KEY_PRIMARY_LTZP_MODE_OVERRIDE, - ServerFlags.KEY_SECONDARY_LTZP_MODE_OVERRIDE, - ServerFlags.KEY_LTZP_INITIALIZATION_TIMEOUT_MILLIS, - ServerFlags.KEY_LTZP_INITIALIZATION_TIMEOUT_FUZZ_MILLIS, - ServerFlags.KEY_LTZP_EVENT_FILTERING_AGE_THRESHOLD_MILLIS, - ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_UNCERTAINTY_DELAY_MILLIS - })); + private static final Set LOCATION_TIME_ZONE_MANAGER_SERVER_FLAGS_KEYS_TO_WATCH = Set.of( + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_FEATURE_SUPPORTED, + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_RUN_IN_BACKGROUND_ENABLED, + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_DEFAULT, + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_SETTING_ENABLED_OVERRIDE, + ServerFlags.KEY_PRIMARY_LTZP_MODE_OVERRIDE, + ServerFlags.KEY_SECONDARY_LTZP_MODE_OVERRIDE, + ServerFlags.KEY_LTZP_INITIALIZATION_TIMEOUT_MILLIS, + ServerFlags.KEY_LTZP_INITIALIZATION_TIMEOUT_FUZZ_MILLIS, + ServerFlags.KEY_LTZP_EVENT_FILTERING_AGE_THRESHOLD_MILLIS, + ServerFlags.KEY_LOCATION_TIME_ZONE_DETECTION_UNCERTAINTY_DELAY_MILLIS + ); private static final Duration DEFAULT_LTZP_INITIALIZATION_TIMEOUT = Duration.ofMinutes(5); private static final Duration DEFAULT_LTZP_INITIALIZATION_TIMEOUT_FUZZ = Duration.ofMinutes(1);