Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

tsctx
Copy link
Member

@tsctx tsctx commented Mar 1, 2024

I'm not done yet.

We understand that some behaviors deviate from the specification and increase maintenance costs, but these APIs should be improved.

@tsctx tsctx force-pushed the perf/improve-fetch branch 3 times, most recently from 37d09c5 to 125eac6 Compare March 1, 2024 13:46
@ronag
Copy link
Member

ronag commented Mar 1, 2024

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

@ronag ronag requested a review from KhafraDev March 1, 2024 13:48
@tsctx tsctx force-pushed the perf/improve-fetch branch 2 times, most recently from d8829bd to e7d905f Compare March 1, 2024 13:51
Copy link
Member

@KhafraDev KhafraDev left a 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

@tsctx tsctx force-pushed the perf/improve-fetch branch 2 times, most recently from 1cf900d to e3c39ed Compare March 1, 2024 14:28
@tsctx
Copy link
Member Author

tsctx commented Mar 1, 2024

@KhafraDev I kept the changes to a minimum. Is this okay?

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.65%. Comparing base (3d9fcf5) to head (798d62e).

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@KhafraDev KhafraDev left a 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

@tsctx
Copy link
Member Author

tsctx commented Mar 1, 2024

@KhafraDev tsctx@06e14d9 (The change is not reflected in the PR, see here.)
This PR objective is, avoid using the redundant operation String.toWellFormed.
When the passed string is passed to the text encoder api, invalid utf-8 characters are replaced and need not be used.

@tsctx
Copy link
Member Author

tsctx commented Mar 1, 2024

Removing these operations would increase performance by a factor of 1.5.

@tsctx tsctx marked this pull request as ready for review March 1, 2024 15:03
@tsctx tsctx force-pushed the perf/improve-fetch branch 2 times, most recently from 8eaa6dd to 06e14d9 Compare March 1, 2024 15:28
@mcollina
Copy link
Member

mcollina commented Mar 1, 2024

wow! This seems meaningful.

@KhafraDev
Copy link
Member

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.

@tsctx
Copy link
Member Author

tsctx commented Mar 1, 2024

@KhafraDev
Copy link
Member

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:

  • A response is created with a string body such as '\ud801'
  • The body is extracted in extractBody which returns a stream that when pulled from, encodes the body with a TextEncoder
  • The TextEncoder replaces lone surrogates with u+fffd

How it worked before:

  • A response is created with a string body such as '\ud801'
  • The webidl converter in its constructor replaced lone surrogates characters with u+fffd

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

@tsctx tsctx force-pushed the perf/improve-fetch branch 3 times, most recently from 8539b33 to c9c4fae Compare March 2, 2024 06:55
@mcollina
Copy link
Member

mcollina commented Mar 2, 2024

I think it's worthwhile landing anyway, the speed bump is noticeable.

Does it affects anyhow the fetch() bench?

@tsctx tsctx force-pushed the perf/improve-fetch branch 4 times, most recently from 5040b62 to 1f902eb Compare March 2, 2024 11:22
@tsctx tsctx force-pushed the perf/improve-fetch branch 2 times, most recently from 10b2397 to 0be1e99 Compare March 2, 2024 12:36
@tsctx
Copy link
Member Author

tsctx commented Mar 2, 2024

Does it affects anyhow the fetch() bench?

Benchmarks that retrieve data show no performance improvement.
Performance is improved only when sending strings.

Benchmark added. #2905

@tsctx tsctx changed the title perf: improve fetch-api perf(fetch): avoid redundant conversion Mar 2, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@metcoder95
Copy link
Member

metcoder95 commented Mar 3, 2024

Can you rebase with main to have the benchmarks?

@tsctx tsctx force-pushed the perf/improve-fetch branch 2 times, most recently from 7209e37 to 8610021 Compare March 5, 2024 09:40
@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 5, 2024

Is it ready for review? Should @KhafraDev review again?

@tsctx tsctx closed this Jun 17, 2024
@tsctx tsctx deleted the perf/improve-fetch branch June 19, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants