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
[core] Add lock::observing
module, for analyzing lock acquisition.
#5586
base: trunk
Are you sure you want to change the base?
Conversation
Current output:
|
c4a5f52
to
3636eb6
Compare
3636eb6
to
91ae2c7
Compare
I'll take on reviewing this. |
Actually, this is going to need some significant changes to be rebased on top of #5603. The general idea isn't going to change but the specifics of |
91ae2c7
to
a0305bb
Compare
Okay, this version has been rebased on trunk, supports RwLock and SnatchLock, and has been cleaned up quite a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting WIP feedback, at least some of which is likely invalidated by the rebase. Looking forward to the next round!
I used Conventional Comments in this review! I hope they help with clarity and tone. 🙂
[Edit: note that this output is borked because of #5610.] Current output
|
Actually, this whole thing is borked by #5610. I'm returning this to draft for the moment. |
a0305bb
to
e38ac6b
Compare
This is now rebased on #5646. I haven't actually tried it again. |
e38ac6b
to
aaacfb6
Compare
Okay, here's a new batch of output that actually looks usable. The line numbers refer to the current tip of this branch, aaacfb6: Current output
|
Filed #5647 based on that output. |
d3b595c
to
3711329
Compare
Better output from 3711329, that lists acquisitions of leaf locks last, since they can't cause deadlocks: Output from `analyze-locks`
|
As far as I can tell this is working. I'll take it out of draft as soon as #5610 has landed. @ErichDonGubler, thanks for the WIP feedback, I'll look it over soon. |
@ErichDonGubler, I think this addresses your comments. Please look over the ones I left unresolved and see what you think. |
Prerequisites have landed, so taking this out of draft. |
Planning on reviewing on Monday, but may postpone if a higher priority arises for my personal workload. Jim is out for next week, so there's not a ton of pressure on this PR, ATM. |
@ErichDonGubler I'm "out" but not "out", if you know what I mean. It'd be helpful to me to get this landed this week. |
@jimblandy: To be clear, I don't intend to postpone longer than a day or so. I hate leaving open PRs to potentially bitrot and cause headaches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything blocking left, so ! @jimblandy: I left s'more notes I'd like to be considered for going forward, but once you mark them as Resolved
, you just need to autosquash-rebase and merge!
@jimblandy: I noticed you'd added some link entries in |
4a92bfa
to
7d5cd55
Compare
Add a new module `lock::observing`, enabled by the `observe-locks` feature, that records all nested lock acquisitions in trace files. Add a new utility to the workspace, `lock-analyzer`, that reads the files written by the `observe-locks` feature and writes out a new `define_lock_ranks!` macro invocation that covers all observed lock usage, along with comments giving the held and acquired source locations.
7d5cd55
to
97fd60c
Compare
Just rebased this change, and autosquashed. |
Add a new module
lock::observing
, enabled by theobserve-locks
feature, that records all nested lock acquisitions in trace files.Add a new utility to the workspace,
lock-analyzer
, that reads the files written by theobserve-locks
feature and writes out a newdefine_lock_ranks!
macro invocation that covers all observed lock usage, along with comments giving the held and acquired source locations.