serde-rs
Clean up unreachable_unchecked
I would like to understand #1275 before landing any workaround. Was the unreachable_unchecked not actually unreachable? If there is a compiler bug has that been minimized and reported?
@dtolnay to be clear, I haven't investigated that at all: I came across this during unsafe review and didn't think the unsafe was necessary here.
I think this removes some unsafe in a way that ought not affect performance, and that makes it a good change on its own. It's not a workaround.
I do not have access to the code in #1275 and cannot debug it. They are welcome to apply this patch and see if it changes anything. I suspect it won't, I suspect their leak comes from elsewhere.
I think this removes some unsafe in a way that ought not affect performance
Based on the benchmark in the PR that introduced this code, on my machines this PR regresses serialization performance by 22% (2990WX) or 51% (M4 Max).
Oh, hm. That's not good.
I'm pretty sure this code can be written without the unreachable, but perhaps my way of doing it has a problem. Either the char_escape match is broken, or the array indexing-and-match introduces an additional branch, is my guess.
Alternate idea: we stick a debug_assert! there and then use a continue or something.
I'm working on an isolate repro scenario however I've been so busy with work that I haven't had time to get anywhere yet. Reading the docs on uncreachable_unchecked it makes me nervous and also very easy to see why in our use case it's likely something that hasn't had a lot of testing put into place. We are compiling to WASM and running this code as a plugin to Envoy. My assumption is that most of the performance/regression testing is is done platform specific code.
