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

Commit 553e8688 authored by Michael Wachenschwanz's avatar Michael Wachenschwanz
Browse files

Simplify OomAdjusterModernImpl traversal 

The current implementation of OomAdjusterModernImpl requires every
procState and oomAdj change to be reported to the ProcessRecordNodes
data structure. If ever there is a mismatch between a ProcessStateRecord
and the slot that it occupies, moving the node risks moving the LAST
reference of a slot to the wrong slot (which will cause a NPE during
traversal).

To avoid this fragile behavior, the ProcessRecordNodes struct is now
reworked to only keep track of what processes need to computed in the
current update (instead of keeping track of all processes). This allows
for ProcessRecords to be moved slots without handling the slot it was
previously in.

Flag: com.android.server.am.simplify_process_traversal
Fixes: 336178916
Test: atest MockingOomAdjusterTests
Change-Id: Ibf2a15fa3141ec1bcb9fe7548fbf977ce6f76e7a
parent 40349d86

services/core/java/com/android/server/am/OomAdjusterModernImpl.java

+182 −29
Original line number Diff line number Diff line
@@ -84,6 +84,7 @@ import java.util.ArrayList; 
import java.util.Arrays;

import java.util.function.BiConsumer;

import java.util.function.Consumer;

import java.util.function.ToIntFunction;



