[Fireperf][AASA] change timebase to elapsedRealtime
#4024
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Timer timebase to elapsedRealtimeelapsedRealtime
Coverage Report 1Affected Products
Test Logs
Notes |
leotianlizhan
commented
Aug 24, 2022
raymondlam
reviewed
Aug 25, 2022
firebase-perf/src/main/java/com/google/firebase/perf/util/Timer.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/util/Timer.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/util/Timer.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/util/Timer.java
Outdated
Show resolved
Hide resolved
raymondlam
reviewed
Aug 26, 2022
firebase-perf/src/main/java/com/google/firebase/perf/util/Timer.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/metrics/AppStartTrace.java
Outdated
Show resolved
Hide resolved
Size Report 1Affected Products
Test Logs
Notes |
raymondlam
approved these changes
Aug 26, 2022
firebase-perf/src/main/java/com/google/firebase/perf/util/Timer.java
Outdated
Show resolved
Hide resolved
|
/retest |
raymondlam
approved these changes
Aug 30, 2022
|
/retest |
visumickey
approved these changes
Aug 31, 2022
firebase-perf/src/main/java/com/google/firebase/perf/util/Timer.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/main/java/com/google/firebase/perf/util/Timer.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /** resets the start time */ | ||
| public void reset() { | ||
| timeInMicros = TimeUnit.MILLISECONDS.toMicros(System.currentTimeMillis()); | ||
| highResTime = System.nanoTime(); | ||
| // TODO: consider removing this method and make Timer immutable thus fully thread-safe |
There was a problem hiding this comment.
Why do we need reset API? Is there a place where this is used?
There was a problem hiding this comment.
It's used for networking classes like FirebasePerfHttpClient and its tests.
I also don't think we need this API when we can just do timerVariable = new Timer(); if you want to "reset" timerVariable. I also want to create a new Timer.now() static factory method to replace Timer() constructor in the future, so timerVariable = Timer.now() also makes more sense to read than reset().
However both of these changes touch a lot of files so I don't think this PR is the place to do it.
| highResTime = System.nanoTime(); | ||
| // TODO: consider removing this method and make Timer immutable thus fully thread-safe | ||
| wallClockMicros = wallClockMicros(); | ||
| elapsedRealtimeMicros = elapsedRealtimeMicros(); | ||
| } | ||
|
|
||
| /** Return wall-clock time in microseconds. */ | ||
| public long getMicros() { |
There was a problem hiding this comment.
All the public APIs are hard to understand. How hard/tricky is to refactor them?
For eg:
getMicros could be getCurrentWallClockTimeMicros
gerDurationMacros to getDurationFromStarttime
gerDurationMacros(Timer) to getDurationBetweenTimersMicros(Timer)
There was a problem hiding this comment.
For getMicros, I don't know the original author's thoughts exactly but I'd imagine it's because they wanted to encapsulate "wall-clock" and "elapsedRealtime" so anyone who uses Timer don't need to understand them. Unfortunately they added getElapsedRealtimeMicros() for testing purposes which makes getMicros confusing and defeats the whole purpose of encapsulation.
I'm not against changing the name because I also like more explicit names, but I also propose that we remove getElapsedRealtimeMicros() because it's test-only.
There was a problem hiding this comment.
How hard/tricky is to refactor them?
It's hard as in these changes touch a lot of files and I don't feel like it makes sense to make a feature-implementation PR heavy with other things for reviewers. I feel like this PR should be focused on getting the elapsedRealtime-change right, and I would like to do other code health improvements in a separate PR.
firebase-perf/src/test/java/com/google/firebase/perf/util/TimerTest.java
Outdated
Show resolved
Hide resolved
jeremyjiang-dev
approved these changes
Aug 31, 2022
raymondlam
reviewed
Sep 1, 2022
firebase-perf/src/test/java/com/google/firebase/perf/util/TimerTest.java
Outdated
Show resolved
Hide resolved
firebase-perf/src/test/java/com/google/firebase/perf/util/TimerTest.java
Show resolved
Hide resolved
qdpham13
pushed a commit
that referenced
this pull request
Sep 13, 2022
* change Timer timebase to elapsedRealtime * address comments * remove code that should be left to another CL * remove unnecessaryy new object in constructor * small change * revert to store in microseconds due to too many tests being broken if not * fix comment * add tests for ofElapsedRealtime * deflake time comparison tests * ShadowSystemClock and remove getElapsedRealtimeMicros * grammar * stylize and add comments to test
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
b/243537606
Blocked by #4027
Main change
Change
Timer.java's timebase toelapsedRealtimefor consistency with Android platform API. Platform APIs likeProcess.startElapsedRealtime()are in that timebase, so if we want to find duration between our timestamps and Android API's, then they must be the same timebase.Additional code health improvement
Changed variable names and some Javadoc, because the old ones are incorrect and misleading. We aren not using different APIs for wallclock time (
timeInMicros) and duration-calculating-time (highResTime) because duration-calculating-time is high-resolution. We are separating them because wall-clock is not monotonic. See the top of this doc https://developer.android.com/reference/android/os/SystemClock to learn more.