Clean up unreachable_unchecked

[Manishearth]

No description provided.

[dtolnay]

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?

[Manishearth]

@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.

[Manishearth]

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.

[dtolnay]

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).

[Manishearth]

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.

[Manishearth]

Alternate idea: we stick a debug_assert! there and then use a continue or something.

[aweiker]

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.