4.77 MB, text/plain
120.03 KB, image/png
44 bytes, text/x-github-pull-request
|Details | Review | Splinter Review|
7.03 KB, patch
|Details | Diff | Splinter Review|
Instrument the SPS profiler to use vsync marker data. This will enable us to see when we miss a frame.
Created attachment 8497189 [details] [diff] [review] Output Vsync Timestamps in Profiles The overall goal of this bug is to instrument the profiler to output HardwareComposer vsync times and display those in cleopatra. This portion enables the gecko profiler to create ProfilerMarkerPayloads at vsync intervals on the Compositor thread so we only have to profiler the compositor thread to get vsync markers.
Created attachment 8497192 [details] [review] Pull Request - Enable Cleopatra to display vsync markers
From comment 1, the part I really dislike is how to synchronize between the android timestamp and mozilla timestamp. If a vsync interval is 16.6ms, and the HwcComposer::Vsync executes within roughly the same interval, we create a marker as the current timestamp. Then we dispatch vsyncs in TimeDuration::FromMicroseconds(androidTimestamp) distances from the start point. Otherwise, if we take TimeStamp::Now() in HwcComposer::Vsync, we will get timestamps of when the function executes, not when the vsync actually occurred. On a flame, using mozilla::TimeStamp now drifts between 14ms - 18ms which is not very good. If you have a better idea of how to sync mozilla::TimeStamps and Android TimeStamps, that'd be great!
Created attachment 8497589 [details] [diff] [review] Output Vsync Timestamps in Profiles Found a better way. During HwcComposer::Init, we take timestamps in both android and mozilla timestamp objects. We use this as a reference point to create a Mozilla::TimeStamp object that is aligned with android timestamps.
Attachment #8497589 - Flags: review?(bgirard) → review+
I also reviewed the github changes. Looks really nice. Great work!
Created attachment 8498251 [details] [diff] [review] Output Vsync Timestamps in Profiles. r=benwa Carrying r+. Fixed a whitespace and updated patch to proper title. Try: https://tbpl.mozilla.org/?tree=Try&rev=caddcd330ce8. Also updated MDN to document how to read the profiler frames data.
Attachment #8498251 - Flags: review+
Attachment #8497589 - Attachment is obsolete: true
Comment on attachment 8497192 [details] [review] Pull Request - Enable Cleopatra to display vsync markers Updated github pull request with comments from the pull request. I also made the vsync marker 0.5 so it makes it harder to tell it is a bar. Also have a wiki from comment 8.
Comment on attachment 8497192 [details] [review] Pull Request - Enable Cleopatra to display vsync markers Landed on Cleopatra Front End
Try issues on OS x 10.8. Trying again - https://tbpl.mozilla.org/?tree=Try&rev=36795dc453ba
Created attachment 8499807 [details] [diff] [review] Bug 1073545 - Add Vsync Timestamps to Profiler. r=benwa Carrying r+. Modified to fix a build issue on b2g emulators. Successful try - https://tbpl.mozilla.org/?tree=Try&rev=7e403d341172
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.