-
Notifications
You must be signed in to change notification settings - Fork 524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(fetch): avoid redundant conversion #2894
Conversation
37d09c5
to
125eac6
Compare
I'm not maintaining the web stuff anymore but if I did, I would be against anything that deviates from spec due to maintenance overhead. @KhafraDev |
d8829bd
to
e7d905f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll make maintaining fetch much harder
1cf900d
to
e3c39ed
Compare
@KhafraDev I kept the changes to a minimum. Is this okay? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2894 +/- ##
=======================================
Coverage 93.65% 93.65%
=======================================
Files 87 87
Lines 23899 23908 +9
=======================================
+ Hits 22383 22392 +9
Misses 1516 1516 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are a lot of unnecessary changes still and I don't see any benefit
@KhafraDev tsctx@06e14d9 (The change is not reflected in the PR, see here.) |
Removing these operations would increase performance by a factor of 1.5. |
8eaa6dd
to
06e14d9
Compare
wow! This seems meaningful. |
I would call it misleading. What is being improved by 1.5x? If it's instantiating Responses, the change is wrong (we have to convert the body to a USVString), and other changes are cold paths/indentation. |
Ignore my previous comment, this will work, but in a not-so-obvious way. I'm not too sure if I like removing the explicit conversion to a USVString for the implicit conversion done in TextEncoder (and Buffer, as a side note), which took a little while to figure out. I would consider it working as a side-effect of how the body is extracted, and something that can easily break. How the PR works now:
How it worked before:
It's a noticeable difference when hunting down through multiple files why it worked. I (may falsely) assume that the reason there was no comment here was because you were confused as to why it worked as well? A test should be added to ensure that if we ever change the TextEncoder approach, lone surrogates are still replaced. Afterwards I don't have much of an issue landing this. const response = new Response('\ud801')
assert.deepStrictEqual(await response.text(), '\ufffd') |
8539b33
to
c9c4fae
Compare
I think it's worthwhile landing anyway, the speed bump is noticeable. Does it affects anyhow the fetch() bench? |
5040b62
to
1f902eb
Compare
10b2397
to
0be1e99
Compare
Benchmarks that retrieve data show no performance improvement. Benchmark added. #2905 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Can you rebase with |
7209e37
to
8610021
Compare
Is it ready for review? Should @KhafraDev review again? |
8610021
to
798d62e
Compare
798d62e
to
51a8973
Compare
I'm not done yet.
We understand that some behaviors deviate from the specification and increase maintenance costs, but these APIs should be improved.