swift-composable-architecture
swift-composable-architecture copied to clipboard
pointfreeco
Show runtime warnings when the stack allocation reaches 95%
Following #752, this PR makes Store show runtime warnings when the stack depth reaches 95%. There are many things to discuss, so obviously a draft for now.
Right now, each store can present such warnings, but it should maybe be restricted to the root store only.
As runtime warnings are inconvenient to write with their StaticString, I'm waiting for feedback to decide if we should produce different messages depending on the nature of the event (when receiving an action, the action itself could be the type responsible for a stack overflow).
I've unified Thread checks and Stack checks under the same banner and renamed ThreadStatus into StoreEvent accordingly.
Right now, the threshold is at 95%. This is tricky because we don't want false positives, but we also want at least one warning to be emitted before reaching the overflow. In the discussions, I've seen folks reporting 20kB states. When the stack size is 512kB (iOS, background), that's around 5%, so I guess 95% it's a fair value. We can probably go down to 90/92% to have more headroom. We could also adjust automatically according to the stack size if we assume that a 20kB state is already problematic, and emit warnings when we reach around 50kB from the end.
When the warning is emitted, the user can see the current stack status and the store's state size. I've added a few comments about workarounds. Nothing definitive of course, but I think you'll get the idea.
What do you think about this?
The CI failure was weird. I've only lowered the trigger to 85% and it failed on 13.2.1. Could runtime warnings make tests fail? In this case, it could probably mean that we reached 85% in the failing tests on 13.2.1, which would be a surprise! Or it's some unrelated flakiness.
After thinking a little more about it, I now think it should only warn once per thread's lifetime. I don't see the point in warning the user more than this, as all warnings are likely sharing the same causes. As a consequence, I also think that we should not warn in scoped store: we may report a totally acceptable leaf state size, and potentially miss a larger one at the root. The root store will always participate, so it will likely spot the incoming overflow anyway. I'll push another commit in this spirit.
I can't explain why it fails on CI: Failures are not involving StackChecks, they seem to disappear if I raise the threshold from 85 to 95%, but on my machine, executing the same command on the same Xcode version, they all pass, and they're still passing with a threshold at 15%. I understand that some low-level stuff is happening. Am I doing something totally wrong and corrupting something in some way?
@tgrapperon The failure you're getting is:
[ComposableArchitectureTests.ReducerTests testDebug] :
Asynchronous wait failed: Exceeded timeout of 2 seconds, with unfulfilled expectations: "logs".
Pretty sure testDebug is just a bit flaky. I've had it fail a few times before. CI doesn't have as much power as your local machine which is why you're seeing the discrepancy.
https://github.com/pointfreeco/swift-composable-architecture/blob/9c47990ee5d2e2e979237404e936ee062c9a36e3/Tests/ComposableArchitectureTests/ReducerTests.swift#L130
From https://github.com/pointfreeco/swift-composable-architecture/pull/1018#issuecomment-1065983632
@mbrandonw @stephencelis CI failed at ReducerTests.testDebug which I don't think is impacted by this change.
Asynchronous wait failed: Exceeded timeout of 2 seconds, with unfulfilled expectations: "logs". swift-composable-architecture/Tests/ComposableArchitectureTests/ReducerTests.swift Line 130 in d924b9a self.wait(for: [logsExpectation], timeout: 2) Think this is similar to https://github.com/pointfreeco/swift-composable-architecture/pull/181
@iampatbrown Thanks for the feedback. What's puzzling me is I can probably make this go away by raising the threshold for stack-related warnings from 85% to 95%, as this is the only change I made the first time it happened. This seems completely unrelated, and the threshold is not met in both cases. And the low-level stuff happens before checking it anyway. The only things that follow are the thread dictionary thing (that wasn't implemented when the failure first appeared in this PR), and the RW, which I suspected to be the cause. I'll raise it back to 95% in the meantime.
@iampatbrown Also thanks for pointing out the correct test, I was reading its content as the error message from the test. It makes a little more sense!
Hey @tgrapperon, thanks for exploring this. It's definitely interesting, but we want to confirm that the purple runtime warnings show when running on a device, and not just the simulator. Have you tested that?
Yes they do! (I've removed the threshold check to force them, hence the 2%)
I've refined the warning message to show the type of the State and added "bytes" units.
I'm sure a native speaker could formulate this more idiomatically.
Again, 0% because I'm forcing them. It should be something above 95% on iOS or 99% on macOS.
With the release of the reducer protocol, I feel that there will be less traction for this PR. I'll close it for now and possibly revisit if folks are reporting stack overflows even with protocols.