/**

 * A modern implementation of the oom adjuster.
@@ -271,11 +272,31 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
        // The last node besides the tail.

        private final ProcessRecordNode[] mLastNode;



        private final ToIntFunction<ProcessRecord> mSlotFunction;

        // Cache of the most important slot with a node in it.

        private int mFirstPopulatedSlot = 0;



        ProcessRecordNodes(@ProcessRecordNode.NodeType int type, int size) {

            mType = type;

            final ToIntFunction<ProcessRecord> valueFunction;

            switch (mType) {

                case ProcessRecordNode.NODE_TYPE_PROC_STATE:

                    valueFunction = (proc) -> proc.mState.getCurProcState();

                    mSlotFunction = (proc) -> processStateToSlot(proc.mState.getCurProcState());

                    break;

                case ProcessRecordNode.NODE_TYPE_ADJ:

                    valueFunction = (proc) -> proc.mState.getCurRawAdj();

                    mSlotFunction = (proc) -> adjToSlot(proc.mState.getCurRawAdj());

                    break;

                default:

                    valueFunction = (proc) -> 0;

                    mSlotFunction = (proc) -> 0;

                    break;

            }



            mProcessRecordNodes = new LinkedProcessRecordList[size];

            for (int i = 0; i < size; i++) {

                mProcessRecordNodes[i] = new LinkedProcessRecordList(type);

                mProcessRecordNodes[i] = new LinkedProcessRecordList(valueFunction);

            }

            mLastNode = new ProcessRecordNode[size];

            resetLastNodes();
@@ -294,6 +315,11 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
        }



        void resetLastNodes() {

            if (Flags.simplifyProcessTraversal()) {

                // Last nodes are no longer used. Just reset instead.

                reset();

                return;

            }

            for (int i = 0; i < mProcessRecordNodes.length; i++) {

                mLastNode[i] = mProcessRecordNodes[i].getLastNodeBeforeTail();

            }
@@ -308,6 +334,36 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
            final ProcessRecordNode tail = mProcessRecordNodes[slot].TAIL;

            while (node != tail) {

                mTmpOomAdjusterArgs.mApp = node.mApp;

                if (node.mApp == null) {

                    // TODO(b/336178916) - Temporary logging for root causing b/336178916.

                    StringBuilder sb = new StringBuilder();

                    sb.append("Iterating null process during OomAdjuster traversal!!!\n");

                    sb.append("Type:");

                    switch (mType) {

                        case ProcessRecordNode.NODE_TYPE_PROC_STATE -> sb.append(

                                "NODE_TYPE_PROC_STATE");

                        case ProcessRecordNode.NODE_TYPE_ADJ -> sb.append("NODE_TYPE_ADJ");

                        default -> sb.append("UNKNOWN");

                    }

                    sb.append(", Slot:");

                    sb.append(slot);

                    sb.append("\nLAST:");

                    ProcessRecordNode last = mLastNode[slot];

                    if (last.mApp == null) {

                        sb.append("null");

                    } else {

                        sb.append(last);

                        sb.append("\nSetProcState:");

                        sb.append(last.mApp.getSetProcState());

                        sb.append(", CurProcState:");

                        sb.append(last.mApp.mState.getCurProcState());

                        sb.append(", SetAdj:");

                        sb.append(last.mApp.getSetAdj());

                        sb.append(", CurRawAdj:");

                        sb.append(last.mApp.mState.getCurRawAdj());

                    }

                    Slog.wtfStack(TAG, sb.toString());

                }

                // Save the next before calling callback, since that may change the node.mNext.

                final ProcessRecordNode next = node.mNext;

                callback.accept(mTmpOomAdjusterArgs);
@@ -325,6 +381,33 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
            }

        }



        ProcessRecord poll() {

            ProcessRecordNode node = null;

            final int size = mProcessRecordNodes.length;

            // Find the next node.

            while (node == null && mFirstPopulatedSlot < size) {

                node = mProcessRecordNodes[mFirstPopulatedSlot].poll();

                if (node == null) {

                    // This slot is now empty, move on to the next.

                    mFirstPopulatedSlot++;

                }

            }

            if (node == null) return null;

            return node.mApp;

        }



        void offer(ProcessRecord proc) {

            ProcessRecordNode node = proc.mLinkedNodes[mType];

            // Find which slot to add the node to.

            final int newSlot = mSlotFunction.applyAsInt(proc);

            if (newSlot < mFirstPopulatedSlot) {

                // node is being added to a more important slot.

                mFirstPopulatedSlot = newSlot;

            }

            node.unlink();

            mProcessRecordNodes[newSlot].offer(node);

        }



        int getNumberOfSlots() {

            return mProcessRecordNodes.length;

        }
@@ -423,12 +506,35 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
            // Sentinel head/tail, to make bookkeeping work easier.

            final ProcessRecordNode HEAD = new ProcessRecordNode(null);

            final ProcessRecordNode TAIL = new ProcessRecordNode(null);

            final @ProcessRecordNode.NodeType int mNodeType;

            final ToIntFunction<ProcessRecord> mValueFunction;



            LinkedProcessRecordList(@ProcessRecordNode.NodeType int nodeType) {

            LinkedProcessRecordList(ToIntFunction<ProcessRecord> valueFunction) {

                HEAD.mNext = TAIL;

                TAIL.mPrev = HEAD;

                mNodeType = nodeType;

                mValueFunction = valueFunction;

            }



            ProcessRecordNode poll() {

                final ProcessRecordNode next = HEAD.mNext;

                if (next == TAIL) return null;

                next.unlink();

                return next;

            }



            void offer(@NonNull ProcessRecordNode node) {

                final int newValue = mValueFunction.applyAsInt(node.mApp);



                // Find the last node with less than or equal value to the new node.

                ProcessRecordNode curNode = TAIL.mPrev;

                while (curNode != HEAD && mValueFunction.applyAsInt(curNode.mApp) > newValue) {

                    curNode = curNode.mPrev;

                }



                // Insert the new node after the found node.

                node.mPrev = curNode;

                node.mNext = curNode.mNext;

                curNode.mNext.mPrev = node;

                curNode.mNext = node;

            }



            void append(@NonNull ProcessRecordNode node) {
@@ -727,6 +833,9 @@ public class OomAdjusterModernImpl extends OomAdjuster { 


    private void updateAdjSlotIfNecessary(ProcessRecord app, int prevRawAdj) {

        if (app.mState.getCurRawAdj() != prevRawAdj) {

            if (Flags.simplifyProcessTraversal()) {

                mProcessRecordAdjNodes.offer(app);

            } else {

                final int slot = adjToSlot(app.mState.getCurRawAdj());

                final int prevSlot = adjToSlot(prevRawAdj);

                if (slot != prevSlot && slot != ADJ_SLOT_INVALID) {
@@ -734,15 +843,23 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
                }

            }

        }

    }



    private void updateAdjSlot(ProcessRecord app, int prevRawAdj) {

        if (Flags.simplifyProcessTraversal()) {

            mProcessRecordAdjNodes.offer(app);

        } else {

            final int slot = adjToSlot(app.mState.getCurRawAdj());

            final int prevSlot = adjToSlot(prevRawAdj);

            mProcessRecordAdjNodes.moveAppTo(app, prevSlot, slot);

        }

    }



    private void updateProcStateSlotIfNecessary(ProcessRecord app, int prevProcState) {

        if (app.mState.getCurProcState() != prevProcState) {

            if (Flags.simplifyProcessTraversal()) {

                mProcessRecordProcStateNodes.offer(app);

            } else {

                final int slot = processStateToSlot(app.mState.getCurProcState());

                final int prevSlot = processStateToSlot(prevProcState);

                if (slot != prevSlot) {
@@ -750,12 +867,17 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
                }

            }

        }

    }



    private void updateProcStateSlot(ProcessRecord app, int prevProcState) {

        if (Flags.simplifyProcessTraversal()) {

            mProcessRecordProcStateNodes.offer(app);

        } else {

            final int slot = processStateToSlot(app.mState.getCurProcState());

            final int prevSlot = processStateToSlot(prevProcState);

            mProcessRecordProcStateNodes.moveAppTo(app, prevSlot, slot);

        }

    }



    @Override

    protected void performUpdateOomAdjLSP(@OomAdjReason int oomAdjReason) {
@@ -832,9 +954,16 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
            // Compute initial values, the procState and adj priority queues will be populated here.

            computeOomAdjLSP(app, UNKNOWN_ADJ, topApp, true, now, false, false, oomAdjReason,

                    false);



            if (Flags.simplifyProcessTraversal()) {

                // Just add to the procState priority queue. The adj priority queue should be

                // empty going into the traversal step.

                mProcessRecordProcStateNodes.offer(app);

            } else {

                updateProcStateSlot(app, prevProcState);

                updateAdjSlot(app, prevAdj);

            }

        }



        // Set adj last nodes now, this way a process will only be reevaluated during the adj node

        // iteration if they adj score changed during the procState node iteration.
@@ -851,6 +980,23 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
     */

    @GuardedBy({"mService", "mProcLock"})

    private void computeConnectionsLSP() {

        if (Flags.simplifyProcessTraversal()) {

            // 1st pass, iterate all nodes in order of procState importance.

            ProcessRecord proc = mProcessRecordProcStateNodes.poll();

            while (proc != null) {

                mTmpOomAdjusterArgs.mApp = proc;

                mComputeConnectionsConsumer.accept(mTmpOomAdjusterArgs);

                proc = mProcessRecordProcStateNodes.poll();

            }



            // 2st pass, iterate all nodes in order of procState importance.

            proc = mProcessRecordAdjNodes.poll();

            while (proc != null) {

                mTmpOomAdjusterArgs.mApp = proc;

                mComputeConnectionsConsumer.accept(mTmpOomAdjusterArgs);

                proc = mProcessRecordAdjNodes.poll();

            }

        } else {

            // 1st pass, scan each slot in the procstate node list.

            for (int i = 0, end = mProcessRecordProcStateNodes.size() - 1; i < end; i++) {

                mProcessRecordProcStateNodes.forEachNewNode(i, mComputeConnectionsConsumer);
@@ -861,6 +1007,7 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
                mProcessRecordAdjNodes.forEachNewNode(i, mComputeConnectionsConsumer);

            }

        }

    }



    /**

     * Perform a partial update on the target processes and their reachable processes.
@@ -987,10 +1134,16 @@ public class OomAdjusterModernImpl extends OomAdjuster { 
            args.mApp = reachable;

            computeOomAdjIgnoringReachablesLSP(args);



            if (Flags.simplifyProcessTraversal()) {

                // Just add to the procState priority queue. The adj priority queue should be

                // empty going into the traversal step.

                mProcessRecordProcStateNodes.offer(reachable);

            } else {

                updateProcStateSlot(reachable, prevProcState);

                updateAdjSlot(reachable, prevAdj);

            }

        }

    }



    /**

     * Calculate initial importance states for {@code app}.

services/core/java/com/android/server/am/flags.aconfig

+11 −0
Original line number Diff line number Diff line
@@ -123,3 +123,14 @@ flag { 
    description: "Avoid OomAdjuster calculations for connections that won't change importance"

    bug: "323376416"

}



flag {

    name: "simplify_process_traversal"

    namespace: "backstage_power"

    description: "Simplify the OomAdjuster's process traversal mechanism."

    bug: "336178916"

    is_fixed_read_only: true

    metadata {

        purpose: PURPOSE_BUGFIX

    }

}