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

Commit d42bd3f0 authored by Fengjiang Li's avatar Fengjiang Li
Browse files

[Launcher Jank] Make ChangeReporter faster 

1. Use compare-and-set in ChangeReporter#reportChange() to reduce 1 read
   access of isAlreadyReported() to mReportedChanges. Also avoided
creating a new ChangeReport obj.
2. Make mReportedChanges thread safe by using ConcurrentHashMap<Uid, SynchronizedSet<ChangeReport>> to remove system-wide lock contention of `ChangeReporter#isAlreadyReported()`

The pattern of accessing the data structure is that a thread will serially call 2 `isAlreadyReported()` then 1 `markAsReported()` in a row from `ChangeReporter#reportChange()` https://screenshot.googleplex.com/5bhyuaaPXYQLcmq

- I have observed concurrent access to different uid from different tid (meaning two different threads accessing different Set<ChangeReport> concurrently)
- I have NOT observed concurrent access to same uid from different tid.

So there is no probably no need to optimized for concurrent access to `Set<ChangeReport>` and we can use `SynchronizedSet`.

Fix: 336364201
Test: presubmit
Flag: NONE
Change-Id: I1a0524302a58f948c51eef318ba35c4e907d855d
parent 39af13d2

core/java/com/android/internal/compat/ChangeReporter.java

+60 −42
Original line number Diff line number Diff line
@@ -18,22 +18,24 @@ package com.android.internal.compat; 


import static android.text.TextUtils.formatSimple;



import static java.util.Collections.EMPTY_SET;



import android.annotation.IntDef;

import android.util.Log;

import android.util.Slog;



import com.android.internal.annotations.GuardedBy;

import com.android.internal.annotations.VisibleForTesting;

import com.android.internal.compat.flags.Flags;

import com.android.internal.util.FrameworkStatsLog;



import java.lang.annotation.Retention;

import java.lang.annotation.RetentionPolicy;

import java.util.HashMap;

import java.util.Collections;

import java.util.HashSet;

import java.util.Map;

import java.util.Objects;

import java.util.Set;

import java.util.concurrent.ConcurrentHashMap;

import java.util.function.Function;



/**

 * A helper class to report changes to stats log.
@@ -42,6 +44,8 @@ import java.util.Set; 
 */

public final class ChangeReporter {

    private static final String TAG = "CompatChangeReporter";

    private static final Function<Integer, Set<ChangeReport>> NEW_CHANGE_REPORT_SET =

            uid -> Collections.synchronizedSet(new HashSet<>());

    private int mSource;



    private static final class ChangeReport {
@@ -69,15 +73,14 @@ public final class ChangeReporter { 
    }



    // Maps uid to a set of ChangeReports (that were reported for that uid).

    @GuardedBy("mReportedChanges")

    private final Map<Integer, Set<ChangeReport>> mReportedChanges;

    private final ConcurrentHashMap<Integer, Set<ChangeReport>> mReportedChanges;



    // When true will of every time to debug (logcat).

    private boolean mDebugLogAll;



