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

Better support for process name with spaces #90

Merged
merged 1 commit into from
Jul 1, 2016

Conversation

skarcha
Copy link
Contributor

@skarcha skarcha commented May 17, 2016

I was playing with perf today for the first time and stackcollapse-perf.pl shows some warnings processing lines with the name of process having more than one space in it, like this one:

Plex DLNA Serve 26638 [001] 856620.530720: 2654110 cycles:

I think it also would close issue #78

I did not write perl code in the past, so please, test it... And thanks for FlameGraphs... ;)

Also removes one "if" statement.
@jkivilin
Copy link

jkivilin commented Jul 1, 2016

Thanks, this solved "Use of uninitialized value $pname" problem I ran into on Kubuntu 16.04.

@skarcha
Copy link
Contributor Author

skarcha commented Jul 1, 2016

Great @jkivilin ! I hope @brendangregg merge it... ;)

@brendangregg
Copy link
Owner

Thanks, but no, I won't be merging it -- it's messed up the logic. Take a look at the comments surrounding the regexp's, which exist as a guide to anyone making changes. And that's just a few examples that need to be parsed properly.

The first regexp is supposed to parse the TID-only case, but the new regexp matches on "/", which is for the second regexp.

@skarcha
Copy link
Contributor Author

skarcha commented Jul 1, 2016

Excuse me @brendangregg , but did you test it? In the new regexp, "/" is optional. I tried to do all the matches in only one regexp.

I tested all cases. Example: https://regex101.com/r/lX5eW5/1

Could you please take a look? Thanks.

@brendangregg
Copy link
Owner

Sorry, I now see you actually deleted the 2nd regexp! Do you know if the test/perf-* files still parse?

@skarcha
Copy link
Contributor Author

skarcha commented Jul 1, 2016

No problem!

Well I don't know how to test it properly, but this:

$ for i in test/* ; do ./stackcollapse-perf.pl $i ; done | 1> /dev/null

does not print nothing, so no warning is printed by the script... :-)

How can I test it more?

@brendangregg
Copy link
Owner

It should go like this:

$ for f in test/perf-*; do ./stackcollapse-perf.pl $f | cksum; done
3501746272 472
1207311716 27510
167978453 35177
612473039 552
901885755 4247
$ for f in test/perf-*; do ./stackcollapse-perf.pl.2 $f | cksum; done
3501746272 472
1207311716 27510
167978453 35177
612473039 552
901885755 4247

And the same for every sub option (--pid, --tid, --kernel), and checking checksums. But github nowadays has support for auto-testing of PRs, so this really should be automated ( #80 ).

Since they do match I'll merge it -- thanks!

I'll also add some more test/perf- files, since I noticed the 5 that's there are CPU only, and it should include other events.

@brendangregg brendangregg merged commit e4cd06c into brendangregg:master Jul 1, 2016
@skarcha
Copy link
Contributor Author

skarcha commented Jul 2, 2016

Oh! I see. I'll try to take a look, but I can't promise... :)

Thanks TO you.

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.

3 participants