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

improve probes lookup by dropping use of regex #557

Merged
merged 1 commit into from
Apr 25, 2019

Conversation

mmarchini
Copy link
Contributor

This PR alone won't solve the performance issues, but combined with #410 I was able to reduce startup time when looking up a large library from almost 20s to about 300ms.

Also, let's keep #526 open since there are other performance improvements we can do (for example, only expand wildcards if there's a wildcard in our probe name).

@mmarchini
Copy link
Contributor Author

Not fully working yet. Will take a look into this again tomorrow.

@mmarchini mmarchini force-pushed the improve-uprobe-lookup-performance branch from 9c7f9e5 to e87b6b0 Compare April 18, 2019 23:39
@mmarchini mmarchini marked this pull request as ready for review April 19, 2019 00:02
@mmarchini
Copy link
Contributor Author

I should probably point out that we won't be able to use regex in probe definitions with this PR, but I don't think that's an issue as long as we have wildcards working.

@mmarchini mmarchini force-pushed the improve-uprobe-lookup-performance branch 3 times, most recently from c88c742 to a115b7f Compare April 23, 2019 20:43
@mmarchini
Copy link
Contributor Author

Ok, should be good to merge if there's no objections to #557 (comment) and if the CI is green.

@mmarchini mmarchini force-pushed the improve-uprobe-lookup-performance branch from a115b7f to 5d7e4cd Compare April 23, 2019 20:54
Copy link
Member

@ajor ajor left a comment

Choose a reason for hiding this comment

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

I'm fine with not having regex anymore (it was just easier to implement wildcards that way), but this does remove the option to have wildcards in the middle of strings, e.g. tracepoint:syscalls:sys_enter_*chown I was wrong

src/bpftrace.cpp Show resolved Hide resolved
@dalehamel
Copy link
Contributor

Not sure if we should fix this here too, but list and attach should use the same method to match and filter probes, but it is currently duplicated (and inconsistent):

https://github.com/iovisor/bpftrace/blob/master/src/list.cpp#L116-L139
https://github.com/iovisor/bpftrace/blob/master/src/bpftrace.cpp#L152

Probably not in the scope of this PR, but we should cut an issue to fix that too.

@mmarchini mmarchini force-pushed the improve-uprobe-lookup-performance branch from 5d7e4cd to 5ed8717 Compare April 25, 2019 18:44
@mmarchini
Copy link
Contributor Author

@dalehamel yes, we need to improve consistency between attaching and listing probes, but it should be worked on another PR.

@mmarchini
Copy link
Contributor Author

mmarchini commented Apr 25, 2019

This is good to go from my side.

@dalehamel
Copy link
Contributor

yes, we need to improve consistency between attaching and listing probes, but it should be worked on another PR.

yup, I cut #565 to address that

@brendangregg
Copy link
Contributor

Thanks!

@brendangregg brendangregg merged commit c37f483 into master Apr 25, 2019
@ajor
Copy link
Member

ajor commented Apr 26, 2019

Nice tests!

@ajor ajor deleted the improve-uprobe-lookup-performance branch April 27, 2019 11:53
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.

None yet

4 participants