Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support simple perf output #2011

Merged
merged 1 commit into from
May 17, 2019

Conversation

julienw
Copy link
Contributor

@julienw julienw commented May 13, 2019

@@ -27,6 +27,29 @@ describe('converting Linux perf profile', function() {
}
expect(version).toEqual(CURRENT_GECKO_VERSION);
});

it('should import a simple perf profile', async function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked that this test throws on master.

@julienw julienw force-pushed the handle-simple-perf-output branch from 36dacd1 to 67e9e04 Compare May 13, 2019 17:05
@codecov
Copy link

codecov bot commented May 13, 2019

Codecov Report

Merging #2011 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2011      +/-   ##
=========================================
+ Coverage   84.79%   84.8%   +<.01%     
=========================================
  Files         191     191              
  Lines       13013   13015       +2     
  Branches     3245    3246       +1     
=========================================
+ Hits        11035   11037       +2     
  Misses       1797    1797              
  Partials      181     181
Impacted Files Coverage Δ
src/profile-logic/import/linux-perf.js 95.34% <100%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dda732...ebe711e. Read the comment docs.

// | | | +- unknown number between brackets (optional)
// | | | | +- timestamp
// vvvvv vvvvvvvvv vvv vvvvvvvvvvvvvv vvvvvv
return /^\S.+?\s+(?:\d+\/)?\d+\s+(?:\[\d+\]\s+)?[\d.]+:/.test(firstLine);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely rewrote this regexp to make it look closer to the other regexps in the same file.

@julienw julienw force-pushed the handle-simple-perf-output branch from 67e9e04 to 90bb04c Compare May 13, 2019 17:19
@julienw julienw force-pushed the handle-simple-perf-output branch from 90bb04c to ebe711e Compare May 13, 2019 17:19
@@ -133,25 +139,29 @@ export function convertPerfScriptProfile(profile: string): Object {
let startTime = 0;
while (lineIndex < lines.length) {
const sampleStartLine = lines[lineIndex++];
if (sampleStartLine === '') {
continue;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helps avoid some useless warning.

// | | +- tid
// | | | +- end of word (space or end of string)
// vvvv vvvvvvvvvvv vvvvv v
const threadNamePidAndTidMatch = /^(.*)\s+(?:(\d+)\/)?(\d+)\b/.exec(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look, a \s instead of a space, to match tabs!

// First, get the sample's time stamp and whatever comes before the timeStamp:
const sampleStartMatch = /^(.*) ([\d.]+):/.exec(sampleStartLine);
// First, get the sample's time stamp and whatever comes before the timestamp:
const sampleStartMatch = /^(.*)\s+([\d.]+):/.exec(sampleStartLine);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look, a \s instead of a space, to match tabs!

if (!sampleStartMatch) {
console.log(
'Could not parse line as the start of a sample in the "perf script" profile format:',
'Could not parse line as the start of a sample in the "perf script" profile format: "%s"',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes it easier to see the result when the string is empty.

@julienw julienw requested a review from jesup May 13, 2019 17:21
@julienw
Copy link
Contributor Author

julienw commented May 13, 2019

Hey @jesup, I flagged you for a review because you wrote the initial implementation and have some knowledge of the simple perf format. But please tell me if you'd rather have somebody else to review it :)
Thanks again for your insight on the issue!

@julienw julienw requested a review from mstange May 16, 2019 13:45
Copy link
Contributor

@mstange mstange left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for the comments on the diff.

@julienw julienw merged commit 1e472ca into firefox-devtools:master May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Android SimplePerf captures
2 participants