Rewrite BatteryStatsHistoryIterator logic for clarity and correctness (7b9cf4cf) · Commits · e / os / android_frameworks_base

core/java/com/android/internal/os/BatteryStatsHistory.java

+97 −93

Original line number	Original line	Diff line number	Diff line
	@@ -45,11 +45,13 @@ import com.android.internal.annotations.GuardedBy;
	import com.android.internal.annotations.VisibleForTesting;		import com.android.internal.annotations.VisibleForTesting;

	import java.io.PrintWriter;		import java.io.PrintWriter;
			import java.util.ArrayDeque;
	import java.util.ArrayList;		import java.util.ArrayList;
	import java.util.ConcurrentModificationException;		import java.util.ConcurrentModificationException;
	import java.util.HashMap;		import java.util.HashMap;
	import java.util.List;		import java.util.List;
	import java.util.Map;		import java.util.Map;
			import java.util.Queue;
	import java.util.concurrent.locks.ReentrantLock;		import java.util.concurrent.locks.ReentrantLock;

	/**		/**
	@@ -202,15 +204,6 @@ public class BatteryStatsHistory {
	@Nullable		@Nullable
	BatteryHistoryFragment getLatestFragment();		BatteryHistoryFragment getLatestFragment();

	/**
	* Given a fragment, returns the earliest fragment that follows it whose monotonic
	* start time falls within the specified range. `startTimeMs` is inclusive, `endTimeMs`
	* is exclusive.
	*/
	@Nullable
	BatteryHistoryFragment getNextFragment(BatteryHistoryFragment current, long startTimeMs,
	long endTimeMs);

	/**		/**
	* Acquires a lock on the entire store.		* Acquires a lock on the entire store.
	*/		*/
	@@ -268,6 +261,60 @@ public class BatteryStatsHistory {
	void reset();		void reset();
	}		}

			class BatteryHistoryParcelContainer {
			private boolean mParcelReadyForReading;
			private Parcel mParcel;
			private BatteryStatsHistory.BatteryHistoryFragment mFragment;
			private long mMonotonicStartTime;

			BatteryHistoryParcelContainer(@NonNull Parcel parcel, long monotonicStartTime) {
			mParcel = parcel;
			mMonotonicStartTime = monotonicStartTime;
			mParcelReadyForReading = true;
			}

			BatteryHistoryParcelContainer(@NonNull BatteryHistoryFragment fragment) {
			mFragment = fragment;
			mMonotonicStartTime = fragment.monotonicTimeMs;
			mParcelReadyForReading = false;
			}

			@Nullable
			Parcel getParcel() {
			if (mParcelReadyForReading) {
			return mParcel;
			}

			Parcel parcel = Parcel.obtain();
			if (readFragmentToParcel(parcel, mFragment)) {
			parcel.readInt(); // skip buffer size
			mParcel = parcel;
			} else {
			parcel.recycle();
			}
			mParcelReadyForReading = true;
			return mParcel;
			}

			long getMonotonicStartTime() {
			return mMonotonicStartTime;
			}

			/**
			* Recycles the parcel (if appropriate). Should be called after the parcel has been
			* processed by the iterator.
			*/
			void close() {
			if (mParcel != null && mFragment != null) {
			mParcel.recycle();
			}
			// ParcelContainers are not meant to be reusable. To prevent any unintentional
			// access to the parcel after it has been recycled, clear the references to it.
			mParcel = null;
			mFragment = null;
			}
			}

	private final Parcel mHistoryBuffer;		private final Parcel mHistoryBuffer;
	private final HistoryStepDetailsCalculator mStepDetailsCalculator;		private final HistoryStepDetailsCalculator mStepDetailsCalculator;
	private final Clock mClock;		private final Clock mClock;
	@@ -447,6 +494,7 @@ public class BatteryStatsHistory {
	mWritableHistory = writableHistory;		mWritableHistory = writableHistory;
	if (mWritableHistory != null) {		if (mWritableHistory != null) {
	mMutable = false;		mMutable = false;
			mHistoryBufferStartTime = mWritableHistory.mHistoryBufferStartTime;
	mHistoryMonotonicEndTime = mWritableHistory.mHistoryMonotonicEndTime;		mHistoryMonotonicEndTime = mWritableHistory.mHistoryMonotonicEndTime;
	}		}

	@@ -689,95 +737,66 @@ public class BatteryStatsHistory {
	}		}

	/**		/**
	* When iterating history files and history buffer, always start from the lowest numbered		* Returns all chunks of accumulated history that contain items within the range between
	* history file, when reached the mActiveFile (highest numbered history file), do not read from		* `startTimeMs` (inclusive) and `endTimeMs` (exclusive)
	* mActiveFile, read from history buffer instead because the buffer has more updated data.
	*
	* @return The parcel that has next record. null if finished all history files and history
	* buffer
	*/		*/
	@Nullable		Queue<BatteryHistoryParcelContainer> getParcelContainers(long startTimeMs, long endTimeMs) {
	public Parcel getNextParcel(long startTimeMs, long endTimeMs) {		if (mMutable) {
	checkImmutable();		throw new IllegalStateException("Iterating over a mutable battery history");

	// First iterate through all records in current parcel.
	if (mCurrentParcel != null) {
	if (mCurrentParcel.dataPosition() < mCurrentParcelEnd) {
	// There are more records in current parcel.
	return mCurrentParcel;
	} else if (mHistoryBuffer == mCurrentParcel) {
	// finished iterate through all history files and history buffer.
	return null;
	} else if (mHistoryParcels == null
	\|\| !mHistoryParcels.contains(mCurrentParcel)) {
	// current parcel is from history file.
	mCurrentParcel.recycle();
	}		}

			if (endTimeMs == MonotonicClock.UNDEFINED \|\| endTimeMs == 0) {
			endTimeMs = Long.MAX_VALUE;
	}		}

			Queue<BatteryHistoryParcelContainer> containers = new ArrayDeque<>();

	if (mStore != null) {		if (mStore != null) {
	BatteryHistoryFragment next = mStore.getNextFragment(mCurrentFragment, startTimeMs,		List<BatteryHistoryFragment> fragments = mStore.getFragments();
	endTimeMs);		for (int i = 0; i < fragments.size(); i++) {
	while (next != null) {		BatteryHistoryFragment fragment = fragments.get(i);
	mCurrentParcel = null;		if (fragment.monotonicTimeMs >= endTimeMs) {
	mCurrentParcelEnd = 0;		break;
	final Parcel p = Parcel.obtain();
	if (readFragmentToParcel(p, next)) {
	int bufSize = p.readInt();
	int curPos = p.dataPosition();
	mCurrentParcelEnd = curPos + bufSize;
	mCurrentParcel = p;
	if (curPos < mCurrentParcelEnd) {
	mCurrentFragment = next;
	return mCurrentParcel;
	}		}
	} else {
	p.recycle();		if (fragment.monotonicTimeMs >= startTimeMs && fragment != mActiveFragment) {
			containers.add(new BatteryHistoryParcelContainer(fragment));
	}		}
	next = mStore.getNextFragment(next, startTimeMs, endTimeMs);
	}		}
	}		}

	// mHistoryParcels is created when BatteryStatsImpl object is created from deserialization
	// of a parcel, such as Settings app or checkin file.
	if (mHistoryParcels != null) {		if (mHistoryParcels != null) {
	while (mParcelIndex < mHistoryParcels.size()) {		for (int i = 0; i < mHistoryParcels.size(); i++) {
	final Parcel p = mHistoryParcels.get(mParcelIndex++);		final Parcel p = mHistoryParcels.get(i);
	if (!verifyVersion(p)) {		if (!verifyVersion(p)) {
	continue;		continue;
	}		}
	// skip monotonic time field.
	p.readLong();
	// skip monotonic end time field
	p.readLong();
	// skip monotonic size field
	p.readLong();

	final int bufSize = p.readInt();		long monotonicStartTime = p.readLong();
	final int curPos = p.dataPosition();		if (monotonicStartTime >= endTimeMs) {
	mCurrentParcelEnd = curPos + bufSize;		continue;
	mCurrentParcel = p;
	if (curPos < mCurrentParcelEnd) {
	return mCurrentParcel;
	}
	}		}

			long monotonicEndTime = p.readLong();
			if (monotonicEndTime < startTimeMs) {
			continue;
	}		}

	// finished iterator through history files (except the last one), now history buffer.		// skip monotonic size field
	if (mHistoryBuffer.dataSize() <= 0) {		p.readLong();
	// buffer is empty.		// skip buffer size field
	return null;		p.readInt();

			containers.add(new BatteryHistoryParcelContainer(p, monotonicStartTime));
	}		}
	mHistoryBuffer.setDataPosition(0);
	mCurrentParcel = mHistoryBuffer;
	mCurrentParcelEnd = mCurrentParcel.dataSize();
	return mCurrentParcel;
	}		}

	private void checkImmutable() {		if (mHistoryBufferStartTime < endTimeMs) {
	if (mMutable) {		mHistoryBuffer.setDataPosition(0);
	throw new IllegalStateException("Iterating over a mutable battery history");		containers.add(
			new BatteryHistoryParcelContainer(mHistoryBuffer, mHistoryBufferStartTime));
	}		}
			return containers;
	}		}

	/**		/**
	@@ -817,21 +836,6 @@ public class BatteryStatsHistory {
	return version == VERSION;		return version == VERSION;
	}		}

	/**
	* Extracts the monotonic time, as per {@link MonotonicClock}, from the supplied battery history
	* buffer.
	*/
	public long getHistoryBufferStartTime(Parcel p) {
	int pos = p.dataPosition();
	p.setDataPosition(0);
	p.readInt(); // Skip the version field
	long monotonicTime = p.readLong();
	p.readLong(); // Skip monotonic end time field
	p.readLong(); // Skip monotonic size field
	p.setDataPosition(pos);
	return monotonicTime;
	}

	/**		/**
	* Writes the battery history contents for persistence.		* Writes the battery history contents for persistence.
	*/		*/

