Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit d1e9ca2c authored by Yohei Yukawa's avatar Yohei Yukawa
Browse files

Make *Repository#get(int userId) lock-free

This reworks how IME-related per-user data can be queried in a
thread-safe manner.

Basically the idea is to make query operations lock-free by making
sure that the underlying SparseArray is immutable. While we still need
a lock for data mutation, simple read access no longer requires any
lock at the cost of putting the sparse array into a volatile field,
which has gotten pretty cheap on both ARM and remain so on X86 at
least compared to synchronized blocks.

There must be no observable behavior change beyond the performance
characteristics.

Bug: 353307830
Fix: 352594784
Test: atest FrameworksInputMethodSystemServerTests:ImmutableSparseArrayTest
Flag: EXEMPT refactor
Change-Id: Id0783e57c7e8ff70f16f3c3d486f5a5e64bbe7db
parent e47b907e
Loading
Loading
Loading
Loading
+20 −30
Original line number Diff line number Diff line
@@ -38,10 +38,11 @@ import java.util.concurrent.locks.ReentrantLock;
final class AdditionalSubtypeMapRepository {
    private static final String TAG = "AdditionalSubtypeMapRepository";

    // TODO(b/352594784): Should we user other lock primitives?
    @GuardedBy("sPerUserMap")
    private static final Object sMutationLock = new Object();

    @NonNull
    private static final SparseArray<AdditionalSubtypeMap> sPerUserMap = new SparseArray<>();
    private static volatile ImmutableSparseArray<AdditionalSubtypeMap> sPerUserMap =
            ImmutableSparseArray.empty();

