perf(fetch): optimize normalizeMethod()

[AaronO]

Which was unnecessarily wasteful and taking up 5% of the JS CPU time in the deno_http_native bench.

It's not clear why byteUpperCase() was useful, especially since we only return the uppercased method if it matches a whitelist of unambiguous strings.

[lucacasonato]

See spec: https://fetch.spec.whatwg.org/#methods

To normalize a method, if it is a byte-case-insensitive match for DELETE, GET, HEAD, OPTIONS, POST, or PUT, byte-uppercase it.

[lucacasonato]

toUpperCase is not the same as byte uppercase in whatwg infra:

"byte uppercase": https://infra.spec.whatwg.org/#byte-uppercase
"toUpperCase": https://tc39.es/ecma262/#sec-string.prototype.touppercase

One is ASCII only, the other uses the "Unicode Default Case Conversion"

[AaronO]

toUpperCase is not the same as byte uppercase in whatwg infra:

"byte uppercase": https://infra.spec.whatwg.org/#byte-uppercase
"toUpperCase": https://tc39.es/ecma262/#sec-string.prototype.touppercase

One is ASCII only, the other uses the "Unicode Default Case Conversion"

I get that, but I don't see how the distinction is meaningful when we only return the upper cased version if it matches a fixed whitelist with strict string equality, I can't see how this could fail or have edge cases.

[lucacasonato]

It might be the case that some unicode characters "upper case" is an ASCII char. For example that é uppercases to E (not the case, but you get my point). If you are sure this is not the case currently, and will never be the case in the future, it LGTM.

[AaronO]

It might be the case that some unicode characters "upper case" is an ASCII char. For example that é uppercases to E (not the case, but you get my point). If you are sure this is not the case currently, and will never be the case in the future, it LGTM.

The only downside then would be a "false positive" normalization of an ambiguous user-provided method name. It's definitely not best-practice for users to use custom unicode method names ... so I don't think it's a huge issue or spec breaking to normalize in those situations.

Even if we want to preserve the previous behaviour, it still makes sense to add the isKnownMethod() first since that check should be true 99% of the time and is substantially faster than the spec's byte upper casing.

[ry]

let's just move this to rust

[lucacasonato]

Even if we want to preserve the previous behaviour, it still makes sense to add the isKnownMethod() first since that check should be true 99% of the time and is substantially faster than the spec's byte upper casing.

Totally. In isolation that change is great, and I think we should land it.

I don't think it's a huge issue or spec breaking to normalize in those situations

This is a very slippery slope. Having inconsistencies like these are something that some dev somewhere is going to spend hours debugging, because some edge case works (or doesn't work) in Chome, FF, and Safari, but not in Deno.

"Correctness is more important than perf" where your words, not mine ;-) https://discord.com/channels/684898665143206084/778060818046386176/830035065669025822

[AaronO]

This is a very slippery slope. Having inconsistencies like these are something that some dev somewhere is going to spend hours debugging, because some edge case works (or doesn't work) in Chome, FF, and Safari, but not in Deno.

"Correctness is more important than perf" where your words, not mine ;-) https://discord.com/channels/684898665143206084/778060818046386176/830035065669025822

I completely agree, it's better to waste machine time than hours of a person's life :)

However the tests seem to pass, shouldn't the spec / edge-cases be enforced by the WPT tests ?

[lucacasonato]

WPT is not yet enabled for fetch. I am not sure if this edge case has a WPT. If not, I'll write one and upstream it once we enable fetch WPT.

[ry]

I'm very much in favor of removing this complexity all together. If someone is using non-upper case methods they deserve the bugs.

Please see #10155

[AaronO]

I'm very much in favor of removing this complexity all together. If someone is using non-upper case methods they deserve the bugs.

Please see #10155

Pragmatically I wouldn't be opposed to it. The main thing I care about here is that we have a fast path and aren't wasting JS cpu time were we shouldn't be

[lucacasonato]

I'm very much in favor of removing this complexity all together. If someone is using non-upper case methods they deserve the bugs.

Please see #10155

This is even more spec incompliant. Someone ran into this a few days ago in fetch, and it needed to be hardened even more: #10090. I am strongly opposed to removing normalization that the spec mandates. These are the kind of inconsistencies I have been trying to slowly weed out of our implementations for the last few months. As I have mentioned previously on multiple occasions, I am going to spend the next few days and weeks making fetch spec compliant and enabling WPT. Part of that will be rewriting these APIs. I will rewrite them in such a way that we can fast path skip the entire constructor for internal operations in the HTTP server. In light of that I would be opposed to "optimizing" this any more than the current state of this PR.

[lucacasonato]

LGTM. Thanks for the changes.