Skip to content

interpreter/go: replace Rust with native Go symbolization#456

Merged
fabled merged 10 commits intomainfrom
rust-free-go-symbolization
Jun 26, 2025
Merged

interpreter/go: replace Rust with native Go symbolization#456
fabled merged 10 commits intomainfrom
rust-free-go-symbolization

Conversation

@florianl
Copy link
Copy Markdown
Member

@florianl florianl commented May 6, 2025

This is a follow up PR show casing the use of #455.

A side effect of this change is the reduced complexity of compilation, less cgo calls, reduction of binary size.

Fixes #457
Fixes #512

Benchmark results of current main vs this branch:

$ benchstat old.txt new.txt
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/ebpf-profiler/interpreter/go
cpu: 12th Gen Intel(R) Core(TM) i7-12700H
          │     old.txt     │               new.txt               │
          │     sec/op      │   sec/op     vs base                │
Golang-20   6375752.5n ± 6%   969.3n ± 2%  -99.98% (p=0.000 n=10)

          │  old.txt   │              new.txt               │
          │    B/op    │    B/op     vs base                │
Golang-20   848.5 ± 1%   632.0 ± 0%  -25.52% (p=0.000 n=10)

          │  old.txt   │              new.txt               │
          │ allocs/op  │ allocs/op   vs base                │
Golang-20   24.00 ± 0%   17.00 ± 0%  -29.17% (p=0.000 n=10)

I think it is important to state, that the Rust based symbolization is not the isse but the overhead introduced by cgo calling the Rust implementation in Go.

Comment thread interpreter/go/go.go Outdated
Base automatically changed from pfelf-mmap to main May 27, 2025 12:51
@florianl florianl force-pushed the rust-free-go-symbolization branch from b53c417 to 7313667 Compare May 28, 2025 07:48
@florianl florianl marked this pull request as ready for review May 28, 2025 09:57
@florianl florianl requested review from a team as code owners May 28, 2025 09:57
@florianl

This comment was marked as outdated.

@florianl florianl force-pushed the rust-free-go-symbolization branch from 03bbb05 to 1cf8d82 Compare May 30, 2025 15:01
@florianl

This comment was marked as outdated.

@florianl

This comment was marked as outdated.

@florianl florianl marked this pull request as draft June 2, 2025 07:00
Comment thread interpreter/go/go.go Outdated
// Avoid race conditions where the mmaped backed data is no longer
// available but we try to symbolize Go frames.
cpy := make([]byte, len(pclnData))
copy(cpy, pclnData)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This copy is unnecessary because the SearchGoPclntab function has already loaded the data into a slice via p.Data(maxBytesGoPclntab). In fact, there's no need to save this data at all - we only need to keep the symTable returned by gosym.NewTable(nil, pcln).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These two lines of code were the result of tests. And in the case, where the executable is no longer available, but we still try to symbolize frames for it, it will run into a panic, if this copy does not exist.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As incredible as it sounds, isn't pclnData a slice with memory already allocated?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As incredible as it sounds, isn't pclnData a slice with memory already allocated?

It was mmapped byte slice. Which gets unmapped when the ELF is Closed. This can be fixed by adding a reference counting mechanism, or having this interpreter plugin steal the pfelf.File from pfelf.Reference to keep it open.

The other bigger problem is that debug/gosym on opening allocates huge amount of memory linear to the number of symbols/functions, and further may allocate more memory on each symbolization account.

I will likely extend our existing elfgopclntab.go code to expose a similar Go symbolization API as debug/gosym which this interpreter plugin can then use. I believe we can implement this in a way that there will be no dynamic memory allocations.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for explaining!

Copy link
Copy Markdown
Member

@athre0z athre0z Jun 3, 2025

Choose a reason for hiding this comment

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

I believe we can implement this in a way that there will be no dynamic memory allocations.

Yes, it's definitely possible -- the Rust impl also doesn't alloc or copy anything, except for the symbol / file name strings when going through the C API / FFI. I documented the symbol / inline table format here; may be useful if you want to go down the route of teaching elfgopclntab to support that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I documented the symbol / inline table format here; may be useful if you want to go down the route of teaching elfgopclntab to support that.

Yes, I saw that. Appreciated!

Though, currently it seems the Rust point resolver frontend for Golang does not support reporting inlining information. So just doing the basic non-inlined version to start with should be on par with the feature set. Right?

Copy link
Copy Markdown
Member

@athre0z athre0z Jun 3, 2025

Choose a reason for hiding this comment

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