    record WriteTask(@UserIdInt int userId, @NonNull AdditionalSubtypeMap subtypeMap,
                     @NonNull InputMethodMap inputMethodMap) {
@@ -198,7 +199,7 @@ final class AdditionalSubtypeMapRepository {
    /**
     * Returns {@link AdditionalSubtypeMap} for the given user.
     *
     * <p>This method is expected be called after {@link #ensureInitializedAndGet(int)}. Otherwise
     * <p>This method is expected be called after {@link #initializeIfNecessary(int)}. Otherwise
     * {@link AdditionalSubtypeMap#EMPTY_MAP} will be returned.</p>
     *
     * @param userId the user to be queried about
@@ -207,10 +208,7 @@ final class AdditionalSubtypeMapRepository {
    @AnyThread
    @NonNull
    static AdditionalSubtypeMap get(@UserIdInt int userId) {
        final AdditionalSubtypeMap map;
        synchronized (sPerUserMap) {
            map = sPerUserMap.get(userId);
        }
        final AdditionalSubtypeMap map = sPerUserMap.get(userId);
        if (map == null) {
            Slog.e(TAG, "get(userId=" + userId + ") is called before loadInitialDataAndGet()."
                    + " Returning an empty map");
@@ -220,28 +218,24 @@ final class AdditionalSubtypeMapRepository {
    }

    /**
     * Ensures that {@link AdditionalSubtypeMap} is initialized for the given user. Load it from
     * the persistent storage if {@link #putAndSave(int, AdditionalSubtypeMap, InputMethodMap)} has
     * not been called yet.
     * Ensures that {@link AdditionalSubtypeMap} is initialized for the given user.
     *
     * @param userId the user to be initialized
     * @return {@link AdditionalSubtypeMap} that is associated with the given user. If
     *         {@link #putAndSave(int, AdditionalSubtypeMap, InputMethodMap)} is already called
     *         then the given {@link AdditionalSubtypeMap}.
     */
    @AnyThread
    @NonNull
    static AdditionalSubtypeMap ensureInitializedAndGet(@UserIdInt int userId) {
    static void initializeIfNecessary(@UserIdInt int userId) {
        if (sPerUserMap.contains(userId)) {
            // Fast-pass. If putAndSave() is already called, then do nothing.
            return;
        }
        final var map = AdditionalSubtypeUtils.load(userId);
        synchronized (sPerUserMap) {
            final AdditionalSubtypeMap previous = sPerUserMap.get(userId);
            // If putAndSave() has already been called, then use it.
            if (previous != null) {
                return previous;
        synchronized (sMutationLock) {
            // Check the condition again.
            if (!sPerUserMap.contains(userId)) {
                sPerUserMap = sPerUserMap.cloneWithPutOrSelf(userId, map);
            }
            sPerUserMap.put(userId, map);
        }
        return map;
    }

    /**
@@ -255,12 +249,8 @@ final class AdditionalSubtypeMapRepository {
    @AnyThread
    static void putAndSave(@UserIdInt int userId, @NonNull AdditionalSubtypeMap map,
            @NonNull InputMethodMap inputMethodMap) {
        synchronized (sPerUserMap) {
            final AdditionalSubtypeMap previous = sPerUserMap.get(userId);
            if (previous == map) {
                return;
            }
            sPerUserMap.put(userId, map);
        synchronized (sMutationLock) {
            sPerUserMap = sPerUserMap.cloneWithPutOrSelf(userId, map);
            sWriter.scheduleWriteTask(userId, map, inputMethodMap);
        }
    }
@@ -277,9 +267,9 @@ final class AdditionalSubtypeMapRepository {

    @AnyThread
    static void remove(@UserIdInt int userId) {
        synchronized (sPerUserMap) {
        synchronized (sMutationLock) {
            sWriter.onUserRemoved(userId);
            sPerUserMap.remove(userId);
            sPerUserMap = sPerUserMap.cloneWithRemoveOrSelf(userId);
        }
    }
}
+183 −0
Original line number Diff line number Diff line
/*
 * Copyright (C) 2024 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

package com.android.server.inputmethod;


import android.annotation.AnyThread;
import android.annotation.NonNull;
import android.annotation.Nullable;
import android.util.SparseArray;

import java.util.function.Consumer;

/**
 * A holder object to expose {@link SparseArray} to multiple threads in a thread-safe manner through
 * "final Field Semantics" defined in JLS 17.5, with only exposing thread-safe methods such as
 * {@link SparseArray#get(int)} and {@link SparseArray#size()} from {@link SparseArray}, and with
 * adding clone-with-update style methods {@link #cloneWithPutOrSelf(int, Object)} and
 * {@link #cloneWithRemoveOrSelf(int)} instead of exposing mutation methods.
 *
 * @param <E> Type of the element
 */
final class ImmutableSparseArray<E> {
    @NonNull
    private final SparseArray<E> mArray;

    private static final ImmutableSparseArray<Object> EMPTY =
            new ImmutableSparseArray<>(new SparseArray<>());

    /**
     * Returns an empty {@link ImmutableSparseArray} instance.
     *
     * @return An empty {@link ImmutableSparseArray} instance.
     * @param <T> Type of the element
     */
    @SuppressWarnings("unchecked")
    @AnyThread
    @NonNull
    static <T> ImmutableSparseArray<T> empty() {
        return (ImmutableSparseArray<T>) EMPTY;
    }

    private ImmutableSparseArray(@NonNull SparseArray<E> array) {
        mArray = array;
    }

    /**
     * @return the size of this array
     */
    @AnyThread
    int size() {
        return mArray.size();
    }

    /**
     * Returns the key of the specified index.
     *
     * @return the key of the specified index
     * @throws ArrayIndexOutOfBoundsException when the index is out of range
     */
    @AnyThread
    int keyAt(int index) {
        return mArray.keyAt(index);
    }

    /**
     * Returns the value of the specified index.
     *
     * @return the value of the specified index
     * @throws ArrayIndexOutOfBoundsException when the index is out of range
     */
    @AnyThread
    @Nullable
    public E valueAt(int index) {
        return mArray.valueAt(index);
    }

    /**
     * Returns the index of the specified key.
     *
     * @return the index of the specified key if exists. Otherwise {@code -1}
     */
    @AnyThread
    int indexOfKey(int key) {
        return mArray.indexOfKey(key);
    }

    /**
     * Returns {@code true} if the given {@code key} exists.
     *
     * @param key the key to be queried
     * @return    {@code true} if the given {@code key} exists
     */
    @AnyThread
    boolean contains(int key) {
        return mArray.contains(key);
    }

    /**
     * Returns the value associated with the {@code key}.
     *
     * @param key the key to be queried
     * @return    the value associated with the {@code key} if exists. Otherwise {@code null}
     */
    @AnyThread
    @Nullable
    E get(int key) {
        return mArray.get(key);
    }

    /**
     * Run {@link Consumer} for each value.
     *
     * @param consumer {@link Consumer} to be called back
     */
    @AnyThread
    void forEach(@NonNull Consumer<E> consumer) {
        final int size = mArray.size();
        for (int i = 0; i < size; ++i) {
            consumer.accept(mArray.valueAt(i));
        }
    }

    /**
     * Returns an instance of {@link ImmutableSparseArray} that has the given key and value on top
     * of items cloned from this instance.
     *
     * @param key   the key to be added
     * @param value the value to be added
     * @return      the same {@link ImmutableSparseArray} instance if there is actually no update.
     *              Otherwise, a new instance of {@link ImmutableSparseArray}
     */
    @AnyThread
    @NonNull
    ImmutableSparseArray<E> cloneWithPutOrSelf(int key, @Nullable E value) {
        final var prevKeyIndex = mArray.indexOfKey(key);
        if (prevKeyIndex >= 0) {
            final var prevValue = mArray.valueAt(prevKeyIndex);
            if (prevValue == value) {
                return this;
            }
        }
        final var clone = mArray.clone();
        clone.put(key, value);
        return new ImmutableSparseArray<>(clone);
    }

    /**
     * Returns an instance of {@link ImmutableSparseArray} that does not have the given key on top
     * of items cloned from this instance.
     *
     * @param key the key to be removed
     * @return    the same {@link ImmutableSparseArray} instance if there is actually no update.
     *            Otherwise, a new instance of {@link ImmutableSparseArray}
     */
    @AnyThread
    @NonNull
    ImmutableSparseArray<E> cloneWithRemoveOrSelf(int key) {
        final int index = indexOfKey(key);
        if (index < 0) {
            return this;
        }
        if (mArray.size() == 1) {
            return empty();
        }
        final var clone = mArray.clone();
        clone.remove(key);
        return new ImmutableSparseArray<>(clone);
    }
}
+2 −2
Original line number Diff line number Diff line
@@ -1063,8 +1063,8 @@ public final class InputMethodManagerService implements IInputMethodManagerImpl.

                for (int userId : userIds) {
                    Slog.d(TAG, "Start initialization for user=" + userId);
                    final var additionalSubtypeMap =
                            AdditionalSubtypeMapRepository.ensureInitializedAndGet(userId);
                    AdditionalSubtypeMapRepository.initializeIfNecessary(userId);
                    final var additionalSubtypeMap = AdditionalSubtypeMapRepository.get(userId);
                    final var settings = InputMethodManagerService.queryInputMethodServicesInternal(
                            context, userId, additionalSubtypeMap,
                            DirectBootAwareness.AUTO).getMethodMap();
+9 −14
Original line number Diff line number Diff line
@@ -19,15 +19,13 @@ package com.android.server.inputmethod;
import android.annotation.AnyThread;
import android.annotation.NonNull;
import android.annotation.UserIdInt;
import android.util.SparseArray;

import com.android.internal.annotations.GuardedBy;

final class InputMethodSettingsRepository {
    // TODO(b/352594784): Should we user other lock primitives?
    @GuardedBy("sPerUserMap")
    private static final Object sMutationLock = new Object();

    @NonNull
    private static final SparseArray<InputMethodSettings> sPerUserMap = new SparseArray<>();
    private static volatile ImmutableSparseArray<InputMethodSettings> sPerUserMap =
            ImmutableSparseArray.empty();

    /**
     * Not intended to be instantiated.
@@ -38,10 +36,7 @@ final class InputMethodSettingsRepository {
    @NonNull
    @AnyThread
    static InputMethodSettings get(@UserIdInt int userId) {
        final InputMethodSettings obj;
        synchronized (sPerUserMap) {
            obj = sPerUserMap.get(userId);
        }
        final InputMethodSettings obj = sPerUserMap.get(userId);
        if (obj != null) {
            return obj;
        }
@@ -50,15 +45,15 @@ final class InputMethodSettingsRepository {

    @AnyThread
    static void put(@UserIdInt int userId, @NonNull InputMethodSettings obj) {
        synchronized (sPerUserMap) {
            sPerUserMap.put(userId, obj);
        synchronized (sMutationLock) {
            sPerUserMap = sPerUserMap.cloneWithPutOrSelf(userId, obj);
        }
    }

    @AnyThread
    static void remove(@UserIdInt int userId) {
        synchronized (sPerUserMap) {
            sPerUserMap.remove(userId);
        synchronized (sMutationLock) {
            sPerUserMap = sPerUserMap.cloneWithRemoveOrSelf(userId);
        }
    }
}
+17 −17
Original line number Diff line number Diff line
@@ -24,7 +24,6 @@ import android.content.ContentResolver;
import android.provider.Settings;
import android.util.ArrayMap;
import android.util.ArraySet;
import android.util.SparseArray;

import com.android.internal.annotations.GuardedBy;
import com.android.server.LocalServices;
@@ -38,6 +37,13 @@ import com.android.server.pm.UserManagerInternal;
 * to the persistent value when the user storage is unlocked.</p>
 */
final class SecureSettingsWrapper {

    private static final Object sMutationLock = new Object();

    @NonNull
    private static volatile ImmutableSparseArray<ReaderWriter> sUserMap =
            ImmutableSparseArray.empty();

    @Nullable
    private static volatile ContentResolver sContentResolver = null;

@@ -61,8 +67,8 @@ final class SecureSettingsWrapper {
     */
    @AnyThread
    static void endTestMode() {
        synchronized (sUserMap) {
            sUserMap.clear();
        synchronized (sMutationLock) {
            sUserMap = ImmutableSparseArray.empty();
        }
        sTestMode = false;
    }
@@ -243,10 +249,6 @@ final class SecureSettingsWrapper {
        }
    }

    @GuardedBy("sUserMap")
    @NonNull
    private static final SparseArray<ReaderWriter> sUserMap = new SparseArray<>();

    private static final ReaderWriter NOOP = new ReaderWriter() {
        @Override
        public void putString(String key, String str) {
@@ -282,15 +284,15 @@ final class SecureSettingsWrapper {
    private static ReaderWriter putOrGet(@UserIdInt int userId,
            @NonNull ReaderWriter readerWriter) {
        final boolean isUnlockedUserImpl = readerWriter instanceof UnlockedUserImpl;
        synchronized (sUserMap) {
        synchronized (sMutationLock) {
            final ReaderWriter current = sUserMap.get(userId);
            if (current == null) {
                sUserMap.put(userId, readerWriter);
                sUserMap = sUserMap.cloneWithPutOrSelf(userId, readerWriter);
                return readerWriter;
            }
            // Upgrading from CopyOnWriteImpl to DirectImpl is allowed.
            if (current instanceof LockedUserImpl && isUnlockedUserImpl) {
                sUserMap.put(userId, readerWriter);
                sUserMap = sUserMap.cloneWithPutOrSelf(userId, readerWriter);
                return readerWriter;
            }
            return current;
@@ -300,12 +302,10 @@ final class SecureSettingsWrapper {
    @NonNull
    @AnyThread
    private static ReaderWriter get(@UserIdInt int userId) {
        synchronized (sUserMap) {
        final ReaderWriter readerWriter = sUserMap.get(userId);
        if (readerWriter != null) {
            return readerWriter;
        }
        }
        if (sTestMode) {
            return putOrGet(userId, new FakeReaderWriterImpl());
        }
@@ -363,8 +363,8 @@ final class SecureSettingsWrapper {
     */
    @AnyThread
    static void onUserRemoved(@UserIdInt int userId) {
        synchronized (sUserMap) {
            sUserMap.remove(userId);
        synchronized (sMutationLock) {
            sUserMap = sUserMap.cloneWithRemoveOrSelf(userId);
        }
    }

Loading