Skip to content

fix(ebpf): improve pcindex binary search speed #2255

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

Merged
merged 3 commits into from
Aug 15, 2023
Merged

Conversation

korniltsev
Copy link
Collaborator

PCIndex.FindIndex is not super hot, it is only 3%. However the change is not hard so I do it anyway. "It ain't much, but it's honest work"

In this PR I've inlined sort.Search in PCIndex.FindIndex.

goos: linux
goarch: amd64
pkg: github.com/grafana/pyroscope/ebpf/symtab/gosym
cpu: AMD Ryzen 9 5950X 16-Core Processor            
             │   old.txt    │               new.txt               │
             │    sec/op    │   sec/op     vs base                │
BinSearch-32   3.354m ± 11%   1.716m ± 1%  -48.83% (p=0.000 n=10)

Copy link
Collaborator

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

Yeah, sort.Search sometimes makes me sad. I wonder if slices.BinarySarch would be of use here – it was released as part of 1.21.0, for versions before there is x/exp/slices package

@korniltsev
Copy link
Collaborator Author

yeah slices do work, thanks

goos: linux
goarch: amd64
pkg: github.com/grafana/pyroscope/ebpf/symtab/gosym
cpu: AMD Ryzen 9 5950X 16-Core Processor            
             │   old.txt    │           new_slices.txt            │
             │    sec/op    │   sec/op     vs base                │
BinSearch-32   3.428m ± 14%   1.765m ± 2%  -48.50% (p=0.000 n=10)

I also realized that there was small inconsistency for different symbols with same addresses (for example function aliases). For example if function is at 0xff0 and it is 0x10 long and it has two names foo bar and I search for 0xff0 the pcindex would return 'foo' and if search for 0xff1 return bar . Maybe I should just remove the the unused dupes, but in a separate PR.

@korniltsev korniltsev enabled auto-merge (squash) August 15, 2023 09:00
@korniltsev korniltsev merged commit 68f7eec into next Aug 15, 2023
@korniltsev korniltsev deleted the ebpf/inline_binsearch branch August 15, 2023 09:07
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.

2 participants