Add an env var to allow disabling debug backtrace recording in the DB pool claim code - #11232
Add an env var to allow disabling debug backtrace recording in the DB pool claim code#11232nickelization wants to merge 1 commit into
Conversation
I ran into some issues running simulated Omicron on macOS because the cost of generating a backtrace was high for the debug build I was using; the whole startup process slowed down enough that we hit the 10 minute timeout and failed on the `start_nexus_external` step. I didn't manage to get numbers on exactly how slow each individual backtrace was, but sampling the call stacks showed lots of threads stuck in this function. Since the docs say this can be expensive in certain circumstances (and especially since backtrace generation is guarded by a global mutex) it's not entirely surprising that this could clog things up. My understanding is this has been measured on Illumos and found to be cheap there, so I don't expect this is something that would manifest in production, but a macOS debug build obviously has plenty of differences from an Illumos release build, so it's not super surprising on its own that we might stumble across a slow path somewhere in there. The odd thing is that although I was able to fix it by locally disabling the call to `Backtrace::force_capture`, I wasn't able to reproduce the problem anymore after I reenabled it. I'm not sure what could have possibly changed, though I did notice that the build size dropped from 1.9GB to 1.2GB, so something weird is definitely going on with my dev setup and I can't for the life of me figure out what happened. All that said...@davepacheco mentioned it might be reasonable to have a switch for this code anyway, and since I already wrote the flag I figured I'd whip up a PR for this while it's fresh on my mind.
davepacheco
left a comment
There was a problem hiding this comment.
Sorry about the trouble. I should have heeded the warning about this being expensive on some platforms!
I do think it's reasonable to have a switch to turn this off. I don't love sticking a getenv call right in this hot path. Thinking out loud: in an ideal world it could be an argument to omicron-dev, though that wouldn't work for the test suite. What about having the ControlPlaneTestContext setup (which is used for both omicron-dev run-all and all the tests) check for an env var like this and plumb some configuration through accordingly? It would probably be an annoying amount of plumbing, though it would ensure that this couldn't be set outside of those contexts. Alternatively, the Pool could do this at construction time, but it doesn't seem right for some arbitrary code like this to look at the environment instead of passing configuration in.
I'd also suggest checking the specific value for the variable (like 1). (I hate having to unset a variable rather than just setting it to 0 to turn it off. It also makes it harder to programmatically clear.)
I ran into some issues running simulated Omicron on macOS because the cost of generating a backtrace was high for the debug build I was using; the whole startup process slowed down enough that we hit the 10 minute timeout and failed on the
start_nexus_externalstep.I didn't manage to get numbers on exactly how slow each individual backtrace was, but sampling the call stacks with
sampleshowed lots of instances of threads stuck in this function. Since the docs say this can be expensive in certain circumstances (and especially since backtrace generation is guarded by a global mutex) it's not entirely surprising that this could clog things up, even though we haven't run into this before on other platforms. My understanding is this has been measured on Illumos and found to be cheap there, so I don't expect this is something that would manifest in production, but a macOS debug build obviously has plenty of differences from an Illumos release build, so it makes sense to me that I could've stumbled across a slow path somewhere in there.The odd thing is that although I was able to fix it by locally disabling the call to
Backtrace::force_capture, I wasn't able to reproduce the problem anymore after I re-enabled it. I'm not sure what could have possibly changed to make this slow path suddenly go fast on the same code, though I did notice that the build size dropped from 1.9GB to 1.2GB, so something weird is definitely going on with my dev setup and I can't for the life of me figure out what happened.All that said...@davepacheco mentioned it might be reasonable to have a switch for this code anyway, and since I already wrote the code to add a flag I figured I'd whip up a PR for this while it's fresh on my mind.