Yes, that is correct. The (Rust) gosym library fully supports providing all the necessary data, though -- I think it wouldn't be more than 10-30 lines to actually also use that data in the Rust point resolver. With point resolving it's much simpler to use the data than when generating symbfiles for it, since the latter involves a lot of complicated range tree fumbling.

The inline structure stuff is a bit more painful to extract because it needs access to moduledata (runtime.firstmoduledata), which isn't exported, so the lib locates it with a pattern search.

So yeah, long story short: it would currently not be a reduction in the feature set to not supported it, but in the Rust impl it would be quite easy to add since all the annoying plumbing is already done.

Copy link
Copy Markdown
Contributor

@fabled fabled Jun 3, 2025

Choose a reason for hiding this comment

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

I do have a strong preference to do this in Go if possible. We've already seen the overhead of bringing the Rust wold in (executable size larger), having two runtimes with two memory subsystems, bundling also the Rust .a in Git and making sure it runs on glibc and musl OSes, managing the new Rust dependencies (lot of PR cruft), the memory management issues, the CGO overhead of calling Rust, and the allocation overhead of just moving strings from Rust to Go all add up to a bundle of overhead.

I think I can implement this in Go in a small enough time to warrant doing this. One time development overhead would also mean saved hours in the long term to not needing to update the Rust dependencies. Even if its automated, its still manual review and fixing if something breaks...

Copy link
Copy Markdown
Member

@athre0z athre0z Jun 4, 2025

Choose a reason for hiding this comment

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

Yeah, fair. Especially having the static libs in git and all the linker foo is indeed rather atrocious for maintenance.

@florianl florianl force-pushed the rust-free-go-symbolization branch 2 times, most recently from d161054 to 81a2d9f Compare June 13, 2025 06:11
fabled added a commit that referenced this pull request Jun 17, 2025
This refactors elfgopclntab code to be more readable, and implements support for Golang symbolization using mmap backed data.

A building block for #456 to not depend on debug/elf and allow Golang symbolization without excessive memory usage. So the first element in solving the recent memory usage increases and dependency on CGO.
florianl and others added 6 commits June 18, 2025 07:38
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
When searching .gopclntab fall back to Sections if not detected in in Programs.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Reference count pfelf.File and take a reference in interpreter/go.
This allows avoiding copying the .gopclntab, and yet unmap the
file when it no longer is needed.
Partial commit from fabled@261a146

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@florianl florianl force-pushed the rust-free-go-symbolization branch from 8b87e5d to 38f07b8 Compare June 18, 2025 06:04
@florianl florianl marked this pull request as ready for review June 18, 2025 06:11
@florianl florianl requested review from athre0z and fabled June 18, 2025 06:11
Copy link
Copy Markdown
Contributor

@fabled fabled left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. Some observations listed - mostly related to code I wrote :)

Comment thread libpf/pfelf/file.go Outdated
Comment thread nativeunwind/elfunwindinfo/elfgopclntab.go Outdated
Comment thread tools/coredump/testdata/arm64/hello.3345.leaf.ret.json Outdated
Copy link
Copy Markdown
Contributor

@fabled fabled left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

Comment thread libpf/pfelf/internal/mmap/mmap.go Outdated
#456 (comment)

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
@fabled fabled merged commit 06a4afd into main Jun 26, 2025
25 checks passed
@fabled fabled deleted the rust-free-go-symbolization branch June 26, 2025 08:48
florianl added a commit that referenced this pull request Jun 30, 2025
Since #456 the eBPF Profiler agent can be build without Rust components.
To speed up CI, separate Rust and Go tests.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
florianl added a commit that referenced this pull request Jun 30, 2025
Since #456 the eBPF Profiler agent can be build without Rust components.
To speed up CI, separate Rust and Go tests.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
florianl added a commit that referenced this pull request Jun 30, 2025
Since #456 the eBPF Profiler agent can be build without Rust components.
To speed up CI, separate Rust and Go tests.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
florianl added a commit that referenced this pull request Jun 30, 2025
Since #456 the eBPF Profiler agent can be build without Rust components.
To speed up CI, separate Rust and Go tests.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
florianl added a commit that referenced this pull request Jun 30, 2025
Since #456 the eBPF Profiler agent can be build without Rust components.
To speed up CI, separate Rust and Go tests.

Signed-off-by: Florian Lehner <florian.lehner@elastic.co>
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.

runc, crictl, and csi-node-driver-registrar errors ebpf profiler is using excessive amount of memory

5 participants