    public ChangeReporter(@Source int source) {

        mSource = source;

        mReportedChanges =  new HashMap<>();

        mReportedChanges =  new ConcurrentHashMap<>();

        mDebugLogAll = false;

    }
@@ -93,14 +96,15 @@ public final class ChangeReporter { 
     *                        actually log. If the sdk version does not matter, should be true.

     */

    public void reportChange(int uid, long changeId, int state, boolean isLoggableBySdk) {

        if (shouldWriteToStatsLog(uid, changeId, state)) {

        boolean isAlreadyReported =

                checkAndSetIsAlreadyReported(uid, new ChangeReport(changeId, state));

        if (!isAlreadyReported) {

            FrameworkStatsLog.write(FrameworkStatsLog.APP_COMPATIBILITY_CHANGE_REPORTED, uid,

                    changeId, state, mSource);

        }

        if (shouldWriteToDebug(uid, changeId, state, isLoggableBySdk)) {

        if (shouldWriteToDebug(isAlreadyReported, state, isLoggableBySdk)) {

            debugLog(uid, changeId, state);

        }

        markAsReported(uid, new ChangeReport(changeId, state));

    }



    /**
@@ -129,7 +133,6 @@ public final class ChangeReporter { 
        mDebugLogAll = false;

    }





    /**

     * Returns whether the next report should be logged to FrameworkStatsLog.

     *
@@ -139,28 +142,26 @@ public final class ChangeReporter { 
     * @return true if the report should be logged

     */

    @VisibleForTesting

    public boolean shouldWriteToStatsLog(int uid, long changeId, int state) {

    boolean shouldWriteToStatsLog(int uid, long changeId, int state) {

        return !isAlreadyReported(uid, new ChangeReport(changeId, state));

    }



    /**

     * Returns whether the next report should be logged to logcat.

     *

     * @param uid             affected by the change

     * @param changeId        the reported change id

     * @param isAlreadyReported is the change already reported

     * @param state             of the reported change - enabled/disabled/only logged

     * @param isLoggableBySdk whether debug logging is allowed for this change based on target

     *                        SDK version. This is combined with other logic to determine whether to

     * @param isLoggableBySdk   whether debug logging is allowed for this change based on target SDK

     *                          version. This is combined with other logic to determine whether to

     *                          actually log. If the sdk version does not matter, should be true.

     * @return true if the report should be logged

     */

    @VisibleForTesting

    public boolean shouldWriteToDebug(

            int uid, long changeId, int state, boolean isLoggableBySdk) {

    private boolean shouldWriteToDebug(

            boolean isAlreadyReported, int state, boolean isLoggableBySdk) {

        // If log all bit is on, always return true.

        if (mDebugLogAll) return true;

        // If the change has already been reported, do not write.

        if (isAlreadyReported(uid, new ChangeReport(changeId, state))) return false;

        if (isAlreadyReported) return false;



        // If the flag is turned off or the TAG's logging is forced to debug level with

        // `adb setprop log.tag.CompatChangeReporter=DEBUG`, write to debug since the above checks
@@ -178,33 +179,53 @@ public final class ChangeReporter { 
     * @param uid         affected by the change

     * @param changeId    the reported change id

     * @param state       of the reported change - enabled/disabled/only logged

     *

     * @return true if the report should be logged

     */

    @VisibleForTesting

    public boolean shouldWriteToDebug(int uid, long changeId, int state) {

    boolean shouldWriteToDebug(int uid, long changeId, int state) {

        return shouldWriteToDebug(uid, changeId, state, true);

    }



    private boolean isAlreadyReported(int uid, ChangeReport report) {

        synchronized (mReportedChanges) {

            Set<ChangeReport> reportedChangesForUid = mReportedChanges.get(uid);

            if (reportedChangesForUid == null) {

                return false;

            } else {

                return reportedChangesForUid.contains(report);

    /**

     * Returns whether the next report should be logged to logcat.

     *

     * @param uid               affected by the change

     * @param changeId          the reported change id

     * @param state             of the reported change - enabled/disabled/only logged

     * @param isLoggableBySdk   whether debug logging is allowed for this change based on target SDK

     *                          version. This is combined with other logic to determine whether to

     *                          actually log. If the sdk version does not matter, should be true.

     * @return true if the report should be logged

     */

    @VisibleForTesting

    boolean shouldWriteToDebug(int uid, long changeId, int state, boolean isLoggableBySdk) {

        return shouldWriteToDebug(

                isAlreadyReported(uid, new ChangeReport(changeId, state)), state, isLoggableBySdk);

    }



    /**

     * Return if change has been reported. Also mark change as reported if not.

     *

     * @param uid affected by the change

     * @param changeReport change reported to be checked and marked as reported.

     *

     * @return true if change has been reported, and vice versa.

     */

    private boolean checkAndSetIsAlreadyReported(int uid, ChangeReport changeReport) {

        boolean isAlreadyReported = isAlreadyReported(uid, changeReport);

        if (!isAlreadyReported) {

            markAsReported(uid, changeReport);

        }

        return isAlreadyReported;

    }



    private boolean isAlreadyReported(int uid, ChangeReport report) {

        return mReportedChanges.getOrDefault(uid, EMPTY_SET).contains(report);

    }



    private void markAsReported(int uid, ChangeReport report) {

        synchronized (mReportedChanges) {

            Set<ChangeReport> reportedChangesForUid = mReportedChanges.get(uid);

            if (reportedChangesForUid == null) {

                mReportedChanges.put(uid, new HashSet<ChangeReport>());

                reportedChangesForUid = mReportedChanges.get(uid);

            }

            reportedChangesForUid.add(report);

        }

        mReportedChanges.computeIfAbsent(uid, NEW_CHANGE_REPORT_SET).add(report);

    }



    /**
@@ -216,10 +237,8 @@ public final class ChangeReporter { 
     * @param uid to reset

     */

    public void resetReportedChanges(int uid) {

        synchronized (mReportedChanges) {

        mReportedChanges.remove(uid);

    }

    }



    private void debugLog(int uid, long changeId, int state) {

        String message = formatSimple("Compat change id reported: %d; UID %d; state: %s", changeId,
@@ -229,7 +248,6 @@ public final class ChangeReporter { 
        } else {

            Log.d(TAG, message);

        }



    }



    /**