feat(perf): support reading symbols from jitdump #1051
Conversation
maxbrunet
force-pushed
the
feat/perf/jitdump
branch
from
bfefe1b to
b799c8d
Compare
| var m Map | ||
| var err error | ||
| switch { | ||
| case strings.HasSuffix(perfFile, ".map"): |
There was a problem hiding this comment.
How about converting the Map to an interface type and have 2 different implementations for .map and .dump?
There was a problem hiding this comment.
Lookup(addr uint64) (string, error) can be the only method.
There was a problem hiding this comment.
Sounds good, the idea went through my mind, I wired things up quickly to get it to work
There was a problem hiding this comment.
Actually, the interface feels like over-engineering too me, MapFromDump() would still be needed to compute end addresses and sort them to allow for binary search. And we would still have a switch-case to instantiate one or the other
There was a problem hiding this comment.
I don't think it's over-engineering. It will make this cleaner and easier to read. We write for humans to read and computers to run 😄
There was a problem hiding this comment.
hmm my thought exactly, simplicity is good for humans, the pattern would feel a bit forced to me 🙂
Anyway I'd like to keep changes to the minimum in this file, we can revisit in another PR, I will only fixed the performance issues I introduced
| ) | ||
|
|
||
| // JITHeader represent a jitdump file header. | ||
| type JITHeader struct { |
There was a problem hiding this comment.
nit: We should consider which types and fields we export if we don't depend on reflection or serialization.
There was a problem hiding this comment.
Agreed, but I am not sure how the package will be used yet, so I do not have an opinion on that, the ratio useful/expensive needs to be taken into account too
There was a problem hiding this comment.
I'm happy to merge this. But I see several to do items. Do you plan to handle them in this PR or shall we merge this? @maxbrunet
| var m Map | ||
| var err error | ||
| switch { | ||
| case strings.HasSuffix(perfFile, ".map"): |
There was a problem hiding this comment.
I don't think it's over-engineering. It will make this cleaner and easier to read. We write for humans to read and computers to run 😄
Yes, I'd like to handle them here, but before I do so, I would appreciate some feedback from @v-thakkar and/or @javierhonduco since self-requested a review 🙏 |
|
@kakkoyun Ready to merge! 🙇 I'll do further work in other PRs. 🙂 |
| @@ -116,7 +116,7 @@ func MapFromDump(logger log.Logger, fs fs.FS, fileName string) (Map, error) { | |||
| } | |||
| // Some runtimes update their dump all the time (e.g. libperf_jvmti.so), | |||
There was a problem hiding this comment.
Yeah this will be the norm in case you attach to a running process like parca will do.
|
I want to proceed with this. Vaishali is on PTO. @javierhonduco could you take a look at this? Is there a blocker on your side? |
|
Haven't had the time, yet. I will make sure I review it by end of the week. edit: Sorry I haven't gotten around to reviewing this. Will do next week |
|
I added tests, here is a first benchmark: $ ls -l testdata/jitdump/*.dump
-rw-r--r-- 1 maxime users 506419 Dec 9 12:11 testdata/jitdump/dotnet.dump
-rw-r--r-- 1 maxime users 13040830 Dec 9 12:11 testdata/jitdump/erlang.dump
-rw-r--r-- 1 maxime users 10582 Dec 9 12:11 testdata/jitdump/julia.dump
-rw-r--r-- 1 maxime users 13082624 Dec 9 12:11 testdata/jitdump/libperf-jvmti.dump
-rw-r--r-- 1 maxime users 1562469 Dec 9 12:11 testdata/jitdump/nodejs.dump
-rw-r--r-- 1 maxime users 6516 Dec 9 12:11 testdata/jitdump/php.dump
-rw-r--r-- 1 maxime users 129246 Dec 9 12:11 testdata/jitdump/wasmtime.dump
$ go test -bench=. -benchmem ./pkg/jit
goos: linux
goarch: amd64
pkg: github.com/parca-dev/parca-agent/pkg/jit
cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
BenchmarkLoadJITDump/dotnet-8 1468 969714 ns/op 692432 B/op 14414 allocs/op
BenchmarkLoadJITDump/erlang-8 34 34686926 ns/op 22501897 B/op 835150 allocs/op
BenchmarkLoadJITDump/julia-8 21128 49061 ns/op 26057 B/op 1147 allocs/op
BenchmarkLoadJITDump/libperf-jvmti-8 39 27826079 ns/op 20026386 B/op 695169 allocs/op
BenchmarkLoadJITDump/nodejs-8 448 2826668 ns/op 2078581 B/op 39746 allocs/op
BenchmarkLoadJITDump/php-8 38241 28755 ns/op 17012 B/op 538 allocs/op
BenchmarkLoadJITDump/wasmtime-8 7779 217346 ns/op 174055 B/op 3231 allocs/op
PASS
ok github.com/parca-dev/parca-agent/pkg/jit 11.512s |
|
Would love to see this merged! |
|
Oh, another minor thing, now we have the latest |
Sorry, what exclusion list are you referring to? I assume that would be in the |
|
New benchmark: -60% allocs! Thank you so much @javierhonduco @brancz for spotting this! Benchstat before/after |
|
Amazing, thanks so much! |
Motivation
The agent already supports reading symbols from perf map files, and there is a second format called
jitdump:https://github.com/torvalds/linux/blob/master/tools/perf/Documentation/jitdump-specification.txt
Some languages with
jitdumpsupport:libperf_jvmti.soagent library (JVM support #1)See also https://developer.arm.com/documentation/101816/0800/JIT-profiling-support/JIT-profiling
Changes
It is currently optimistic and warns if a field cannot be read, hoping the rest can be (happy to discuss if it is good or bad)/proc/<pid>/maps(the file ismmap()'d to be used as marker)Currently only JIT Code Loads records are used to mimic a Perf map, but the format has potential for more, it could be used to build Debug Info and upload to Parca, but the problem is JIT Code does not have a build ID. Perf Maps keep priority over jitdumps as they are more lightweight, but if we find advantages in the richer format, it might change in the future. (Otherwise, I might reconfigure the parser to discard unnecessary data as an optimization)
Testing
I
have in mind to addadded jitdumps and their JSON snapshot to https://github.com/parca-dev/testdataRight now, you can:
--perf-profflagERL_FLAGS='+JPperf dump'environment variable--jitdumpflag