-
Notifications
You must be signed in to change notification settings - Fork 17
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
Support faster methods of reading process memory #118
Conversation
I'm testing this in Firefox today, I want to get a feel of the speedup and make sure everything is working alright. |
Cool, I didn't actually do any benchmarking as this was just a side task while doing other stuff, so it'd be good to see in a real world scenario if this moves the needle (I would hope it would when doing 1 syscall vs n syscalls :p) |
Didn't have time to review properly but I did test it, and it's looking good! On my machine, using a debug build of Firefox it takes 160-180ms to grab a minidump w/o this patch. With this applied it drops to 50-75ms. I hope to have time to test an optimized build tomorrow. I also tried on a cheap Android tablet but the I/O is so slow there that the improvements are lost in the noise. |
I guess one thing to note is that I don't think the file path is being taken in any test so I'm not sure about that one, I'll add a test specifically for that before merging. |
9aaf3b5
to
3de6aeb
Compare
Sorry for the super-long delay here but I've finally had time to come back to this. I've tested this extensively on Linux builds and I see at minimum a speedup of an order of magnitude. When running Firefox crash generation tests in debug builds the speedup is 25 times (you read that right, ~12ms with the patch applied vs ~300ms without it). This is going to be extremely useful. I didn't have time to test Android again but I expect larger gains there (because syscalls are slower) but maybe a smaller impact in the overall time because I/O is also slower and takes a larger portion of the time. |
@Jake-Shadle if you don't have other changes to make we can merge it today. |
Ahh sounds good, I just want to add a test specifically for the file case since that will never currently be hit on CI/my local machine, then we can merge. |
This adds 2 new methods for reading external process memory, in addition to the existing
PTRACE_PEEKDATA
approach currently used.process_vm_readv
- Reads contiguous blocks of a specified size, available since Linux 3.2, so realistically available in every reasonable environment.../proc/{pid}/mem
- As a fallback in case running in an ancient Linux, we can read from this procfs file, which also allows easy reading of blocks of memory.PTRACE_PEEKDATA
- The reliable but extremely slow fallbackThese 3 methods are probed in the order above until one succeeds, as they all require the same permissions.
Resolves: #72