core/java/com/android/internal/os/BatteryStatsHistoryIterator.java

+49 −25

Original line number	Original line	Diff line number	Diff line
	@@ -24,6 +24,7 @@ import android.util.Slog;
	import android.util.SparseArray;		import android.util.SparseArray;

	import java.util.Iterator;		import java.util.Iterator;
			import java.util.Queue;

	/**		/**
	* An iterator for {@link BatteryStats.HistoryItem}'s.		* An iterator for {@link BatteryStats.HistoryItem}'s.
	@@ -48,7 +49,10 @@ public class BatteryStatsHistoryIterator implements Iterator<BatteryStats.Histor
	private long mBaseMonotonicTime;		private long mBaseMonotonicTime;
	private long mBaseTimeUtc;		private long mBaseTimeUtc;
	private int mItemIndex = 0;		private int mItemIndex = 0;
	private int mMaxHistoryItems;		private final int mMaxHistoryItems;
			private int mParcelDataPosition;

			private Queue<BatteryStatsHistory.BatteryHistoryParcelContainer> mParcelContainers;

	public BatteryStatsHistoryIterator(@NonNull BatteryStatsHistory history, long startTimeMs,		public BatteryStatsHistoryIterator(@NonNull BatteryStatsHistory history, long startTimeMs,
	long endTimeMs) {		long endTimeMs) {
	@@ -62,7 +66,11 @@ public class BatteryStatsHistoryIterator implements Iterator<BatteryStats.Histor
	@Override		@Override
	public boolean hasNext() {		public boolean hasNext() {
	if (!mNextItemReady) {		if (!mNextItemReady) {
	advance();		if (!advance()) {
			mHistoryItem = null;
			close();
			}
			mNextItemReady = true;
	}		}

	return mHistoryItem != null;		return mHistoryItem != null;
	@@ -75,35 +83,48 @@ public class BatteryStatsHistoryIterator implements Iterator<BatteryStats.Histor
	@Override		@Override
	public BatteryStats.HistoryItem next() {		public BatteryStats.HistoryItem next() {
	if (!mNextItemReady) {		if (!mNextItemReady) {
	advance();		if (!advance()) {
			mHistoryItem = null;
			close();
			}
	}		}
	mNextItemReady = false;		mNextItemReady = false;
	return mHistoryItem;		return mHistoryItem;
	}		}

	private void advance() {		private boolean advance() {
	while (true) {		if (mParcelContainers == null) {
	if (mItemIndex > mMaxHistoryItems) {		mParcelContainers = mBatteryStatsHistory.getParcelContainers(mStartTimeMs, mEndTimeMs);
	Slog.wtfStack(TAG, "Number of battery history items is too large: " + mItemIndex);
	break;
	}		}

	Parcel p = mBatteryStatsHistory.getNextParcel(mStartTimeMs, mEndTimeMs);		BatteryStatsHistory.BatteryHistoryParcelContainer container;
	if (p == null) {		while ((container = mParcelContainers.peek()) != null) {
	break;		Parcel p = container.getParcel();
			if (p == null \|\| p.dataPosition() >= p.dataSize()) {
			container.close();
			mParcelContainers.remove();
			mParcelDataPosition = 0;
			continue;
	}		}

	if (!mTimeInitialized) {		if (!mTimeInitialized) {
	mBaseMonotonicTime = mBatteryStatsHistory.getHistoryBufferStartTime(p);		mBaseMonotonicTime = container.getMonotonicStartTime();
	mHistoryItem.time = mBaseMonotonicTime;		mHistoryItem.time = mBaseMonotonicTime;
	mTimeInitialized = true;		mTimeInitialized = true;
	}		}

	try {		try {
	readHistoryDelta(p, mHistoryItem);		readHistoryDelta(p, mHistoryItem);
			int dataPosition = p.dataPosition();
			if (dataPosition <= mParcelDataPosition) {
			Slog.wtf(TAG, "Corrupted battery history, parcel is not progressing: "
			+ dataPosition + " of " + p.dataSize());
			return false;
			}
			mParcelDataPosition = dataPosition;
	} catch (Throwable t) {		} catch (Throwable t) {
	Slog.wtf(TAG, "Corrupted battery history", t);		Slog.wtf(TAG, "Corrupted battery history", t);
	break;		return false;
	}		}

	if (mHistoryItem.cmd == BatteryStats.HistoryItem.CMD_CURRENT_TIME		if (mHistoryItem.cmd == BatteryStats.HistoryItem.CMD_CURRENT_TIME
	@@ -111,21 +132,24 @@ public class BatteryStatsHistoryIterator implements Iterator<BatteryStats.Histor
	mBaseTimeUtc = mHistoryItem.currentTime - (mHistoryItem.time - mBaseMonotonicTime);		mBaseTimeUtc = mHistoryItem.currentTime - (mHistoryItem.time - mBaseMonotonicTime);
	}		}

	mHistoryItem.currentTime = mBaseTimeUtc + (mHistoryItem.time - mBaseMonotonicTime);		if (mHistoryItem.time < mStartTimeMs) {
			continue;
	if (mEndTimeMs != 0 && mHistoryItem.time >= mEndTimeMs) {
	break;
	}		}
	if (mHistoryItem.time >= mStartTimeMs) {
	mItemIndex++;		if (mEndTimeMs != 0 && mEndTimeMs != MonotonicClock.UNDEFINED
	mNextItemReady = true;		&& mHistoryItem.time >= mEndTimeMs) {
	return;		return false;
	}		}

			if (mItemIndex++ > mMaxHistoryItems) {
			Slog.wtfStack(TAG, "Number of battery history items is too large: " + mItemIndex);
			return false;
	}		}

	mHistoryItem = null;		mHistoryItem.currentTime = mBaseTimeUtc + (mHistoryItem.time - mBaseMonotonicTime);
	mNextItemReady = true;		return true;
	close();		}
			return false;
	}		}

	private void readHistoryDelta(Parcel src, BatteryStats.HistoryItem cur) {		private void readHistoryDelta(Parcel src, BatteryStats.HistoryItem cur) {

services/core/java/com/android/server/power/stats/BatteryHistoryDirectory.java

+4 −38

Original line number	Original line	Diff line number	Diff line
	@@ -374,6 +374,10 @@ public class BatteryHistoryDirectory implements BatteryStatsHistory.BatteryHisto
	@SuppressWarnings("unchecked")		@SuppressWarnings("unchecked")
	@Override		@Override
	public List<BatteryHistoryFragment> getFragments() {		public List<BatteryHistoryFragment> getFragments() {
			if (!mLock.isHeldByCurrentThread()) {
			throw new IllegalStateException("Reading battery history without a lock");
			}

	ensureInitialized();		ensureInitialized();
	return (List<BatteryHistoryFragment>)		return (List<BatteryHistoryFragment>)
	(List<? extends BatteryHistoryFragment>) mHistoryFiles;		(List<? extends BatteryHistoryFragment>) mHistoryFiles;
	@@ -442,44 +446,6 @@ public class BatteryHistoryDirectory implements BatteryStatsHistory.BatteryHisto
	return file;		return file;
	}		}

	@Override
	public BatteryHistoryFragment getNextFragment(BatteryHistoryFragment current, long startTimeMs,
	long endTimeMs) {
	ensureInitialized();

	if (!mLock.isHeldByCurrentThread()) {
	throw new IllegalStateException("Iterating battery history without a lock");
	}

	int nextFileIndex = 0;
	int firstFileIndex = 0;
	// skip the last file because its data is in history buffer.
	int lastFileIndex = mHistoryFiles.size() - 2;
	for (int i = lastFileIndex; i >= 0; i--) {
	BatteryHistoryFragment fragment = mHistoryFiles.get(i);
	if (current != null && fragment.monotonicTimeMs == current.monotonicTimeMs) {
	nextFileIndex = i + 1;
	}
	if (fragment.monotonicTimeMs > endTimeMs) {
	lastFileIndex = i - 1;
	}
	if (fragment.monotonicTimeMs <= startTimeMs) {
	firstFileIndex = i;
	break;
	}
	}

	if (nextFileIndex < firstFileIndex) {
	nextFileIndex = firstFileIndex;
	}

	if (nextFileIndex <= lastFileIndex) {
	return mHistoryFiles.get(nextFileIndex);
	}

	return null;
	}

	@Override		@Override
	public boolean hasCompletedFragments() {		public boolean hasCompletedFragments() {
	ensureInitialized();		ensureInitialized();

services/tests/powerstatstests/src/com/android/server/power/stats/BatteryStatsHistoryTest.java

+30 −42

Original line number	Original line	Diff line number	Diff line
	@@ -22,6 +22,7 @@ import static com.google.common.truth.Truth.assertWithMessage;
	import static org.junit.Assert.assertEquals;		import static org.junit.Assert.assertEquals;
	import static org.junit.Assert.assertFalse;		import static org.junit.Assert.assertFalse;
	import static org.junit.Assert.assertTrue;		import static org.junit.Assert.assertTrue;
			import static org.junit.Assert.fail;
	import static org.mockito.ArgumentMatchers.any;		import static org.mockito.ArgumentMatchers.any;
	import static org.mockito.Mockito.doAnswer;		import static org.mockito.Mockito.doAnswer;
	import static org.mockito.Mockito.spy;		import static org.mockito.Mockito.spy;
	@@ -330,31 +331,24 @@ public class BatteryStatsHistoryTest {
	return invocation.callRealMethod();		return invocation.callRealMethod();
	}).when(mHistory).readFragmentToParcel(any(), any());		}).when(mHistory).readFragmentToParcel(any(), any());

	// Prepare history for iteration		int eventsRead = 0;
	mHistory.iterate(0, MonotonicClock.UNDEFINED);		BatteryStatsHistoryIterator iterator = mHistory.iterate(0, MonotonicClock.UNDEFINED);
			while (iterator.hasNext()) {
	Parcel parcel = mHistory.getNextParcel(0, Long.MAX_VALUE);		HistoryItem item = iterator.next();
	assertThat(parcel).isNotNull();		if (item.eventCode == HistoryItem.EVENT_JOB_START) {
			eventsRead++;
	assertThat(mReadFiles).containsExactly("123.bh");		assertThat(mReadFiles).containsExactly("123.bh");
			} else if (item.eventCode == HistoryItem.EVENT_JOB_FINISH) {
			eventsRead++;
			assertThat(mReadFiles).containsExactly("123.bh", "1000.bh");
			} else if (item.eventCode == HistoryItem.EVENT_ALARM) {
			eventsRead++;
			assertThat(mReadFiles).containsExactly("123.bh", "1000.bh", "2000.bh");
			}
			}

	// Skip to the end to force reading the next parcel		assertThat(eventsRead).isEqualTo(3);
	parcel.setDataPosition(parcel.dataSize());		assertThat(mReadFiles).containsExactly("123.bh", "1000.bh", "2000.bh", "3000.bh");
	mReadFiles.clear();
	parcel = mHistory.getNextParcel(0, Long.MAX_VALUE);
	assertThat(parcel).isNotNull();
	assertThat(mReadFiles).containsExactly("1000.bh");

	parcel.setDataPosition(parcel.dataSize());
	mReadFiles.clear();
	parcel = mHistory.getNextParcel(0, Long.MAX_VALUE);
	assertThat(parcel).isNotNull();
	assertThat(mReadFiles).containsExactly("2000.bh");

	parcel.setDataPosition(parcel.dataSize());
	mReadFiles.clear();
	parcel = mHistory.getNextParcel(0, Long.MAX_VALUE);
	assertThat(parcel).isNull();
	assertThat(mReadFiles).isEmpty();
	}		}

	@Test		@Test
	@@ -372,25 +366,19 @@ public class BatteryStatsHistoryTest {
	return invocation.callRealMethod();		return invocation.callRealMethod();
	}).when(mHistory).readFragmentToParcel(any(), any());		}).when(mHistory).readFragmentToParcel(any(), any());

	// Prepare history for iteration		BatteryStatsHistoryIterator iterator = mHistory.iterate(1000, 3000);
	mHistory.iterate(1000, 3000);		while (iterator.hasNext()) {
			HistoryItem item = iterator.next();
	Parcel parcel = mHistory.getNextParcel(1000, 3000);		if (item.eventCode == HistoryItem.EVENT_JOB_START) {
	assertThat(parcel).isNotNull();		fail("Event outside the range");
			} else if (item.eventCode == HistoryItem.EVENT_JOB_FINISH) {
	assertThat(mReadFiles).containsExactly("1000.bh");		assertThat(mReadFiles).containsExactly("1000.bh");
			} else if (item.eventCode == HistoryItem.EVENT_ALARM) {
			fail("Event outside the range");
			}
			}

	// Skip to the end to force reading the next parcel		assertThat(mReadFiles).containsExactly("1000.bh", "2000.bh");
	parcel.setDataPosition(parcel.dataSize());
	mReadFiles.clear();
	parcel = mHistory.getNextParcel(1000, 3000);
	assertThat(parcel).isNotNull();
	assertThat(mReadFiles).containsExactly("2000.bh");

	parcel.setDataPosition(parcel.dataSize());
	mReadFiles.clear();
	parcel = mHistory.getNextParcel(1000, 3000);
	assertThat(parcel).isNull();
	assertThat(mReadFiles).isEmpty();
	}		}

	private void prepareMultiFileHistory() {		private void prepareMultiFileHistory() {