diff options
4 files changed, 180 insertions, 198 deletions
diff --git a/core/java/com/android/internal/os/BatteryStatsHistory.java b/core/java/com/android/internal/os/BatteryStatsHistory.java index 81ca23173457..c6207f9451c2 100644 --- a/core/java/com/android/internal/os/BatteryStatsHistory.java +++ b/core/java/com/android/internal/os/BatteryStatsHistory.java @@ -45,11 +45,13 @@ import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; import java.io.PrintWriter; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.ConcurrentModificationException; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Queue; import java.util.concurrent.locks.ReentrantLock; /** @@ -203,15 +205,6 @@ public class BatteryStatsHistory { 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. */ void lock(); @@ -268,6 +261,60 @@ public class BatteryStatsHistory { 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 HistoryStepDetailsCalculator mStepDetailsCalculator; private final Clock mClock; @@ -447,6 +494,7 @@ public class BatteryStatsHistory { mWritableHistory = writableHistory; if (mWritableHistory != null) { mMutable = false; + mHistoryBufferStartTime = mWritableHistory.mHistoryBufferStartTime; mHistoryMonotonicEndTime = mWritableHistory.mHistoryMonotonicEndTime; } @@ -689,95 +737,66 @@ public class BatteryStatsHistory { } /** - * When iterating history files and history buffer, always start from the lowest numbered - * history file, when reached the mActiveFile (highest numbered history file), do not read from - * 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 + * Returns all chunks of accumulated history that contain items within the range between + * `startTimeMs` (inclusive) and `endTimeMs` (exclusive) */ - @Nullable - public Parcel getNextParcel(long startTimeMs, long endTimeMs) { - checkImmutable(); + Queue<BatteryHistoryParcelContainer> getParcelContainers(long startTimeMs, long endTimeMs) { + if (mMutable) { + 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) { - BatteryHistoryFragment next = mStore.getNextFragment(mCurrentFragment, startTimeMs, - endTimeMs); - while (next != null) { - mCurrentParcel = null; - mCurrentParcelEnd = 0; - 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(); + List<BatteryHistoryFragment> fragments = mStore.getFragments(); + for (int i = 0; i < fragments.size(); i++) { + BatteryHistoryFragment fragment = fragments.get(i); + if (fragment.monotonicTimeMs >= endTimeMs) { + break; + } + + 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) { - while (mParcelIndex < mHistoryParcels.size()) { - final Parcel p = mHistoryParcels.get(mParcelIndex++); + for (int i = 0; i < mHistoryParcels.size(); i++) { + final Parcel p = mHistoryParcels.get(i); if (!verifyVersion(p)) { continue; } - // skip monotonic time field. - p.readLong(); - // skip monotonic end time field - p.readLong(); + + long monotonicStartTime = p.readLong(); + if (monotonicStartTime >= endTimeMs) { + continue; + } + + long monotonicEndTime = p.readLong(); + if (monotonicEndTime < startTimeMs) { + continue; + } + // skip monotonic size field p.readLong(); + // skip buffer size field + p.readInt(); - final int bufSize = p.readInt(); - final int curPos = p.dataPosition(); - mCurrentParcelEnd = curPos + bufSize; - mCurrentParcel = p; - if (curPos < mCurrentParcelEnd) { - return mCurrentParcel; - } + containers.add(new BatteryHistoryParcelContainer(p, monotonicStartTime)); } } - // finished iterator through history files (except the last one), now history buffer. - if (mHistoryBuffer.dataSize() <= 0) { - // buffer is empty. - return null; - } - mHistoryBuffer.setDataPosition(0); - mCurrentParcel = mHistoryBuffer; - mCurrentParcelEnd = mCurrentParcel.dataSize(); - return mCurrentParcel; - } - - private void checkImmutable() { - if (mMutable) { - throw new IllegalStateException("Iterating over a mutable battery history"); + if (mHistoryBufferStartTime < endTimeMs) { + mHistoryBuffer.setDataPosition(0); + containers.add( + new BatteryHistoryParcelContainer(mHistoryBuffer, mHistoryBufferStartTime)); } + return containers; } /** @@ -818,21 +837,6 @@ public class BatteryStatsHistory { } /** - * 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. */ public void writeSummaryToParcel(Parcel out, boolean inclHistory) { diff --git a/core/java/com/android/internal/os/BatteryStatsHistoryIterator.java b/core/java/com/android/internal/os/BatteryStatsHistoryIterator.java index 38398b4bf6f0..09f9b0bf8c7e 100644 --- a/core/java/com/android/internal/os/BatteryStatsHistoryIterator.java +++ b/core/java/com/android/internal/os/BatteryStatsHistoryIterator.java @@ -24,6 +24,7 @@ import android.util.Slog; import android.util.SparseArray; import java.util.Iterator; +import java.util.Queue; /** * An iterator for {@link BatteryStats.HistoryItem}'s. @@ -48,7 +49,10 @@ public class BatteryStatsHistoryIterator implements Iterator<BatteryStats.Histor private long mBaseMonotonicTime; private long mBaseTimeUtc; 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, long endTimeMs) { @@ -62,7 +66,11 @@ public class BatteryStatsHistoryIterator implements Iterator<BatteryStats.Histor @Override public boolean hasNext() { if (!mNextItemReady) { - advance(); + if (!advance()) { + mHistoryItem = null; + close(); + } + mNextItemReady = true; } return mHistoryItem != null; @@ -75,35 +83,48 @@ public class BatteryStatsHistoryIterator implements Iterator<BatteryStats.Histor @Override public BatteryStats.HistoryItem next() { if (!mNextItemReady) { - advance(); + if (!advance()) { + mHistoryItem = null; + close(); + } } mNextItemReady = false; return mHistoryItem; } - private void advance() { - while (true) { - if (mItemIndex > mMaxHistoryItems) { - Slog.wtfStack(TAG, "Number of battery history items is too large: " + mItemIndex); - break; - } + private boolean advance() { + if (mParcelContainers == null) { + mParcelContainers = mBatteryStatsHistory.getParcelContainers(mStartTimeMs, mEndTimeMs); + } - Parcel p = mBatteryStatsHistory.getNextParcel(mStartTimeMs, mEndTimeMs); - if (p == null) { - break; + BatteryStatsHistory.BatteryHistoryParcelContainer container; + while ((container = mParcelContainers.peek()) != null) { + Parcel p = container.getParcel(); + if (p == null || p.dataPosition() >= p.dataSize()) { + container.close(); + mParcelContainers.remove(); + mParcelDataPosition = 0; + continue; } if (!mTimeInitialized) { - mBaseMonotonicTime = mBatteryStatsHistory.getHistoryBufferStartTime(p); + mBaseMonotonicTime = container.getMonotonicStartTime(); mHistoryItem.time = mBaseMonotonicTime; mTimeInitialized = true; } try { 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) { Slog.wtf(TAG, "Corrupted battery history", t); - break; + return false; } 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); } - mHistoryItem.currentTime = mBaseTimeUtc + (mHistoryItem.time - mBaseMonotonicTime); + if (mHistoryItem.time < mStartTimeMs) { + continue; + } - if (mEndTimeMs != 0 && mHistoryItem.time >= mEndTimeMs) { - break; + if (mEndTimeMs != 0 && mEndTimeMs != MonotonicClock.UNDEFINED + && mHistoryItem.time >= mEndTimeMs) { + return false; } - if (mHistoryItem.time >= mStartTimeMs) { - mItemIndex++; - mNextItemReady = true; - return; + + if (mItemIndex++ > mMaxHistoryItems) { + Slog.wtfStack(TAG, "Number of battery history items is too large: " + mItemIndex); + return false; } - } - mHistoryItem = null; - mNextItemReady = true; - close(); + mHistoryItem.currentTime = mBaseTimeUtc + (mHistoryItem.time - mBaseMonotonicTime); + return true; + } + return false; } private void readHistoryDelta(Parcel src, BatteryStats.HistoryItem cur) { diff --git a/services/core/java/com/android/server/power/stats/BatteryHistoryDirectory.java b/services/core/java/com/android/server/power/stats/BatteryHistoryDirectory.java index 5563f98e8842..7cd9bdbc662c 100644 --- a/services/core/java/com/android/server/power/stats/BatteryHistoryDirectory.java +++ b/services/core/java/com/android/server/power/stats/BatteryHistoryDirectory.java @@ -374,6 +374,10 @@ public class BatteryHistoryDirectory implements BatteryStatsHistory.BatteryHisto @SuppressWarnings("unchecked") @Override public List<BatteryHistoryFragment> getFragments() { + if (!mLock.isHeldByCurrentThread()) { + throw new IllegalStateException("Reading battery history without a lock"); + } + ensureInitialized(); return (List<BatteryHistoryFragment>) (List<? extends BatteryHistoryFragment>) mHistoryFiles; @@ -443,44 +447,6 @@ public class BatteryHistoryDirectory implements BatteryStatsHistory.BatteryHisto } @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 public boolean hasCompletedFragments() { ensureInitialized(); diff --git a/services/tests/powerstatstests/src/com/android/server/power/stats/BatteryStatsHistoryTest.java b/services/tests/powerstatstests/src/com/android/server/power/stats/BatteryStatsHistoryTest.java index 5165e34c7fcd..fc864dd230d9 100644 --- a/services/tests/powerstatstests/src/com/android/server/power/stats/BatteryStatsHistoryTest.java +++ b/services/tests/powerstatstests/src/com/android/server/power/stats/BatteryStatsHistoryTest.java @@ -22,6 +22,7 @@ import static com.google.common.truth.Truth.assertWithMessage; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.spy; @@ -330,31 +331,24 @@ public class BatteryStatsHistoryTest { return invocation.callRealMethod(); }).when(mHistory).readFragmentToParcel(any(), any()); - // Prepare history for iteration - mHistory.iterate(0, MonotonicClock.UNDEFINED); - - Parcel parcel = mHistory.getNextParcel(0, Long.MAX_VALUE); - assertThat(parcel).isNotNull(); - assertThat(mReadFiles).containsExactly("123.bh"); - - // Skip to the end to force reading the next parcel - parcel.setDataPosition(parcel.dataSize()); - 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"); + int eventsRead = 0; + BatteryStatsHistoryIterator iterator = mHistory.iterate(0, MonotonicClock.UNDEFINED); + while (iterator.hasNext()) { + HistoryItem item = iterator.next(); + if (item.eventCode == HistoryItem.EVENT_JOB_START) { + eventsRead++; + 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"); + } + } - parcel.setDataPosition(parcel.dataSize()); - mReadFiles.clear(); - parcel = mHistory.getNextParcel(0, Long.MAX_VALUE); - assertThat(parcel).isNull(); - assertThat(mReadFiles).isEmpty(); + assertThat(eventsRead).isEqualTo(3); + assertThat(mReadFiles).containsExactly("123.bh", "1000.bh", "2000.bh", "3000.bh"); } @Test @@ -372,25 +366,19 @@ public class BatteryStatsHistoryTest { return invocation.callRealMethod(); }).when(mHistory).readFragmentToParcel(any(), any()); - // Prepare history for iteration - mHistory.iterate(1000, 3000); - - Parcel parcel = mHistory.getNextParcel(1000, 3000); - assertThat(parcel).isNotNull(); - assertThat(mReadFiles).containsExactly("1000.bh"); - - // Skip to the end to force reading the next parcel - parcel.setDataPosition(parcel.dataSize()); - mReadFiles.clear(); - parcel = mHistory.getNextParcel(1000, 3000); - assertThat(parcel).isNotNull(); - assertThat(mReadFiles).containsExactly("2000.bh"); + BatteryStatsHistoryIterator iterator = mHistory.iterate(1000, 3000); + while (iterator.hasNext()) { + HistoryItem item = iterator.next(); + if (item.eventCode == HistoryItem.EVENT_JOB_START) { + fail("Event outside the range"); + } else if (item.eventCode == HistoryItem.EVENT_JOB_FINISH) { + assertThat(mReadFiles).containsExactly("1000.bh"); + } else if (item.eventCode == HistoryItem.EVENT_ALARM) { + fail("Event outside the range"); + } + } - parcel.setDataPosition(parcel.dataSize()); - mReadFiles.clear(); - parcel = mHistory.getNextParcel(1000, 3000); - assertThat(parcel).isNull(); - assertThat(mReadFiles).isEmpty(); + assertThat(mReadFiles).containsExactly("1000.bh", "2000.bh"); } private void prepareMultiFileHistory() { |